Config tests failing when using MySQL
FMCorz opened this issue · 7 comments
As reported on the official Moodle Tracker (https://tracker.moodle.org/browse/MDL-74847):
In the latest version of block_xp (https://moodle.org/plugins/block_xp) there is a file named config_test.php which contains two tests that are failing:
test_table_row_config
test_table_row_config_with_nullThe issues are that the tables should not contain the prefix ( i.e phpunit_block_xp_config ) and that the database queries are using a different table structure, with columns ( char1, char2, text1, text2, int1, int2) that does not exist in the table block_xp_config.
The table used in the unit tests is not meant to reflect the structure of the table of the plugin itself, but rather to assert the valid operations done by the table_row_config implementation of the config interface. The use of the name phpunit_block_xp_config was the most accurate to represent the association between the interface and the unit tests.
The table name can be changed if it is causing unexpected issues, or test failures.
As of the latest releases, the config tests do not seem to be failing. If they are, we would need more information to address them.
Good day @FMCorz , may I ask, how do you run the PHPUnit tests for block_xp? I ask because you said "the config tests do not seem to be failing".
The way Moodle Core works (Core as in, without any additional code, a.k.a. Moodle Vanilla), is by using prefixes to manage tables. The three basic (default) prefixes are: mdl_, phpu_, and behat_. Thus, any table can exist with these three prefixes. For example the table block_xp_config, if I generate a Behat site, along with a PHPUnit site and the regular Moodle site, then three tables are generated: mdl_block_xp_config, phpu_block_xp_config, and behat_block_xp_config. Here are some screenshots showing this:
Moodle Vanilla uses these three default prefixes because that's the minimum amount of sites that need to be generated in order to have a Moodle testing platform. In theory each of the three would live in its own database, but in practice, the three 'databases' are merged into a single one, and they are differentiated from each other via prefixes.
The problem with using the 'phpunit' prefix, is that then wrongly named tables are generated. In the case of the test block_xp_config_testcase::test_table_row_config, by default this would generate a table called phpu_phpunit_block_xp_config which is a table that first of all doesn't exist, and second it is against Moodle's coding style, as can be reviewed here: https://docs.moodle.org/dev/PHPUnit#Initialisation_of_test_environment specifically where it shows the default prefix as $CFG->phpunit_prefix = 'phpu_';
Thank you so much for you attention, and sorry if this was a bit long, I decided to write at length given your addition of the label named 'additional information required'. Let me know if something of what I say is wrong, that's why I ask about how do you perform the PHPUnit tests using a Moodle site with the PHPUnit environment initialized.
Hi @Julian-Tovar,
Thank you for the thorough explanation.
I am quite familiar with the database structure and the use of prefixes. You are right that the phpunit prefix used in the unit tests does not follow any standard, and that final table name phpu_phpunit_block_xp_config is not formatted for production code.
However, the table name there is only used as a temporary table that is both created and dropped during each of the corresponding unit tests. This table will not persist after tests are run, nor should it. Creating a new table is required to assess the well functioning the table_row_config class which implements the config interface. The implementation is not directly related to the production block_xp_config table. Moreover, to accurately test the implementation of the table_row_config using various field types, defaults and null allowed configuration, a new table was the best course of action.
The name of the table is arbitrary, and I chose phpunit_block_xp_config because at the time it seemed like a good enough name for a table that could not conflict with an existing table, and somewhat accurately represented what it relates to. The name could be updated to anything so long as it does not conflict with any of the core tables. For instance, we could not use block_xp_config on its own as that table already exists when prefixed with phpu_.
If unit tests are not failing when you run them, then I think we can safely close this issue as the unit test does what it is meant to do, irrespective of the arbitrary table name. If the tests are failing, then please do share the traceback so that we can implement a fix.
I run unit tests with mdk phpunit -r -s block_xp, this is no different to running the tests by invoking phpunit directly.
Hi @FMCorz thank you so much for introducing me to mdk I will try it out, since the two tests fail when using, from the Moodle's root vendor/bin/phpunit path/to/test, so I'll check what does mdk do different to not fail.
Hi @FMCorz
So here is a more detailed report of the failures.
Setting up the PHPUnit environment
Using a terminal in the Moodle's root directory, execute:
php admin/tool/phpunit/cli/init.php
Failure in the block_xp_config_testcase::test_table_row_config test
Run vendor/bin/phpunit --filter "block_xp_config_testcase::test_table_row_config$" --test-suffix config_test.php /vagrant/www/core/blocks/xp/tests
After running this command, this is the generated output:
vendor/bin/phpunit --filter "block_xp_config_testcase::test_table_row_config$" --test-suffix config_test.php /vagrant/www/core/blocks/xp/tests
Moodle 4.1dev (Build: 20220519), 6c114e2a80add1f1bd4374d8e613d38cfc2b508b
Php: 7.4.11, mysqli: 5.7.32-0ubuntu0.18.04.1, OS: Linux 4.15.0-122-generic x86_64
PHPUnit 9.5.13 by Sebastian Bergmann and contributors.
Warning: Test case class not matching filename is deprecated
in /vagrant/www/core/blocks/xp/tests/config_test.php
Class name was 'block_xp_config_testcase', expected 'config_test'
Warning: Test case class not matching filename is deprecated
in /vagrant/www/core/blocks/xp/tests/course_world_config_test.php
Class name was 'block_xp_course_world_config_testcase', expected 'course_world_config_test'
E 1 / 1 (100%)
Time: 00:00.338, Memory: 64.50 MB
There was 1 error:
1) block_xp_config_testcase::test_table_row_config
dml_write_exception: Error writing to database (You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'int1,int2) VALUES('1','default_char1','default_char2','default_text1','default_t' at line 1
INSERT INTO phpu_phpunit_block_xp_config (courseid,char1,char2,text1,text2,int1,int2) VALUES(?,?,?,?,?,?,?)
[array (
0 => 1,
1 => 'default_char1',
2 => 'default_char2',
3 => 'default_text1',
4 => 'default_text2',
5 => 333,
6 => 222,
)])
/vagrant/www/core/lib/dml/moodle_database.php:489
/vagrant/www/core/lib/dml/mysqli_native_moodle_database.php:1357
/vagrant/www/core/lib/dml/mysqli_native_moodle_database.php:1403
/vagrant/www/core/blocks/xp/classes/local/config/table_row_config.php:168
/vagrant/www/core/blocks/xp/classes/local/config/table_row_config.php:182
/vagrant/www/core/blocks/xp/tests/config_test.php:244
/vagrant/www/core/lib/phpunit/classes/advanced_testcase.php:80
phpvfscomposer:///vagrant/www/core/vendor/phpunit/phpunit/phpunit:97
ERRORS!
Tests: 1, Assertions: 9, Errors: 1.
Brief analysis of the error: A dml_write_exception is thrown. This one: "dml_write_exception: Error writing to database (You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'int1,int2) VALUES('1','default_char1','default_char2','default_text1','default_t' at line 1"
This is because, in a MySQL database, it is not allowed to use the keyword int as one of the column names. I would think that int1 and int2 would not fall into this category, since the addition of the number at the end should be enough to make them different from int. I think this might be because we use MySQL 5.7.32, because we haven't been able to upgrade to MySQL 8, but I can't test this on MySQL 8 so I'm not sure.
Failure in the block_xp_config_testcase::test_table_row_config_with_null test
Run vendor/bin/phpunit --filter "block_xp_config_testcase::test_table_row_config_with_null" --test-suffix config_test.php /vagrant/www/core/blocks/xp/tests
After running this command, this is the generated output:
vendor/bin/phpunit --filter "block_xp_config_testcase::test_table_row_config_with_null" --test-suffix config_test.php /vagrant/www/core/blocks/xp/tests
Moodle 4.1dev (Build: 20220519), 6c114e2a80add1f1bd4374d8e613d38cfc2b508b
Php: 7.4.11, mysqli: 5.7.32-0ubuntu0.18.04.1, OS: Linux 4.15.0-122-generic x86_64
PHPUnit 9.5.13 by Sebastian Bergmann and contributors.
Warning: Test case class not matching filename is deprecated
in /vagrant/www/core/blocks/xp/tests/config_test.php
Class name was 'block_xp_config_testcase', expected 'config_test'
Warning: Test case class not matching filename is deprecated
in /vagrant/www/core/blocks/xp/tests/course_world_config_test.php
Class name was 'block_xp_course_world_config_testcase', expected 'course_world_config_test'
E 1 / 1 (100%)
Time: 00:00.423, Memory: 62.50 MB
There was 1 error:
1) block_xp_config_testcase::test_table_row_config_with_null
dml_write_exception: Error writing to database (You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'int1,char1,text1) VALUES('1','11','hh','ww')' at line 1
INSERT INTO phpu_phpunit_block_xp_config (courseid,int1,char1,text1) VALUES(?,?,?,?)
[array (
0 => 1,
1 => 11,
2 => 'hh',
3 => 'ww',
)])
/vagrant/www/core/lib/dml/moodle_database.php:489
/vagrant/www/core/lib/dml/mysqli_native_moodle_database.php:1357
/vagrant/www/core/lib/dml/mysqli_native_moodle_database.php:1403
/vagrant/www/core/blocks/xp/tests/config_test.php:295
/vagrant/www/core/lib/phpunit/classes/advanced_testcase.php:80
phpvfscomposer:///vagrant/www/core/vendor/phpunit/phpunit/phpunit:97
ERRORS!
Tests: 1, Assertions: 0, Errors: 1.
Brief analysis of the error: As you can see, this is the same error as before. So, after all, maybe the table name is not problematic at all, and it's just a stylistic issue. But I'm not completely sure of this yet, because the table doesn't get to be used at this point in the test, because the test fails before that, due to the syntax error.
Hi @Julian-Tovar,
We should have ran the tests on MySQL for sure, as you identified the int1 is a reserved keyword in MySQL (who would have thought?), as well as other int* variants. We'll address that. (Ref: https://dev.mysql.com/doc/refman/8.0/en/keywords.html)
We will leave the warning as is for now, and address it later.
Thanks again!
Thank you so much, and sorry for the confusion about the table name, I think it is a valid name, although I have never seen one named like that, but then again I'm not too experienced with table names in PHPUnit tests in Moodle plugins, so for all I know, it's valid.