Sergei Golubchik <serg@askmonty.org> writes:
Just one question and couple of suggestions below. If you agree with them - please change accordingly and push!
Thanks for review! I believe I addressed all your suggestions. Comments below. - Kristian.
+ $base_dir= dirname($file) + unless defined($base_dir);
why do you pass base_dir as an argument, instead of always using basedir($file) ?
It was a mistake. I was trying to preserve old behaviour, but misunderstood what that behaviour was. Changed to use basedir($file) always.
+ for my $sourced_file ($base_dir . '/' . $include, + $::glob_mysql_test_dir . '/' . $include)
Can you also look in the suite dir ? In the order of basedir(file), suite_dir, glob_mysql_test_dir.
Yes, done.
+ my ($sub_tags, $sub_master_opts, $sub_slave_opts)= + get_tags_from_file($sourced_file, $base_dir); + push @$tags, @$sub_tags; + # Tests (rpl.rpl_ddl at least) rely on options being in reverse order + # of include :-/ Hence the unshift(). + unshift @$master_opts, @$sub_master_opts; + unshift @$slave_opts, @$sub_slave_opts;
I think having options in the order of includes is more logical. Better to sort includes in the rpl_ddl.test, and use push here. It is more intuitive and less error-prone.
Agree. It turns out what is really needed is that options from includes are added before options from main file, so main file can override options from includes (this is what rpl_ddl needs, include/have_innodb adds --innodb, then rpl_ddl.test adds --skip-innodb for the slave only). So I changed it so that options come in order of includes, but first from included files, last from including file. More logical indeed. - Kristian.