maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Sergey!

> === added file 'mysql-test/include/have_daemon_example_plugin.inc'
> --- a/mysql-test/include/have_daemon_example_plugin.inc	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/include/have_daemon_example_plugin.inc	2014-04-14 09:20:24 +0000
> @@ -0,0 +1,16 @@
> +disable_query_log;
> +#
> +# Check if server has support for loading plugins
> +#
> +if (`SELECT @@have_dynamic_loading != 'YES'`) {
> +  --skip daemon example plugin requires dynamic loading
> +}
> +
> +#
> +# Check if the variable DAEMONEXAMPLE is set
> +#
> +if (!$LIBDAEMON_EXAMPLE_SO) {
> +  --skip daemon_example plugin requires the environment variable \$DAEMONEXAMPLE to be set (normally done by mtr)

Eh? It was in your first patch. I thought it's an additional patch,
but it looks like it includes the first one?

Okay, I will copy all my comments from the first patch here
to have a complete review in one email. So, here I said

************************
it's always done by mtr, a better error message should be "No libdaemon_example.so was found"
or simply "Need daemon_example plugin"

and, btw, the first if() is redundant, if no dynamic loading was enabled no
dynamic plugins will be built whatsoever.

> +}
> +
> +enable_query_log;
> 
> === added file 'mysql-test/include/ib_logfile_size_check.inc'
> --- a/mysql-test/include/ib_logfile_size_check.inc	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/include/ib_logfile_size_check.inc	2014-04-14 09:20:24 +0000
> @@ -0,0 +1,14 @@

I'm not a great fan of obscure include files that are only used once in one
single test. Perhaps it's better to inline it?

> +perl;
> + my $dir = $ENV{'MYSQLD_DATADIR'};
> + my $size;
> + opendir(DIR, $dir) or die $!;
> + while (my $file = readdir(DIR)) 
> + {
> + 
> +   next unless ($file =~ m/\ib_logfile.$/);
> +   $size = -s "$dir/$file";
> +   print "The size of the ib_logfile(0/1): $size \n"; 
> + }
> + close(DIR);
> + exit(0)
> +EOF
> 
> === added file 'mysql-test/include/vardir_size_check.inc'
> --- a/mysql-test/include/vardir_size_check.inc	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/include/vardir_size_check.inc	2014-04-14 09:20:24 +0000
> @@ -0,0 +1,26 @@

Same here. Although it's worse than with the previous file :)

ib_logfile_size_check.inc is only used in
innodb_log_file_size_functionality.test. This file - too.

But in 5.6 this one is also used in query_cache_type_functionality,
table_open_cache_functionality, and table_definition_cache_functionality,
sort_buffer_size_functionality, query_cache_size_functionality.

I suppose somebody just copied innodb_log_file_size_functionality over
and edited it. I don't see why would anyone really want to check the
vardir size in all these other tests.

So, yes, better to inline this file too, to avoid silly mistakes like that.

> +--echo Check the size of the vardir
> +--echo The output size is in unit blocks
> +perl;
> +#!/usr/bin/perl
> +use warnings;
> +use strict;
> +use File::Find;
> +my $dir = $ENV{'MYSQLTEST_VARDIR'};
> +my $size = 0;
> +find( sub { $size += -f $_ ? -s _ : 0 }, $dir );
> +
> +if ( $size < 1717987000 )
> +        { print "TRUE", "\n";}
> +else   
> +        { print "FALSE $size", "\n";}
> +
> +
> +## Expected size for the VAR dir being changed for the PB2 runs
> +
> +##if ( $size =~ /^496[\d]{5}$/ )
> +##	{  
> +##               { print "TRUE", "\n";}
> +##        }
> +##else
> +##	{ print "FALSE $size", "\n";}
> +EOF
> 
> === modified file 'mysql-test/mysql-test-run.pl'
> --- a/mysql-test/mysql-test-run.pl	2014-02-26 14:28:07 +0000
> +++ b/mysql-test/mysql-test-run.pl	2014-04-14 09:20:24 +0000
> @@ -5350,6 +5350,14 @@ sub mysqld_arguments ($$$) {
>      mtr_add_arg($args, "--gdb");
>    }
>  
> +  # Enable the debug sync facility, set default wait timeout.
> +  # Facility stays disabled if timeout value is zero.
> +  mtr_add_arg($args, "--loose-debug-sync-timeout=%s",
> +              $opt_debug_sync_timeout) unless $opt_user_args;

Why was it moved?

> +
> +  # Options specified in .opt files should be added last so they can
> +  # override defaults above.
> +
>    my $found_skip_core= 0;
>    foreach my $arg ( @$extra_opts )
>    {
> === added file 'mysql-test/suite/archive/archive_no_symlink-master.opt'
> --- a/mysql-test/suite/archive/archive_no_symlink-master.opt	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/archive/archive_no_symlink-master.opt	2014-04-14 09:20:24 +0000
> @@ -0,0 +1 @@
> +--skip-symbolic-links

just fyi: in MariaDB the file name can simply be archive_no_symlink.opt
-master and -slave are only needed for replication tests to have different
options for the master and the slave.

> 
> === added file 'mysql-test/t/bug12969156.test'
> --- a/mysql-test/t/bug12969156.test	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/t/bug12969156.test	2014-04-14 09:20:24 +0000
> @@ -0,0 +1,18 @@
> +--source include/have_daemon_example_plugin.inc
> +--source include/not_embedded.inc
> +# TODO: the windows emulation of pthreads doesn't behave predictably
> +--source include/not_windows.inc
> +
> +--echo #
> +--echo # Bug #12969156 : SEGMENTATION FAULT ON UNINSTALLING
> +--echo #  DAEMON_EXAMPLE PLUGIN
> +--echo #
> +
> +let $counter= 0;
> +while ($counter < 10)
> +{
> +--replace_result $LIBDAEMON_EXAMPLE_SO DAEMONEXAMPLE
> +  eval INSTALL PLUGIN daemon_example SONAME '$LIBDAEMON_EXAMPLE_SO';

in our own tests (not copied from 5.6) better use

  install soname 'libdaemon_example';

it's shorter, doesn't need --eval and doesn't need --replace_result

> +  UNINSTALL PLUGIN daemon_example;
> +  inc $counter;
> +}
> 
> === added file 'mysql-test/t/innodb_log_file_size_functionality.test'
> --- a/mysql-test/t/innodb_log_file_size_functionality.test	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/t/innodb_log_file_size_functionality.test	2014-04-14 09:20:24 +0000
> @@ -0,0 +1,124 @@

This should be renamed to sys_vars.innodb_log_file_size_func test

> +###############################################################################
> +#                                                                             #
> +# Variable Name: innodb_log_file_size                                         #
> +# Scope: Global                                                               #
> +# Access Type: Static                                                         #
> +# Data Type: numeric                                                          #
> +#                                                                             #
> +#                                                                             #
> +# Creation Date: 2012-08-20                                                   #
> +# Author : Tanjot Singh Uppal                                                 #
> +#                                                                             #
> +#                                                                             #
> +# Description:Test Cases of Static System Variable innodb_log_file_size       #
> +#             that checks the behavior of this variable in the following ways #
> +#              * Value Check                                                  #
> +#              * Scope Check                                                  #
> +#              * Functionality Check                                          #
> +#              * Accessability Check                                          #
> +#                                                                             #               
> +# This test does not perform the crash recovery on this variable              # 
> +# For crash recovery test on default change please run the ibtest             #
> +###############################################################################

Regards,
Sergei