Problem/Motivation

If a database driver (contrib) is not implementing a PDO connection as Connection::$connection, the ConnectionUnitTest::testConnectionOpen test fails because it expects that the connction is PDO and PDO methods are available.

Incurred into this while experimenting a db driver https://github.com/mondrake/drudbal in conjuction with #2605284: Testing framework does not work with contributed database drivers.

Proposed resolution

Just mark the test skipped in that case - implementing driver should take care of an alternative test implementation.

Remaining tasks

Review patch.

User interface changes

None

API changes

None

Data model changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mondrake created an issue. See original summary.

mondrake’s picture

mondrake’s picture

Status: Active » Needs review
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This seems reasonable for me

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 2867700-2.patch, failed testing.

mondrake’s picture

Status: Needs work » Reviewed & tested by the community
cilefen’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/tests/Drupal/KernelTests/Core/Database/ConnectionUnitTest.php
@@ -221,6 +221,11 @@ public function testConnectionOpen() {
+      $this->markTestSkipped('The database driver does not implement a PDO connection.');

Other skipped test exception messages explain a little more about what should be done instead. Like "PhantomJS is either not installed or not running. Start it via phantomjs...". Could we add to the message something from the issue summary, which would be something like "The database driver does not implement a PDO connection. The driver should provide an alternative test implementation."?

cilefen’s picture

Issue tags: +Novice

A novice can complete #7.

mradcliffe’s picture

Issue summary: View changes
Issue tags: +Baltimore2017

Tagging for Baltimore2017.

erozqba’s picture

Status: Needs work » Needs review
FileSize
1.13 KB
1.72 KB

Apply change proposed in #7.

mradcliffe’s picture

Status: Needs review » Needs work

Thank you for posting both a patch and an interdiff!

+++ b/core/tests/Drupal/KernelTests/Core/Database/ConnectionUnitTest.php
@@ -233,6 +233,11 @@ public function testConnectionOpen() {
+    // In case a database driver is implementing a non-PDO connection, skip this test
+    // the driver should provide an alternative test.

Comment text should not exceed 80 characters per line as seen on the API Documentation and Comment Standards page.

T

dawehner’s picture

The new error message doesn't makes that much sense ... given this issue is all about enabling people to build db drivers without PDO.

erozqba’s picture

Status: Needs work » Needs review
FileSize
1.23 KB
1.12 KB

Apply change proposed in comment #11

mradcliffe’s picture

  1. +++ b/core/tests/Drupal/KernelTests/Core/Database/ConnectionUnitTest.php
    @@ -233,6 +233,11 @@ public function testConnectionOpen() {
    +    // In case a database driver is implementing a non-PDO connection.
    +    // Skip this test, the driver should provide an alternative test.
    

    Sorry, more bike shed on comments.

    "Skip this test when a database driver does not
    implement PDO connection. An alternative database driver that does not implement PDO should implement its own connection test."

  2. +++ b/core/tests/Drupal/KernelTests/Core/Database/ConnectionUnitTest.php
    @@ -233,6 +233,11 @@ public function testConnectionOpen() {
    +    if (get_class($connection_property->getValue($connection)) != 'PDO') {
    

    Probably needs to be !==, right?

  3. +++ b/core/tests/Drupal/KernelTests/Core/Database/ConnectionUnitTest.php
    @@ -233,6 +233,11 @@ public function testConnectionOpen() {
    +      $this->markTestSkipped('The database driver does not implement a PDO connection, it should provide an alternative test implementation.');
    

    Taking a shot at rewriting the error message per @dawehner in #12. I'm not sure if putting comma helps here.

    "Ignored PDO connection unit test for this driver because it does not implement PDO."

erozqba’s picture

FileSize
1.14 KB
1.31 KB
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

That is indeed a better message now.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 725356f to 8.4.x and 1b34c76 to 8.3.x. Thanks!

I credited both @cilefen and @mradcliffe for providing reviews that affected the final patch. Thanks @dawehner for reviewing the issue too. And thanks to @mondrake and @erozqba for providing the patch.

Backported to 8.3.x because this is a test only change that makes things easier for people implementing alternate db drivers.

  • alexpott committed 725356f on 8.4.x
    Issue #2867700 by erozqba, mondrake, mradcliffe, cilefen:...

  • alexpott committed 1b34c76 on 8.3.x
    Issue #2867700 by erozqba, mondrake, mradcliffe, cilefen:...
mondrake’s picture

Thanks all!

With regards to

makes things easier for people implementing alternate db drivers

there are a few more issues that would need some love in this sense, if anybody feels like giving a hand :)

See #2605284: Testing framework does not work with contributed database drivers and related issues.

mondrake’s picture

Status: Fixed » Needs work

Actually, please revert this - in the process of creating new versions of the patch, the $this->markTestSkipped moved down in the method and now it's too late to avoid the error:

  /**
   * Tests pdo options override.
   */
  public function testConnectionOpen() {
    $connection = Database::getConnection('default');
    $reflection = new \ReflectionObject($connection);
    $connection_property = $reflection->getProperty('connection');
    $connection_property->setAccessible(TRUE);
    $error_mode = $connection_property->getValue($connection)
      ->getAttribute(\PDO::ATTR_ERRMODE);
    $this->assertEqual($error_mode, \PDO::ERRMODE_EXCEPTION, 'Ensure the default error mode is set to exception.');

    $connection = Database::getConnectionInfo('default');
    $connection['default']['pdo'][\PDO::ATTR_ERRMODE] = \PDO::ERRMODE_SILENT;
    Database::addConnectionInfo('test', 'default', $connection['default']);
    $connection = Database::getConnection('default', 'test');

    $reflection = new \ReflectionObject($connection);
    $connection_property = $reflection->getProperty('connection');
    $connection_property->setAccessible(TRUE);
    // Skip this test when a database driver does not implement PDO.
    // An alternative database driver that does not implement PDO
    // should implement its own connection test."
    if (get_class($connection_property->getValue($connection)) !== 'PDO') {
      $this->markTestSkipped('Ignored PDO connection unit test for this driver because it does not implement PDO.');
    }
    $error_mode = $connection_property->getValue($connection)
      ->getAttribute(\PDO::ATTR_ERRMODE);
    $this->assertEqual($error_mode, \PDO::ERRMODE_SILENT, 'Ensure PDO connection options can be overridden.');

    Database::removeConnection('test');
  }

it should be just after the first 'setAccessible' as it was in the patch in #2.

erozqba’s picture

FileSize
1.14 KB
1.85 KB

Yes, @mondrake is right! Sorry for that, I fix it in a new patch.

erozqba’s picture

Status: Needs work » Needs review
mondrake’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

+++ b/core/tests/Drupal/KernelTests/Core/Database/ConnectionUnitTest.php
@@ -221,6 +221,12 @@ public function testConnectionOpen() {
+    // should implement its own connection test."

Ending quote can be removed on commit, I think :)

  • alexpott committed eccd591 on 8.4.x
    Revert "Issue #2867700 by erozqba, mondrake, mradcliffe, cilefen:...

  • alexpott committed a02d8a1 on 8.3.x
    Revert "Issue #2867700 by erozqba, mondrake, mradcliffe, cilefen:...
alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Reverted... please provide a new patch with everything in the right place and nothing for committers to fix.

erozqba’s picture

Status: Needs work » Needs review
FileSize
1.14 KB
800 bytes
mondrake’s picture

Status: Needs review » Reviewed & tested by the community

Sorry for being nitpicky :(

Good to go for me when it turns back green. Tested #28 with an alternative db driver, works OK.

erozqba’s picture

@mondrake, I'm really happy with your suggestion because the final code will be cleaner and easy to read, so don't worry about being nitpicky at all, what it is important is the code quality, and at the end, this suggestion help me a lot, I will pay more attention in the future before submitting new changes. :)

mondrake’s picture

Running PHPUnit kernel tests for an alternative db driver that does not implement PDO connection:

Without the patch in #28 applied:

$ ../vendor/bin/phpunit --testsuite kernel --group Database,Cache --verbose
PHPUnit 4.8.35 by Sebastian Bergmann and contributors.
Runtime:	PHP 7.0.17
Configuration:	/home/travis/drupal8/core/phpunit.xml.dist
...............................................................  63 / 251 ( 25%)
.................................E............................. 126 / 251 ( 50%)
............................................................... 189 / 251 ( 75%)
..............................................................
Time: 8.03 minutes, Memory: 58.00MB
There was 1 error:
1) Drupal\KernelTests\Core\Database\ConnectionUnitTest::testConnectionOpen
Error: Call to undefined method Doctrine\DBAL\Connection::getAttribute()
/home/travis/drupal8/core/tests/Drupal/KernelTests/Core/Database/ConnectionUnitTest.php:225

With the patch in #28 applied:

$ ../vendor/bin/phpunit --testsuite kernel --group Database,Cache --verbose
PHPUnit 4.8.35 by Sebastian Bergmann and contributors.
Runtime:	PHP 7.0.17
Configuration:	/home/travis/drupal8/core/phpunit.xml.dist
...............................................................  63 / 251 ( 25%)
............................................................... 126 / 251 ( 50%)
............................................................... 189 / 251 ( 75%)
....................S.........................................
Time: 6.52 minutes, Memory: 58.00MB
There was 1 skipped test:
1) Drupal\KernelTests\Core\Database\ConnectionUnitTest::testConnectionOpen
Ignored PDO connection unit test for this driver because it does not implement PDO.
/home/travis/drupal8/core/tests/Drupal/KernelTests/Core/Database/ConnectionUnitTest.php:228

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 28: 2867700-28.patch, failed testing.

mondrake’s picture

Status: Needs work » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 84d1e51 to 8.4.x and b01ba65 to 8.3.x. Thanks!

I credited both @cilefen and @mradcliffe for providing reviews that affected the final patch. Thanks @dawehner for reviewing the issue too. And thanks to @mondrake and @erozqba for providing the patch.

Backported to 8.3.x because this is a test only change that makes things easier for people implementing alternate db drivers.

  • alexpott committed 84d1e51 on 8.4.x
    Issue #2867700 by erozqba, mondrake, mradcliffe, cilefen:...

  • alexpott committed b01ba65 on 8.3.x
    Issue #2867700 by erozqba, mondrake, mradcliffe, cilefen:...

Status: Fixed » Closed (fixed)

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