Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Followup for #2232861: Create BrowserTestBase for web-testing on top of Mink. In that issue are two new methods added to the class Drupal\Core\Database. These new methods need PHPUnit testing. The two methods are Database::convertDbUrlToConnectionInfo() and Database::getConnectionInfoAsUrl().
Comment | File | Size | Author |
---|---|---|---|
#23 | interdiff.txt | 1.47 KB | marcoscano |
#23 | add-unit-tests-to-db-conversion-methods-2434567-23.patch | 4.71 KB | marcoscano |
Comments
Comment #1
mgiffordComment #3
marcoscanoComment #4
marcoscanoUnit tests beginner here, wanting to learn. Feedback appreciated :)
Comment #5
daffie CreditAttribution: daffie commented@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:
Can we change this to: @coversDefaultClass \Drupal\Core\Database\Database. This is for tools that check which classes have tests for them.
Can we change this to: @covers ::convertDbUrlToConnectionInfo. The same as above.
Can we change this to: @covers ::convertDbUrlToConnectionInfo
Can we create a provider function for this test. We can then add as much special cases as we need.
This function can be droped. The provider function can be added to testDbUrltoConnectionConversion().
This test function also needs a provider function. So that we easily can add more special test cases.
Comment #6
marcoscano@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!
Comment #7
daffie CreditAttribution: daffie commented@marcoscano:
Comment #8
marcoscanoThanks @daffie, will investigate this a bit further then :)
In the meantime, here is the interdiff.txt between #4 and #6.
Thanks!
Comment #9
daffie CreditAttribution: daffie commentedAn other idea is to look at other Drupal PHP Unit tests and see how they do data providers.
Comment #10
marcoscanoHere 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!
Comment #11
marcoscanoComment #12
dawehnerCould 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.
Comment #13
marcoscanoHello @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!
Comment #14
dawehner@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.
Comment #15
marcoscanoOK then! let's get back to the previous idea :)
Attached a revised version with the tests methods split.
Thanks!
Comment #16
dawehnerJust another quick feedback.
quick CS review: let's remove those empty lines.
Note:
$this->setExpectedException()
is the best practise now, see https://thephp.cc/news/2016/02/questioning-phpunit-best-practicesQuick CS review: We use 2 spaces for indentation.
Comment #17
marcoscanoSorry for the CS issues!
Attached patch intends to solve them
Thanks again for your help and patience!
Comment #18
daffie CreditAttribution: daffie commented@marcoscano: It looks good. Almost there. A remark and a nitpick left:
Nitpick: Can you remove this line?
In providerConvertDbUrlToConnectionInfo() the expected result is added. Can you do the same forproviderGetConnectionInfoAsUrl() instead of calculating it.
Comment #19
marcoscanoAnother 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...)
Comment #20
dawehnerThat's indeed much better! Nice
Comment #21
daffie CreditAttribution: daffie commentedGood work marcoscano:
You can just give the expected url: "mysql://test_user:test_pass@test_host:3306/test_database". The same for the other two.
Comment #23
marcoscanoComment #24
daffie CreditAttribution: daffie commentedGreat work marcoscano!
Comment #27
daffie CreditAttribution: daffie commentedBack to RTBC.
Comment #30
alexpottCommitted a8e6171 and pushed to 8.1.x and 8.2.x. Thanks!
Fixed a coding standard issue on commit.