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
Comment | File | Size | Author |
---|---|---|---|
#28 | interdiff-2867700-22-28.txt | 800 bytes | erozqba |
#28 | 2867700-28.patch | 1.14 KB | erozqba |
#2 | 2867700-2.patch | 1.02 KB | mondrake |
Comments
Comment #2
mondrakeComment #3
mondrakeComment #4
dawehnerThis seems reasonable for me
Comment #6
mondrakeComment #7
cilefen CreditAttribution: cilefen commentedOther 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."?
Comment #8
cilefen CreditAttribution: cilefen commentedA novice can complete #7.
Comment #9
mradcliffeTagging for Baltimore2017.
Comment #10
erozqba CreditAttribution: erozqba commentedApply change proposed in #7.
Comment #11
mradcliffeThank you for posting both a patch and an interdiff!
Comment text should not exceed 80 characters per line as seen on the API Documentation and Comment Standards page.
T
Comment #12
dawehnerThe new error message doesn't makes that much sense ... given this issue is all about enabling people to build db drivers without PDO.
Comment #13
erozqba CreditAttribution: erozqba commentedApply change proposed in comment #11
Comment #14
mradcliffeSorry, 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."
Probably needs to be
!==
, right?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."
Comment #15
erozqba CreditAttribution: erozqba commentedComment #16
dawehnerThat is indeed a better message now.
Comment #17
alexpottCommitted 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.
Comment #20
mondrakeThanks all!
With regards to
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.
Comment #21
mondrakeActually, 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:it should be just after the first 'setAccessible' as it was in the patch in #2.
Comment #22
erozqba CreditAttribution: erozqba commentedYes, @mondrake is right! Sorry for that, I fix it in a new patch.
Comment #23
erozqba CreditAttribution: erozqba commentedComment #24
mondrakeThanks!
Ending quote can be removed on commit, I think :)
Comment #27
alexpottReverted... please provide a new patch with everything in the right place and nothing for committers to fix.
Comment #28
erozqba CreditAttribution: erozqba commentedComment #29
mondrakeSorry for being nitpicky :(
Good to go for me when it turns back green. Tested #28 with an alternative db driver, works OK.
Comment #30
erozqba CreditAttribution: erozqba commented@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. :)
Comment #31
mondrakeRunning PHPUnit kernel tests for an alternative db driver that does not implement PDO connection:
Without the patch in #28 applied:
With the patch in #28 applied:
Comment #33
mondrakeComment #34
alexpottCommitted 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.