Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[PhpUnitBridge] add a triggered errors assertion helper#18880
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
76cfc10 tob8fccedComparexabbuh commentedMay 26, 2016
@nicolas-grekas Any idea why the |
GuilhemN commentedMay 28, 2016
@xabbuh the phpunit-bridge is not required in several composer files such the one of the framework bundle (https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/composer.json). |
GuilhemN commentedMay 28, 2016
Anyway the tests probably won't pass as long as this isn't merged. |
| */ | ||
| final class ErrorAssert | ||
| { | ||
| public static function assertDeprecationsAreTriggered($expectedMessages, callable $testCode) |
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.
should we use singular here?assertDeprecationIsTriggered or anything likesetExpectedException ?
setExpectedDeprecation ?
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.
Given that the method takes an array of expected messages, plural is fine.
setExpectedDeprecation is not fine for the current API. It would work if the method was register the expectation to check it later after running the test, but this is not what happens in the current API.
OskarStarkMay 31, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Given that the method takes an array of expected messages, plural is fine.
Ok fine, but do we have many cases where we assert many deprecations?
https://github.com/symfony/symfony/pull/18880/files/b8fcced6cd5b6f324e85c232fbb853f514fe8ea3#diff-ecb644500d31b54f3fa4574298dc6156R537
setExpectedDeprecation is not fine for the current API. It would work if the method was register the expectation to check it later after running the test, but this is not what happens in the current API.
👍
stof commentedMay 31, 2016
@Ener-Getick we have a special phpunit launcher performing a local composer-based installation of PHPUnit (to remove unnecessary dependency which would hurt us, for instance |
stof commentedMay 31, 2016
@xabbuh looks like the PHPUnit setup installed the 3.1.0-beta1 version of the bridge, and so does not use the updated version of the bridge:https://travis-ci.org/symfony/symfony/jobs/133035973#L578 |
GuilhemN commentedJun 1, 2016
@stof thanks for the explanation, I didn't know that. |
| $triggeredMessages[] = $message; | ||
| }); | ||
| $testCode(); |
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.
What about putting the entire function in a try/finally structure in case an exception is thrown ?
try {// ...}finally {restore_error_handler();}
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.
done
fabpot commentedJun 14, 2016
ping@nicolas-grekas |
| $triggeredMessages = array(); | ||
| set_error_handler(function ($type, $message) use ($expectedType, &$triggeredMessages) { | ||
| if ($expectedType !== $type) { |
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.
shouldn't this be a bitfield?
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.
I don't think that that would be a good idea. I think if you really need to test for different kind of errors, you should probably rethink your code or write your own assertion code.
6458c99 toec0464cComparenicolas-grekas commentedJun 14, 2016
👍 |
| $triggeredMessages[] = $message; | ||
| }); | ||
| $testCode(); |
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.
Isn't it safer to usecall_user_func here as well ?
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.
Nope: no code has been written that can't be run. Having it here since the beginning will enforce this forever.
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.
so we won't accept other callables than closure ?
Edit: if yes, then we should change the typehint toclosure
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.
@Ener-Getick But that would mean to drop support for other callables that are not anonymous functions.
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.
@xabbuh indeed sorry, let's forget it, I thought the syntax$bar() only worked with closures...
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.
I suggest doing$testCode($this) to ease writing tests in 5.3
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.
@nicolas-grekas the class is static.
Edit: or did you mean adding it to the method signature ?
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.
I forgot it is static :)
| */ | ||
| public static function assertDeprecationsAreTriggered($expectedMessages, $testCode) | ||
| { | ||
| if (!is_callable($testCode)) { |
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.
useless here as will be checked in the second function anyway
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.
That's true but I'd like to keep it here too for two reasons: Do not confuse users by letting them deal with exceptions that have been thrown by methods they did not call if we cannot avoid that. Secondly, mitigate the risk of breaking anything if we refactor the method that we call 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.
ok fine for me :)
nicolas-grekas commentedJun 15, 2016
Status: reviewed |
fabpot commentedJun 15, 2016
Thank you@xabbuh. |
…r (xabbuh)This PR was merged into the 3.2-dev branch.Discussion----------[PhpUnitBridge] add a triggered errors assertion helper| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |In the past, we updated the test code several times that made sure that deprecation messages are triggered. I think we should move this code to a reusable class in the PhpUnitBridge to make it available everywhere and to be able to change a single method in case we need to update the logic.Commits-------b5c2095 add a triggered errors assertion helper
…cation` (nicolas-grekas)This PR was merged into the 3.2-dev branch.Discussion----------[PhpUnitBridge] Replace ErrorAssert by `@expectedDeprecation`| Q | A| ------------- | ---| Branch? | 3.2| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#18880| License | MIT| Doc PR |symfony/symfony-docs#7074For 3.2, that's what the feat. freeze is for in this case. ping@xabbuhSeehttps://github.com/symfony/symfony/pull/20255/files?w=1Commits-------c344203 [PhpUnitBridge] Drop ErrorAssert in favor of @expectedDeprecation
In the past, we updated the test code several times that made sure that deprecation messages are triggered. I think we should move this code to a reusable class in the PhpUnitBridge to make it available everywhere and to be able to change a single method in case we need to update the logic.