New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Forward compatibility with PHPUnit 6 #133
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 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? 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be aware that this change has implications for the test. Now, all RuntimeException
s 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to add @expectedExceptionMessage
in all changes to double check!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also simply go with something like sebastianbergmann/phpunit#2074 (comment) instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping @Gabriel-Caruso, what's the status here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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:
|
@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 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for keeping up with this, changes LGTM! 👍
I use the
PHPUnit\Framework\TestCase
notation instead ofPHPUnit_Framework_TestCase
while 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.35
and^5.7
, that support this namespace.