Code review comment for lp:~laurynas-biveinis/percona-server/ahi-partitions-5.6-5.6

More comments on test cases:

> === added file 'Percona-Server/mysql-test/suite/sys_vars/t/innodb_adaptive_hash_index_partitions_basic.test'
> --- Percona-Server/mysql-test/suite/sys_vars/t/innodb_adaptive_hash_index_partitions_basic.test 1970-01-01 00:00:00 +0000
> +++ Percona-Server/mysql-test/suite/sys_vars/t/innodb_adaptive_hash_index_partitions_basic.test 2013-08-26 13:16:30 +0000
> @@ -0,0 +1,32 @@
> +# A sys_vars suite test from innodb_adaptive_hash_index_partitions. Adapted from
> +# innodb_buffer_pool_instances_basic.test.
> +
> +--source include/have_innodb.inc
> +
> +SELECT COUNT(@@GLOBAL.innodb_adaptive_hash_index_partitions) AS should_be_1;
> +

I’m not sure what the above check is supposed to verify. Apparently that
the variable exists and is not NULL. But if that’s the case, the test
would fail anyway.

Just checking the default value would make sense. But it’s not checked
anywhere in the test.

> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SET @@GLOBAL.innodb_adaptive_hash_index_partitions=1;
> +
> +SELECT COUNT(@@GLOBAL.innodb_adaptive_hash_index_partitions) AS should_be_1;
> +

The same check again?

> +SELECT COUNT(VARIABLE_VALUE) AS should_be_1
> +FROM INFORMATION_SCHEMA.GLOBAL_VARIABLES
> +WHERE VARIABLE_NAME='innodb_adaptive_hash_index_partitions';
> +
> +SELECT @@GLOBAL.innodb_adaptive_hash_index_partitions = VARIABLE_VALUE AS should_be_1
> +FROM INFORMATION_SCHEMA.GLOBAL_VARIABLES
> +WHERE VARIABLE_NAME='innodb_adaptive_hash_index_partitions';
> +
> +SELECT @@innodb_adaptive_hash_index_partitions = @@GLOBAL.innodb_adaptive_hash_index_partitions AS should_be_1;
> +
> +SELECT COUNT(@@innodb_adaptive_hash_index_partitions) AS should_be_1;
> +
> +--Error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT COUNT(@@LOCAL.innodb_adaptive_hash_index_partitions);
> +
> +--Error ER_INCORRECT_GLOBAL_LOCAL_VAR
> +SELECT COUNT(@@SESSION.innodb_adaptive_hash_index_partitions);
> +
> +--Error ER_BAD_FIELD_ERROR
> +SELECT innodb_adaptive_hash_index_partitions = @@SESSION.innodb_adaptive_hash_index_partitions;

I don’t know the purpose of all these checks which are replicated from
one test case to another. It looks like some incompetent (to put it
mildly) person first wrote them, and then everyone else copies that
stuff whenever he needs to implement a variable test.

What I would expect to see in a sys_var.*_basic test is verifying
the variable type, and default/min/max values. Instead most tests do
some acrobatics with I_S and different syntax to access the variable.

Please also consider backporting it to 5.5 as I mentioned in the 5.5 MP.