Problem/Motivation

Rehearsing for a talk, I was tired of pointing out the low PHPUnit coverage on Database namespace.

Proposed resolution

Write tests. :-)

This one is for Drupal\Core\Database\Connection, and covers low-hanging fruit. That is, things that are easy to write tests for.

Remaining tasks

Even more coverage for other classes in this namespace.

#1938068: Convert UnitTestBase to PHPUnit
#2003342: Convert system module's database unit tests to phpunit

CommentFileSizeAuthor
#19 2124365_18.patch13.02 KBmile23
#19 interdiff.txt11.15 KBmile23
#16 interdiff.txt3.15 KBmile23
#16 2124365_16.patch12.32 KBmile23
#5 interdiff.txt3.93 KBmile23
#5 2124365-5_db_connection_phpunit.patch12.43 KBmile23
#1 2124365_db_connection_phpunit.patch10.81 KBmile23

Comments

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
StatusFileSize
new 2124365_db_connection_phpunit.patch10.81 KB

And the patch...

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Active » Needs review
dawehner’s picture

dawehner
Primary language German
commented

The following methods could probably be tested as well, but I am fine with postpone that to another issue.
The work done here is already important.

  • prepareQuery
  • setTarget/getTarget
  • setKey/getKey
  • setLogger/getLogger
  • query (kind of important, isn't it)
  • Specific methods for insert/delete/update/truncate/etc.
  • many more
  1. +++ b/core/tests/Drupal/Tests/Core/Database/ConnectionTest.php
@@ -0,0 +1,252 @@
+ * Definition of Drupal\system\Tests\Database\ConnectionTest.

    Let's use "Contains \Drupal\Tests\Core..." 

  2. +++ b/core/tests/Drupal/Tests/Core/Database/ConnectionTest.php
@@ -0,0 +1,252 @@
+  public static function getInfo() {

    {@inheritdoc} 

  3. +++ b/core/tests/Drupal/Tests/Core/Database/ConnectionTest.php
@@ -0,0 +1,252 @@
+  public function providerPrefixRoundTrip() {
...
+  public function providerTestPrefixTables() {
...
+  public function providerEscapeMethods() {
...
+  public function providerGetDriverClass() {
...
+  public function providerSchema() {
...
+  public function providerMakeComments() {
...
+  public function providerFilterComments() {

    A little bit of documentation would be cool.

dawehner’s picture

dawehner
Primary language German
commented
Issue summary: View changes
Status: Needs review » Needs work

Just some small points are needed.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
StatusFileSize
new 2124365-5_db_connection_phpunit.patch12.43 KB
new interdiff.txt3.93 KB

Thanks for the ping. :-)

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 5: 2124365-5_db_connection_phpunit.patch, failed testing.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Needs review

5: 2124365-5_db_connection_phpunit.patch queued for re-testing.

areke’s picture

areke commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

The patch no longer applies; it needs to be re-rolled.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Issue tags: -Needs reroll

Applies for me, against be70033ea1e33da35d2fcc545714ef644df6f262

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

5: 2124365-5_db_connection_phpunit.patch queued for re-testing.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Needs review
dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Let's get it in and maybe improve the test coverage even more later.

xjm’s picture

xjm
she/her
Primary language English
commented

5: 2124365-5_db_connection_phpunit.patch queued for re-testing.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

Overall this looks great. I found some very minor nitpicks. Also are there any Simpletest tests we can get rid of now these are added, or is this all completely new?

Sorry this took a while to get to and it's a bit of a nothing review.

  1. +++ b/core/tests/Drupal/Tests/Core/Database/ConnectionTest.php
@@ -0,0 +1,315 @@
+        array( // expected

    Comments should be on the line above, or is there a particular reason for this? 

  2. +++ b/core/tests/Drupal/Tests/Core/Database/ConnectionTest.php
@@ -0,0 +1,315 @@
+        'nonexistant_class', // class

    existent. 

  3. +++ b/core/tests/Drupal/Tests/Core/Database/Stub/Driver/Schema.php
@@ -0,0 +1,18 @@
+  // No-op.

    It's a bit weird for a class to be no-op, just no methods or something? 

  4. +++ b/core/tests/Drupal/Tests/Core/Database/Stub/StubPDO.php
@@ -0,0 +1,21 @@
+/**
+ * A stub of \PDO for testing purposes.
+ *
+ * We override the constructor method so that PHPUnit can mock the PDO class.
+ */

    Should either use \PDO or PDO consistently in the comments. 

  5. +++ b/core/tests/Drupal/Tests/Core/Database/ConnectionTest.php
@@ -0,0 +1,315 @@
+   * Test Connection::makeComments().

    Missing blank line under the summary here and a few other places.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
StatusFileSize
new 2124365_16.patch12.32 KB
new interdiff.txt3.15 KB

#15.1: Comments are inline to help keep track of the test data. The arrays are documented in @return, but it's sometimes hard to read data arrays. I'll just remove them and let @return do its job.

#15.3: We're testing that Connection can load a driver class, so we need a class to load. This seems to be the best way to mock that scenario, since Connection requires the file to exist and load successfully. See ConnectionTest::providerSchema(). The class is tucked away in a deeply recessed namespace, and it only gets auto-loaded during test time anyway. An improvement to Connection would include dependency injection for this kind of thing...

Formatting/typo stuff: done.

As far as the SimpleTest-based tests, there's Drupal\system\Tests\Database\ConnectionUnitTest, but it's really more of a minimal functional test, with a live database. It even skips itself if the database isn't MySQL. That's a valuable regression test, and not replaceable in PHPUnit.

ParisLiakos’s picture

ParisLiakos commented
Status: Needs review » Reviewed & tested by the community

this still applies! back to catch

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/tests/Drupal/Tests/Core/Database/Stub/StubPDO.php
@@ -0,0 +1,21 @@
+class StubPDO extends \PDO {
+
+  public function __construct() {
+    // No-op.
+  }
+
+}

On first review I was like why are we not using mockBuilder() to disable the constructor so I tried this. And got lots of errors about PDOException: You cannot serialize or unserialize PDO instances. I think we should be mentioning this in the StubPDO class so no one else has to wonder the same thing.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt11.15 KB
new 2124365_18.patch13.02 KB

Good point.

Added some explanation, and also some coding standards stuff for the test class.

ParisLiakos’s picture

ParisLiakos commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 6c410da and pushed to 8.x. Thanks!

  • Commit 6c410da on 8.x by alexpott: 
    Issue #2124365 by Mile23: Expand phpunit tests for \Drupal\Core\Database...

Status: Fixed » Closed (fixed)

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