Problem/Motivation

Follow-up to #3315604: Move remaining database specific kernel tests to the driver modules with additional opportunities identified while testing with a custom driver.

Points:

  • DriverSpecificSchemaTestBase::testSchema() has the following code
        // Use database specific data type and ensure that table is created.
        $table_specification = [
          'description' => 'Schema table description.',
          'fields' => [
            'timestamp'  => [
              'mysql_type' => 'timestamp',
              'pgsql_type' => 'timestamp',
              'sqlite_type' => 'datetime',
              'not null' => FALSE,
              'default' => NULL,
            ],
          ],
        ];
        try {
          $this->schema->createTable('test_timestamp', $table_specification);
        }
        catch (\Exception $e) {
        }
        $this->assertTrue($this->schema->tableExists('test_timestamp'), 'Table with database specific datatype was created.');
    

    that makes hard for contrib to test their own type --> extract in a separate abstract method that each diver implements

  • Drupal\KernelTests\Core\Cache\EndOfTransactionQueriesTest requires a driver specific \Drupal\database_statement_monitoring_test\{driver}\Connection class --> change the entire test class to an abstract base one extending DriverSpecificKernelTestBase, re-implement in modules
  • Drupal\KernelTests\Core\Database\TransactionTest::testTransactionWithDdlStatement
    Rolling back a transaction containing DDL should produce a warning, but not all drivers can handle that (i.e. mysqli, oracle). --> change the entire test class to an abstract base one extending DriverSpecificKernelTestBase, re-implement in modules
  • Drupal\KernelTests\Core\Database\BasicSyntaxTest::testAllowSquareBrackets in Oracle, this produces an error ORA-00904: "NAME": invalid identifier: select name from "test18855128test" where name = '[square]' --> extract the testAllowSquareBrackets() test to an abstract base one extending DriverSpecificKernelTestBase, implement in modules

Proposed resolution

See above.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#33 3316923-33.patch20.99 KBmondrake

Issue fork drupal-3316923

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

mondrake created an issue. See original summary.

mondrake’s picture

Issue summary: View changes
mondrake’s picture

Issue summary: View changes
mondrake’s picture

Issue summary: View changes
mondrake’s picture

Status: Active » Needs review

Also reviewed the logic in DriverSpecificKernelTestBase::setUp to skip tests that do not match the SUT's driver before database init, this allows to save some time.

mondrake’s picture

Assigned: Unassigned » mondrake
Status: Needs review » Needs work

Those failures need addressing.

mondrake’s picture

Assigned: mondrake » Unassigned
Status: Needs work » Needs review
daffie’s picture

Status: Needs review » Needs work

For the unresolved threads on the MR.

mondrake’s picture

Status: Needs work » Needs review

Thanks @daffie, points addressed.

daffie’s picture

Status: Needs review » Reviewed & tested by the community

The added comments look good to me.
All test changes look good to me.
The IS is very clear.
For me it is RTBC.

catch’s picture

Status: Reviewed & tested by the community » Needs review

Couple of questions on the MR.

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for reviews. Addressed @catch's comments. Resetting to RTBC since changes are doc only.

I suggest to address the move of the test statement monitoring classes in a follow-up. Maybe a better solution would be to work with the Log class instead, instead of overriding the Connection one. I think that deserves more discussion.

mondrake’s picture

Assigned: Unassigned » mondrake
Status: Reviewed & tested by the community » Needs work

Let's try switching to use the standard logger here directly.

mondrake’s picture

Status: Needs work » Needs review

Actually, getting rid of the database_statement_monitoring_test seems to simplify this test and make it more accurate since we can take log of the insert statements too, that the current solution can't since it only logs call to query.

mondrake’s picture

Status: Needs review » Needs work

Some more work for pgsql and sqlite needed.

mondrake’s picture

Assigned: mondrake » Unassigned
Status: Needs work » Needs review

Further optimized EndOfTransactionQueriesTest

daffie’s picture

Status: Needs review » Needs work
Issue tags: +Contrib database driver
daffie’s picture

Status: Needs work » Reviewed & tested by the community

All code changes look good to me.
Back to RTBC.

mondrake’s picture

mondrake’s picture

Assigned: Unassigned » mondrake
Status: Reviewed & tested by the community » Needs work

Back to NW to refactor EndOfTransactionQueriesTest

mondrake’s picture

Assigned: mondrake » Unassigned
Status: Needs work » Needs review

For review again.

daffie’s picture

Status: Needs review » Reviewed & tested by the community

As requested by @catch is the test EndOfTransactionQueriesTest no longer extending the class DriverSpecificKernelTestBase. It is back to extending the class KernelTestBase.
Back to RTBC.

catch’s picture

Couple more questions sorry:

None of the core drivers overrride TransactionTest, but the issue summary suggests that MySQL should (and that Oracle must). Is it just that MySQL doesn't need to override it after all (but that Oracle still does)? Or am I missing something?

SyntaxTest - I assume this is per-driver just for Oracle?

Nice to see the test module gone, that's a lot cleaner and glad I asked!

mondrake’s picture

None of the core drivers overrride TransactionTest, but the issue summary suggests that MySQL should (and that Oracle must). Is it just that MySQL doesn't need to override it after all (but that Oracle still does)? Or am I missing something?

Not MySql (the core driver based on PDO). A MySql driver based on the mysqli extension should, since mysqli does not fail when rolling back and no transaction active. See https://github.com/mondrake/mysqli/blob/main/tests/src/Kernel/mysqli/Tra... for an attempted mysqli driver.

SyntaxTest - I assume this is per-driver just for Oracle?

Exactly. See https://github.com/mondrake/drudbal/blob/master/tests/src/Kernel/dbal/Sy... for example.

  • catch committed cf3ae0f on 10.1.x
    Issue #3316923 by mondrake, daffie: Sort out more driver specific...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 10.1.x, thanks!

mondrake’s picture

Great, thanks. This could be backportable if we omit the deprecation of the test module.

mondrake’s picture

Fleshed and published the CR.

mondrake’s picture

Status: Fixed » Patch (to be ported)
mondrake’s picture

Version: 10.1.x-dev » 10.0.x-dev
mondrake’s picture

Status: Patch (to be ported) » Needs review
StatusFileSize
new20.99 KB

A 10.0.x and 9.5.x patch.

mondrake’s picture

So we cannot backport to 9.5 unless we backport the parent first. Not sure worth the effort.

daffie’s picture

Status: Needs review » Reviewed & tested by the community

The patch from comment #33 can be backported to 10.0.x.
Back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 92c09c8 and pushed to 10.0.x. Thanks!

  • alexpott committed 92c09c8 on 10.0.x
    Issue #3316923 by mondrake, daffie, catch: Sort out more driver specific...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.