Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: [MariaDB] phpunit test errors #7929

Open
neznaika0 opened this issue Sep 12, 2023 · 10 comments
Open

Bug: [MariaDB] phpunit test errors #7929

neznaika0 opened this issue Sep 12, 2023 · 10 comments
Labels
bug Verified issues on the current code behavior or pull requests that will fix them

Comments

@neznaika0
Copy link
Contributor

neznaika0 commented Sep 12, 2023

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:

if (version_compare($this->db->getVersion(), '8.0.17', '>=')) {
// As of MySQL 8.0.17, the display width attribute for integer data types
// is deprecated and is not reported back anymore.
// @see https://dev.mysql.com/doc/refman/8.0/en/numeric-type-attributes.html
$expected[0]['max_length'] = null;
$expected[3]['max_length'] = null;
}

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
  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','[email protected]','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','[email protected]','Ahmadinejad'), ('El Salvador','[email protected]','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
@neznaika0 neznaika0 added the bug Verified issues on the current code behavior or pull requests that will fix them label Sep 12, 2023
@kenjis
Copy link
Member

kenjis commented Sep 12, 2023

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
Copy link
Member

kenjis commented Sep 12, 2023

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
Copy link
Contributor Author

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
Copy link
Member

kenjis commented Sep 12, 2023

You cannot change any config.

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

@neznaika0
Copy link
Contributor Author

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
Copy link
Member

kenjis commented Sep 12, 2023

See the comment:

'DBPrefix' => 'db_', // Needed to ensure we're working correctly with prefixes live. DO NOT REMOVE FOR CI DEVS

@neznaika0
Copy link
Contributor Author

neznaika0 commented Sep 12, 2023

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

@kenjis
Copy link
Member

kenjis commented Sep 13, 2023

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.

@kenjis kenjis changed the title Bug: phpunit test errors Bug: [MariaDB] phpunit test errors Sep 23, 2023
@neznaika0
Copy link
Contributor Author

PR released, closed?

@kenjis
Copy link
Member

kenjis commented Jun 26, 2024

Error №2 is not fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

No branches or pull requests

2 participants