codeigniter4 / CodeIgniter4

Open Source PHP Framework (originally from EllisLab)
https://codeigniter.com/
MIT License
5.38k stars 1.9k forks source link

Bug: [MariaDB] `phpunit` test errors #7929

Open neznaika0 opened 1 year ago

neznaika0 commented 1 year ago

PHP Version

8.2

CodeIgniter4 Version

4.4.1

CodeIgniter4 Installation Method

Git

Which operating systems have you tested for this bug?

Linux

Which server did you use?

cli-server (PHP built-in webserver)

Database

10.11.4-MariaDB-1

What happened?

:~/www/codeigniter$ vendor/bin/phpunit
There were 2 failures:

Error №1 (DB is configured in .env )

1) CodeIgniter\Database\Live\ForgeTest::testAddFields
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
     0 => Array &1 (
         'name' => 'id'
         'type' => 'int'
-        'max_length' => null
+        'max_length' => 11
         'nullable' => false
         'default' => null
         'primary_key' => 1
@@ @@
     3 => Array &4 (
         'name' => 'active'
         'type' => 'int'
-        'max_length' => null
+        'max_length' => 11
         'nullable' => false
         'default' => '0'
         'primary_key' => 0
     )
 )

/www/codeigniter/tests/system/Database/Live/ForgeTest.php:1087

Now mysql (8.1.0) and mariadb (10.11.4-MariaDB-1 ) are compatible databases, but they have different versions.

Connection return version as 10.11.4-MariaDB-1: https://github.com/codeigniter4/CodeIgniter4/blob/f35f956d083eb3872b52af0f578a1a14ba6a471f/tests/system/Database/Live/ForgeTest.php#L944-L950

Maybe you need to rewrite to $this->mysqli->server_version (101104)? This is BC. Or rewrite the test for different versions of mysql and mariadb. I commented out the condition and the test is successful.

Error №2 (DB is configured in .env )

2) CodeIgniter\Database\Live\GetVersionTest::testGetVersion
Failed asserting that '10.11.4-MariaDB-1' matches PCRE pattern "/\A\d+(\.\d+)*\z/".

/www/codeigniter/tests/system/Database/Live/GetVersionTest.php:32

The problem is in the versions for mariadb. Related to the first error.

Error №3 (DB is not configured in .env)

The third failure relates to the prefix: Some tests are built on the db_ prefix, others correctly add $prefix. $tableName

Tests are performed only when ENV: database.tests.DBPrefix = db_

It is necessary to replace the hard-coded table names.

phpunit log ```console There were 3 errors: 1) CodeIgniter\Database\Live\MySQLi\NumberNativeTest::testQueryDataAfterEnableNumberNative CodeIgniter\Database\Exceptions\DatabaseException: Table 'ci4_test.db_type_test' doesn't exist /www/codeigniter/system/Database/BaseConnection.php:647 /www/codeigniter/system/Database/BaseBuilder.php:1615 /www/codeigniter/tests/system/Database/Live/MySQLi/NumberNativeTest.php:78 Caused by CodeIgniter\Database\Exceptions\DatabaseException: Table 'ci4_test.db_type_test' doesn't exist /www/codeigniter/system/Database/MySQLi/Connection.php:311 /www/codeigniter/system/Database/BaseConnection.php:693 /www/codeigniter/system/Database/BaseConnection.php:607 /www/codeigniter/system/Database/BaseBuilder.php:1615 /www/codeigniter/tests/system/Database/Live/MySQLi/NumberNativeTest.php:78 Caused by mysqli_sql_exception: Table 'ci4_test.db_type_test' doesn't exist /www/codeigniter/system/Database/MySQLi/Connection.php:306 /www/codeigniter/system/Database/BaseConnection.php:693 /www/codeigniter/system/Database/BaseConnection.php:607 /www/codeigniter/system/Database/BaseBuilder.php:1615 /www/codeigniter/tests/system/Database/Live/MySQLi/NumberNativeTest.php:78 2) CodeIgniter\Database\Live\MySQLi\NumberNativeTest::testQueryDataAfterDisableNumberNative CodeIgniter\Database\Exceptions\DatabaseException: Table 'ci4_test.db_type_test' doesn't exist /www/codeigniter/system/Database/BaseConnection.php:647 /www/codeigniter/system/Database/BaseBuilder.php:1615 /www/codeigniter/tests/system/Database/Live/MySQLi/NumberNativeTest.php:96 Caused by CodeIgniter\Database\Exceptions\DatabaseException: Table 'ci4_test.db_type_test' doesn't exist /www/codeigniter/system/Database/MySQLi/Connection.php:311 /www/codeigniter/system/Database/BaseConnection.php:693 /www/codeigniter/system/Database/BaseConnection.php:607 /www/codeigniter/system/Database/BaseBuilder.php:1615 /www/codeigniter/tests/system/Database/Live/MySQLi/NumberNativeTest.php:96 Caused by mysqli_sql_exception: Table 'ci4_test.db_type_test' doesn't exist /www/codeigniter/system/Database/MySQLi/Connection.php:306 /www/codeigniter/system/Database/BaseConnection.php:693 /www/codeigniter/system/Database/BaseConnection.php:607 /www/codeigniter/system/Database/BaseBuilder.php:1615 /www/codeigniter/tests/system/Database/Live/MySQLi/NumberNativeTest.php:96 3) CodeIgniter\Database\Live\UpdateTest::testUpdateBatchUpdateFieldsAndAlias CodeIgniter\Database\Exceptions\DatabaseException: Unknown column 'db_user.country' in 'on clause' /www/codeigniter/system/Database/BaseConnection.php:647 /www/codeigniter/system/Database/BaseBuilder.php:1800 /www/codeigniter/system/Database/BaseBuilder.php:2561 /www/codeigniter/tests/system/Database/Live/UpdateTest.php:405 Caused by CodeIgniter\Database\Exceptions\DatabaseException: Unknown column 'db_user.country' in 'on clause' /www/codeigniter/system/Database/MySQLi/Connection.php:311 /www/codeigniter/system/Database/BaseConnection.php:693 /www/codeigniter/system/Database/BaseConnection.php:607 /www/codeigniter/system/Database/BaseBuilder.php:1800 /www/codeigniter/system/Database/BaseBuilder.php:2561 /www/codeigniter/tests/system/Database/Live/UpdateTest.php:405 Caused by mysqli_sql_exception: Unknown column 'db_user.country' in 'on clause' /www/codeigniter/system/Database/MySQLi/Connection.php:306 /www/codeigniter/system/Database/BaseConnection.php:693 /www/codeigniter/system/Database/BaseConnection.php:607 /www/codeigniter/system/Database/BaseBuilder.php:1800 /www/codeigniter/system/Database/BaseBuilder.php:2561 /www/codeigniter/tests/system/Database/Live/UpdateTest.php:405 -- There were 7 failures: 1) CodeIgniter\Database\Live\ForgeTest::testCreateTableWithExists Failed asserting that false is true. /www/codeigniter/tests/system/Database/Live/ForgeTest.php:176 2) CodeIgniter\Database\Live\ForgeTest::testAddFields Failed asserting that two arrays are identical. --- Expected +++ Actual @@ @@ 0 => Array &1 ( 'name' => 'id' 'type' => 'int' - 'max_length' => null + 'max_length' => 11 'nullable' => false 'default' => null 'primary_key' => 1 @@ @@ 3 => Array &4 ( 'name' => 'active' 'type' => 'int' - 'max_length' => null + 'max_length' => 11 'nullable' => false 'default' => '0' 'primary_key' => 0 ) ) /www/codeigniter/tests/system/Database/Live/ForgeTest.php:1087 3) CodeIgniter\Database\Live\GetVersionTest::testGetVersion Failed asserting that '10.11.4-MariaDB-1' matches PCRE pattern "/\A\d+(\.\d+)*\z/". /www/codeigniter/tests/system/Database/Live/GetVersionTest.php:32 4) CodeIgniter\Database\Live\MetadataTest::testListTablesUnconstrainedByPrefixReturnsAllTables Failed asserting that two arrays are identical. --- Expected +++ Actual @@ @@ 5 => 'misc' 6 => 'secondary' 7 => 'stringifypkey' - 8 => 'type_test' - 9 => 'user' - 10 => 'without_auto_increment' - 11 => 'tmp_widgets' + 8 => 'test_exists' + 9 => 'tmp_widgets' + 10 => 'type_test' + 11 => 'user' + 12 => 'without_auto_increment' ) /www/codeigniter/tests/system/Database/Live/MetadataTest.php:102 5) CodeIgniter\Database\Live\MetadataTest::testListTablesConstrainedByPrefixReturnsOnlyTablesWithMatchingPrefix Failed asserting that two arrays are identical. --- Expected +++ Actual @@ @@ 5 => 'misc' 6 => 'secondary' 7 => 'stringifypkey' - 8 => 'type_test' - 9 => 'user' - 10 => 'without_auto_increment' + 8 => 'test_exists' + 9 => 'tmp_widgets' + 10 => 'type_test' + 11 => 'user' + 12 => 'without_auto_increment' ) /www/codeigniter/tests/system/Database/Live/MetadataTest.php:118 6) CodeIgniter\Database\Live\UpsertTest::testGetCompiledUpsert Failed asserting that two strings are identical. --- Expected +++ Actual @@ @@ -'INSERT INTO `db_user` (`country`, `email`, `name`) +'INSERT INTO `user` (`country`, `email`, `name`) VALUES ('Iran','ahmadinejad@world.com','Ahmadinejad') ON DUPLICATE KEY UPDATE -`db_user`.`country` = VALUES(`country`), -`db_user`.`email` = VALUES(`email`), -`db_user`.`name` = VALUES(`name`)' +`user`.`country` = VALUES(`country`), +`user`.`email` = VALUES(`email`), +`user`.`name` = VALUES(`name`)' /www/codeigniter/tests/system/Database/Live/UpsertTest.php:244 7) CodeIgniter\Database\Live\UpsertTest::testGetCompiledUpsertBatch Failed asserting that 'INSERT INTO `user` (`country`, `email`, `name`)\n VALUES ('Iran','ahmadinejad@example.com','Ahmadinejad'), ('El Salvador','pedro@example.com','Pedro')\n ON DUPLICATE KEY UPDATE\n `user`.`country` = VALUES(`country`),\n `user`.`email` = VALUES(`email`),\n `user`.`name` = VALUES(`name`)' contains "INSERT INTO `db_user` (`country`, `email`, `name`)". /www/codeigniter/tests/system/Database/Live/UpsertTest.php:282 ```
kenjis commented 1 year ago

Please create one issue for one issue. Long description is difficult to understand.

About 3.

The third failure relates to the prefix: Some tests are built on the db prefix, others correctly add $prefix. $tableName Tests are performed only when ENV: database.tests.DBPrefix = db It is necessary to replace the hard-coded table names.

We expect the tests for CI4 are executed with the clean state of the repository. You cannot change any config. So the hard coded db_ is not a problem.

Of course, it is acceptable to change a hard-coded value to a variable.

kenjis commented 1 year ago

Maybe you need to rewrite to $this->mysqli->server_version (101104)? This is BC.

It breaks apps for MySQL. So not acceptable.

Or rewrite the test for different versions of mysql and mariadb.

Yes, this is the way to go. Change the expectations for MySQL or MariaDB.

neznaika0 commented 1 year ago

We expect the tests for CI4 are executed with the clean state of the repository. You cannot change any config. So the hard coded db_ is not a problem.

Hm. Good. Then how do I check the tests for mysqli? When is .env empty? Tests are skipped. If I set it up .env tests failed. Because they are created incorrectly containing the prefix.

Please create one issue for one issue.

They all depend on one fix. I don't think it's necessary - the big log will be fixed. I visually separated the errors

kenjis commented 1 year ago

You cannot change any config.

This was an exaggeration. However, DBPrefix cannot be changed.

neznaika0 commented 1 year ago

That's when you need to decide in the tests: "always db_ " or "always empty/configured prefix"

That's all you need to work. There will be time I will open a PR or someone else.

kenjis commented 1 year ago

See the comment: https://github.com/codeigniter4/CodeIgniter4/blob/f35f956d083eb3872b52af0f578a1a14ba6a471f/app/Config/Database.php#L59

neznaika0 commented 1 year ago

Oh, yes, I see it now. What to do with the hardcode? If you keep it, then the issue can be closed

kenjis commented 1 year ago

The db_ is the default configuration for CI4 testing. So if you change the Config value, tests may fail. It is no problem.

And some, or very few? tests should ensure that the result contains db_. If it is changed to a comparison to the Config DBPrefix value, the test becomes meaningless. For example, if you set DBPrefix to '', the test cannot make sure the result SQL contain the correct DBprefix. Even if you remove the logic to add DBPrefix, the test would pass.

I think it is fine to compare many tests against the Config value rather than against hard-coded db_ value. However, reviewing changes in tests can be quite difficult.

In conclusion, I don't think it is worth doing much.

neznaika0 commented 4 months ago

PR released, closed?

kenjis commented 4 months ago

Error №2 is not fixed.