Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mgifford’s picture

Status: Postponed » Active

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

marcoscano’s picture

Assigned: Unassigned » marcoscano
marcoscano’s picture

Version: 8.1.x-dev » 8.2.x-dev
Assigned: marcoscano » Unassigned
Status: Active » Needs review
FileSize
3.08 KB

Unit tests beginner here, wanting to learn. Feedback appreciated :)

daffie’s picture

Status: Needs review » Needs work

@marcoscano: Thank you for creating the tests. Lets work together to get this issue fixed. You tests look good, but I do have some feedback for you:

  1. +++ b/core/tests/Drupal/Tests/Core/Database/UrlConversionTest.php
    @@ -0,0 +1,97 @@
    + * Tests the methods Database::convertDbUrlToConnectionInfo() and
    + * Database::getConnectionInfoAsUrl().
    

    Can we change this to: @coversDefaultClass \Drupal\Core\Database\Database. This is for tools that check which classes have tests for them.

  2. +++ b/core/tests/Drupal/Tests/Core/Database/UrlConversionTest.php
    @@ -0,0 +1,97 @@
    +   * Test the conversion in ::convertDbUrlToConnectionInfo().
    

    Can we change this to: @covers ::convertDbUrlToConnectionInfo. The same as above.

  3. +++ b/core/tests/Drupal/Tests/Core/Database/UrlConversionTest.php
    @@ -0,0 +1,97 @@
    +   * Test the conversion in ::convertDbUrlToConnectionInfo().
    

    Can we change this to: @covers ::convertDbUrlToConnectionInfo

    +++ b/core/tests/Drupal/Tests/Core/Database/UrlConversionTest.php
    @@ -0,0 +1,97 @@
    +  public function testDbUrltoConnectionConversion() {
    

    Can we create a provider function for this test. We can then add as much special cases as we need.

  4. +++ b/core/tests/Drupal/Tests/Core/Database/UrlConversionTest.php
    @@ -0,0 +1,97 @@
    +  /**
    +   * Test ::convertDbUrlToConnectionInfo() exception for invalid arguments.
    +   *
    +   * @expectedException \InvalidArgumentException
    +   * @dataProvider providerInvalidArgumentsUrlConversion
    +   */
    +  public function testGetInvalidArgumentExceptionInUrlConversion($url, $root) {
    +    Database::convertDbUrlToConnectionInfo($url, $root);
    +  }
    

    This function can be droped. The provider function can be added to testDbUrltoConnectionConversion().

  5. +++ b/core/tests/Drupal/Tests/Core/Database/UrlConversionTest.php
    @@ -0,0 +1,97 @@
    +  public function testgetConnectionInfoAsUrl() {
    

    This test function also needs a provider function. So that we easily can add more special test cases.

marcoscano’s picture

Status: Needs work » Needs review
FileSize
4.55 KB

@daffie, many thanks for your help and feedback!

The attached patch addresses the points you raised in 1, 2, 3-a and 5.

Concerning the points in 3-b and 4:

There is maybe something conceptual about the data providers that I probably don't understand entirely, sorry... Is it possible to have a single data provider function that provides both good and bad datasets to test? If so, how can we differentiate the 2 cases inside the test function? In other words, how do we know if an exception was thrown correctly due to an invalid argument passed in by the dataprovider, or if something is broken in the code being tested and an exception was thrown with a valid dataset?

Many thanks again!

daffie’s picture

Status: Needs review » Needs work

@marcoscano:

Is it possible to have a single data provider function that provides both good and bad datasets to test?/blockquote> answer is YES.

I have two links for you: https://phpunit.de/manual/current/en/writing-tests-for-phpunit.html and http://www.startutorial.com/articles/view/phpunit-beginner-part-2-data-p.... The idea is that one of the variables from the data provider is the expected result. Can be good or bad. Hopefully this is enough help. If not please ask for more.

Also can you create an interdiff.txt file. Makes reviewing easier.

marcoscano’s picture

Assigned: Unassigned » marcoscano
FileSize
3.48 KB

Thanks @daffie, will investigate this a bit further then :)

In the meantime, here is the interdiff.txt between #4 and #6.

Thanks!

daffie’s picture

An other idea is to look at other Drupal PHP Unit tests and see how they do data providers.

marcoscano’s picture

Here a patch with the idea suggested in #7.

Nevertheless, I was not able to do it maintaining the @expectedException annotation, once this applies apparently to all test runs. I have included then conditionally the expectation with $this->setExpectedException() and this way everything seems to work fine.

I hope it looks better this way.

Thanks again for your patience!

marcoscano’s picture

Assigned: marcoscano » Unassigned
Status: Needs work » Needs review
dawehner’s picture

+++ b/core/tests/Drupal/Tests/Core/Database/UrlConversionTest.php
@@ -0,0 +1,163 @@
+  public function testDbUrltoConnectionConversion($root, $url, $database_array) {
+
+    if (empty($database_array)) {
+      // We are just testing for the Exception being thrown.
+      $this->setExpectedException(\InvalidArgumentException::class);
+      Database::convertDbUrlToConnectionInfo($url, $root);
+    }
+    else {
+      $result = Database::convertDbUrlToConnectionInfo($url, $root);
+      $this->assertEquals($database_array, $result);
+    }
+

Could we split up the test method into one that expects exceptions and one which doesn't? I guess this would improve readability a bit, as we could have a method name that makes this clear.

marcoscano’s picture

Hello @dawehner thanks for your review!

This was exactly the point I raised in #6, when the two tests were separate, and I was told to unify them :)

Which way should we go? (your suggestion or @daffie's one)
I have no preference once I'm just learning how to create tests, but wanted to understand what the best practices are in this case.

Thanks again!

dawehner’s picture

@marcoscano Ha, so we already agree :)

IMHO its good practise to optimize for readabilty and well, IMHO splitting it up into several methods improves that. There are certainly more than one example in core doing that.

marcoscano’s picture

OK then! let's get back to the previous idea :)

Attached a revised version with the tests methods split.

Thanks!

dawehner’s picture

Just another quick feedback.

  1. +++ b/core/tests/Drupal/Tests/Core/Database/UrlConversionTest.php
    @@ -0,0 +1,168 @@
    +
    ...
    +
    

    quick CS review: let's remove those empty lines.

  2. +++ b/core/tests/Drupal/Tests/Core/Database/UrlConversionTest.php
    @@ -0,0 +1,168 @@
    +   * @expectedException \InvalidArgumentException
    

    Note: $this->setExpectedException() is the best practise now, see https://thephp.cc/news/2016/02/questioning-phpunit-best-practices

  3. +++ b/core/tests/Drupal/Tests/Core/Database/UrlConversionTest.php
    @@ -0,0 +1,168 @@
    +  public function testGetInvalidArgumentExceptionInUrlConversion($url, $root) {
    +      Database::convertDbUrlToConnectionInfo($url, $root);
    +    }
    +
    ...
    +  public function providerInvalidArgumentsUrlConversion() {
    +      return [
    +          ['foo', ''],
    +          ['foo', 'bar'],
    +          ['foo://', 'bar'],
    +          ['foo://bar', 'baz'],
    +          ['foo://bar:port', 'baz'],
    +          ['foo/bar/baz', 'bar2'],
    +          ['foo://bar:baz@test1', 'test2'],
    +        ];
    +  }
    

    Quick CS review: We use 2 spaces for indentation.

marcoscano’s picture

Sorry for the CS issues!

Attached patch intends to solve them

Thanks again for your help and patience!

daffie’s picture

@marcoscano: It looks good. Almost there. A remark and a nitpick left:

  1. +++ b/core/tests/Drupal/Tests/Core/Database/UrlConversionTest.php
    @@ -0,0 +1,163 @@
    +   *
    

    Nitpick: Can you remove this line?

  2. +++ b/core/tests/Drupal/Tests/Core/Database/UrlConversionTest.php
    @@ -0,0 +1,163 @@
    +    $database = !empty($info['database']) ? '/' . $info['database'] : '';
    +    $username = !empty($info['username']) ? $info['username'] : '';
    +    $password = !empty($info['password']) ? $info['password'] : '';
    +    $prefix = !empty($info['prefix']) ? '#' . $info['prefix'] : '';
    +    $host = !empty($info['host']) ? $info['host'] : '';
    +    $port = !empty($info['port']) ? ':' . $info['port'] : '';
    +    $driver = !empty($info['driver']) ? $info['driver'] : '';
    +    $user_and_pass = !empty($username) ? $username . ':' . $password . '@' : '';
    +
    +    if ($driver == 'sqlite') {
    +      $expected_url = 'sqlite://localhost' . $database;
    +    }
    +    else {
    +      $expected_url = $driver . '://' . $user_and_pass . $host . $port . $database . $prefix;
    +    }
    

    In providerConvertDbUrlToConnectionInfo() the expected result is added. Can you do the same forproviderGetConnectionInfoAsUrl() instead of calculating it.

marcoscano’s picture

Status: Needs work » Needs review
FileSize
4.96 KB
3.28 KB

Another try.

Wasn't able to identify the line that you suggest to delete in 1. Can you please tell me which number it is? (For the empty lines, I have tried to follow the examples of other tests like ConditionTest.php, OrderByTest.php or ConnectionTest.php...)

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

That's indeed much better! Nice

daffie’s picture

Good work marcoscano:

  1. +++ b/core/tests/Drupal/Tests/Core/Database/UrlConversionTest.php
    @@ -0,0 +1,158 @@
    +    $expected_url1 = $info1['driver'] . '://' .
    +      $info1['username'] . ':' . $info1['password'] . '@' .
    +      $info1['host'] . ':' . $info1['port'] . '/' . $info1['database'];
    

    You can just give the expected url: "mysql://test_user:test_pass@test_host:3306/test_database". The same for the other two.

  2. My idea was to remove the empty line between the docblock line with @covers and the docblock line with @dataProvider. They are now all the same. i can live with that too. If the committer does not like it he can change it on commit.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: add-unit-tests-to-db-conversion-methods-2434567-19.patch, failed testing.

marcoscano’s picture

Status: Needs work » Needs review
FileSize
4.71 KB
1.47 KB
daffie’s picture

Status: Needs review » Reviewed & tested by the community

Great work marcoscano!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 23: add-unit-tests-to-db-conversion-methods-2434567-23.patch, failed testing.

The last submitted patch, 23: add-unit-tests-to-db-conversion-methods-2434567-23.patch, failed testing.

daffie’s picture

Status: Needs work » Reviewed & tested by the community

Back to RTBC.

  • alexpott committed b430fe2 on 8.2.x
    Issue #2434567 by marcoscano, daffie, dawehner: Create PHPUnit tests for...

  • alexpott committed a8e6171 on 8.1.x
    Issue #2434567 by marcoscano, daffie, dawehner: Create PHPUnit tests for...
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed a8e6171 and pushed to 8.1.x and 8.2.x. Thanks!

Fixed a coding standard issue on commit.

FILE: ...upal/core/tests/Drupal/Tests/Core/Database/UrlConversionTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 153 | ERROR | [x] The closing brace for the class must have an empty
     |       |     line before it
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------
diff --git a/core/tests/Drupal/Tests/Core/Database/UrlConversionTest.php b/core/tests/Drupal/Tests/Core/Database/UrlConversionTest.php
index f28ec73..06653f6 100644
--- a/core/tests/Drupal/Tests/Core/Database/UrlConversionTest.php
+++ b/core/tests/Drupal/Tests/Core/Database/UrlConversionTest.php
@@ -150,4 +150,5 @@ public function providerGetConnectionInfoAsUrl() {
       [$info3, $expected_url3],
     ];
   }
+
 }

Status: Fixed » Closed (fixed)

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