Forward compatibility with PHPUnit 6 #133
Conversation
WyriHaximus
left a comment
WyriHaximus
left a comment
There was a problem hiding this comment.
👍 Makes perfect sense to make, paves the road to PHPUnit 6 and beyond
|
According to #132 it should be 5.7.11 as min version. |
|
@andig I think that this covers a different issue, so I would vote to keep this separated for now? 👍 |
clue
left a comment
clue
left a comment
There was a problem hiding this comment.
Thank for you digging into this and filing this PR! The changes LGTM, does it make sense to you to add support for PHPUnit 6 as part of this PR? 
|
@clue Added 😄 |
tests/IntegrationTest.php
Outdated
| /** @test */ | ||
| /** | ||
| * @test | ||
| * @expectedException RuntimeException |
There was a problem hiding this comment.
Be aware that this change has implications for the test. Now, all RuntimeExceptions are expected (eg. from the setup code) while before, only exceptions thrown from connect() were expected (I'm aware this change has been made for being compatible with PHPUnit 6).
Maybe test with @expectedExceptionMessage explicitely to avoid covering other exceptions?
There was a problem hiding this comment.
Going to add @expectedExceptionMessage in all changes to double check!
There was a problem hiding this comment.
You could also simply go with something like sebastianbergmann/phpunit#2074 (comment) instead.
There was a problem hiding this comment.
@kelunik's suggestion may have the benefit of resulting in a smaller changeset (with less potential for subtle issues as below), but this would still require some additional logic to skip this on newer PHPUnit versions.
Either way, I'm fine with either solution and will leave this up to you 👍
There was a problem hiding this comment.
Guys, what should I do? expectExceptionMessage isn't available anymore on PHPUnit 6, and this assertion in variable 😢
There was a problem hiding this comment.
How about this pseudo code? Maybe even move this to a setExpectException() helper method for backwards compatibility as @kelunik suggested?
if (phpunit >= 6) {
$this->expectException('RuntimeException');
$this->expectExceptionMessage("hello");
} else {
$this->setExpectException('RuntimeException', 'hello');
}
tests/IntegrationTest.php
Outdated
| /** @test */ | ||
| /** | ||
| * @test | ||
| * @expectedException RuntimeException |
There was a problem hiding this comment.
@kelunik's suggestion may have the benefit of resulting in a smaller changeset (with less potential for subtle issues as below), but this would still require some additional logic to skip this on newer PHPUnit versions.
Either way, I'm fine with either solution and will leave this up to you 👍
tests/IntegrationTest.php
Outdated
|
|
||
| $this->expectExceptionMessage( | ||
| 'DNS query for google.com failed: Unable to connect to DNS server: php_network_getaddresses: getaddrinfo failed: ' . $result | ||
| ); |
There was a problem hiding this comment.
This will not be executed as the above method already throws. This is also the reason why it may make sense to keep the original test structure (see above discussion), but I'll leave this up to you 👍
(see also below occurrences)
There was a problem hiding this comment.
@clue Sorry, I didn't see that. I gonna move expect methods to the beginning of the methods.
|
Ok, we are facing some issues with this update. I have some solutions, and I want your opinion:
|
clue
changed the title
|
@Gabriel-Caruso Thank you for keeping up with this! I've updated this PR to add a BC layer as per the above discussion so that all PHPUnit versions are now supported 🎉 |
clue
left a comment
clue
left a comment
There was a problem hiding this comment.
Thank you for keeping up with this, changes LGTM! 👍
6 participants
I use the
PHPUnit\Framework\TestCasenotation instead ofPHPUnit_Framework_TestCasewhile extending our TestCase. This will help us migrate to PHPUnit 6, that no longer support snake case class names.I just need to bump PHPUnit version to
^4.8.35and^5.7, that support this namespace.