Split Database tests in 'core' ones and 'driver specific' ones

+++ b/core/modules/pgsql/tests/src/Kernel/pgsql/ConnectionUnitTest.php
@@ -0,0 +1,25 @@
+use Drupal\Tests\mysql\Kernel\mysql\ConnectionUnitTest as MySqlConnectionUnitTest;

I do not think that extending a test from another database driver is going to work, because when we are running the testbot for PostgreSQL the mysql module will NOT be enabled.

I like the idea of making the base test class abstract and letting each driver override that abstract class in a way that is best for the driver. Or not override at all when they do not want to use the test. Good idea!

@mondrake: Thank you for working on this.

To me this a good solution for the by core supported database drivers.

I am not sure how it is for contrib database drivers. If I have some core test that will not pass for my contrib database driver and that test is not extending DriverSpecificDatabaseTestBase, then I will have a problem. I will need to create a patch that will change the test to one that is extending DriverSpecificDatabaseTestBase and add for each by core supported database driver a new test (with a comment that it is needed for a contrib database driver). It works, only the idea that I will need to get a patch to land in core, just so that I can override a core test is to me a bit hmmm. It is not how overriding core should be.

My preference would be to place a phpunit.xml.dist file in every database drivers module and let the testbot use that instead of the one in the core directory. Each phpunit.xml.dist file can then add element to skip the tests in the other 2 by core supported database driver modules and also skip any other test it need to and replace it by its own version.
Contrib database drivers can place phpunit.xml.dist in their module. They will need to exclude the 3 by core supported database driver modules and add any other core test they want to override. No need to patch core.
We can write a test that will keep the different versions of the phpunit.xml.dist files in sync (with the exception of the element). Just as the as the core/tests/Drupal/Tests/Composer/Plugin/Scaffold/Functional/ScaffoldTest.php does for the files core/assets/scaffold/files/default.settings.php and sites/default/default.settings.php.
To me, this is more the way the PHP community would expect it to be fixed. Feel free to disagree.

I took some time to think about your solution in this issue. The problem is that for my MongoDB database driver it is just not enough. Let me give some examples:

1. Each entity is stored in a single document, not as multiple row in different tables. Testing the table 'node_field_data' does not work
2. MongoDB does not do SQL and Views uses almost only SQL and is based on the SQL table structure, not MongoDB's document storage.
3. For MongoDB is an integer with the value 4 not the same as the string with the value '4'. I am not sure if we can update core test with (int) to make the test also pass for MongoDB?

For MongoDB is just allowing the test in Drupal\KernelTests\Core\Database just not enough. Maybe splitting the tests in 'core' tests and 'driver specific' ones works when it is part of a larger solution. Sorry.

I am afraid that when we do this your way (@mondrake), then it will work for all contrib databse driver with the exception of the one for MongoDB. And for MongoDB will it never be solved.

This patch has a different approach for overriding tests by the module that is proving the current database driver. The Drupal testbot uses the script core/scripts/run-tests.sh. That script uses the class core/lib/Drupal/Core/Test/TestDiscovery.php to create the list of tests that need to be run. I have made some modifications to that class to exclude the test from the database modules that not providing the current database driver. Also only the module that is providing the current database driver can override tests with the annotation "overrideTastClass".

The problem with the current patch is that the testbot is failing with;

--- Commands Executed ---
sudo -u www-data php /var/www/html/core/scripts/run-tests.sh --list > /var/lib/drupalci/workdir/run_tests.phpunit/testgroups.txt
Return Code: 2
--- Output ---

--- Errors ---

Should fix it for the testbot.

The method Drupal\KernelTests\KernelTestBase::setUp() now has code that skips tests that are from module with a database driver that is not from the active database connection and it skips tests that have been overridden by the module with the databse driver that is used in the active database connection. This solution is is a mix between the solution #1 and #3.

@mondrake: Thank you for reviewing my patch. Hopefully the new patch makes it more clear how it works.

I have removed the UnitTests and moved the code in KernelTestBase::setUp() to its own method.

For #20.1: Fixed.

For #20.2: I did my best to improve them.

For #20.3: Yes, this is something we need to support, only we need #3256642: Introduce database driver extensions and autoload database drivers' dependencies for that. My suggestion is to that in a followup.

The missing interdiff file.

@mondrake: Let go for your solution.

#3260007: Decouple Connection from the wrapped PDO connection to allow alternative clients has landed.

Before moving further, a conceptual thing I think we need to discuss - IMHO we should do this only for kernel tests, and only for tests of the @group Database.

As far as this issue is concerned, yes lets do that.

Some of the tests in that category are extending DataBaseTestBase and some are extending KernelTestBase. Can we make the base DriverSpecificKernelTestBase extend KernelTestBase and have DriverSpecificDatabaseTestBase extend DriverSpecificKernelTestBase. To do that we can/need to move the DatabaseTestBase table creations and the sample data to a trait. We can than removed the new class variable $installSchemaAndData.

If a non-db test (kernel or functional) needs to skip or switch depending on the db driver, that would smell like our DB API need to be fixed not being 'abstract' enough. In other words, if the test does that, probably it's the runtime code that behaves differently on the driver under the hood, and that means that core would potentially not be compatible with non-core drivers. In that case, I think we should strive to fix the API rather than giving the testing framework ways to workaround.

+1 for this solution. Not sure if this solution will work for every SQL-database driver. I also think that some of the Database API testing is done in tests that are outside of the directory core/tests/Drupal/KernelTests/Core/Database and/or @group Database, but that is another problem.

I have refactored the MR a bit to get rid of the variable $installSchemaAndData.

For the rest does it look great.

Re-rolled patch from #36 against current 9.5.x branch.

The reroll in comment #40 looks good to me.
Now we need a version for D10.0 and D10.1.
Back to needs work.

The version for D10.0 and D10.1 looks good to me.
Back to RTBC.

+++ b/core/tests/Drupal/KernelTests/Core/Database/DatabaseTestSchemaDataTrait.php
@@ -0,0 +1,121 @@
+trait DatabaseTestSchemaDataTrait {

+++ b/core/tests/Drupal/KernelTests/Core/Database/DatabaseTestSchemaInstallTrait.php
@@ -0,0 +1,32 @@
+trait DatabaseTestSchemaInstallTrait {

@mondrake: In your D10.0 patch you have created 2 traits called DatabaseTestSchemaDataTrait and DatabaseTestSchemaInstallTrait. The 9.5 version only has the trait InstallDatabaseTestSchemaAndDataTrait. Could you update either one, so that they both do the same thing. Or explain why you made them different.

@mondrake: Thank you for the explanation.

Back to RTBC.

For the comitter: The patch for D9.5 has the trait InstallDatabaseTestSchemaAndDataTrait and the patch for D10.X has the traits DatabaseTestSchemaDataTrait and DatabaseTestSchemaInstallTrait. I leave it up to you to determine if the difference is acceptable or that it must be the same for both patches.

Committed and pushed d755c76eb1 to 10.1.x and 8d8b24055f to 10.0.x. Thanks!

So we need to backport the changes talked about in #48/#49 to the 9.5.x version (might even got for 9.4.x given this is all test related).

Please make sure the patch passes on PHP 7.3 as I see some things in the 10.x patch that require a higher PHP version.

Testbot is not happy

fix cs error. Please review it

The patch for D9.5 looks good to me.

Needs to pass on PHP7.3

Fix
PHP Parse error: syntax error, unexpected 'Connection' (T_STRING), expecting function (T_FUNCTION) or const (T_CONST) in /var/www/html/core/modules/system/tests/src/Functional/Database/DatabaseTestBase.php on line 20. Needs Review

Checking core/modules/system/tests/src/Functional/Database/DatabaseTestBase.php

FILE: ...odules/system/tests/src/Functional/Database/DatabaseTestBase.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
5 | WARNING | [x] Unused use statement
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY.

Fix unused use statement.

Testbot is failing.

Hi @daffie @alexpott
i m new to contribution would you please guide how to solve this error
ERROR: PHP Fatal error: Trait "Drupal\KernelTests\Core\Database\DatabaseTestSchemaDataTrait" not found in /var/www/html/core/tests/Drupal/KernelTests/Core/Database/DatabaseTestBase.php on line 14

@pradhumanjainOSL: The patch for D10.0 is adding 2 traits and they are not in your patch for D9.5 (DatabaseTestSchemaDataTrait and DatabaseTestSchemaInstallTrait). Please look carefully how it is done in the patch for D10.0.

@daffie Thanks for your guidance.
But i fix the patch now please review and apologies.

The D9.5 looks good to me.
It passes the testbot for all by 3 by core supported databases.
It passes the testbot for PHP7.3 and PHP8.0.
For me it is RTBC.

Committed b855673 and pushed to 9.5.x. Thanks!