-
-
Notifications
You must be signed in to change notification settings - Fork 9.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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
<?php | ||
|
||
/* | ||
* This file is part of the Symfony package. | ||
* | ||
* (c) Fabien Potencier <fabien@symfony.com> | ||
* | ||
* For the full copyright and license information, please view the LICENSE | ||
* file that was distributed with this source code. | ||
*/ | ||
|
||
namespace Symfony\Bridge\PhpUnit; | ||
|
||
/** | ||
* Test that your code triggers expected error messages. | ||
* | ||
* @author Christian Flothmann <christian.flothmann@xabbuh.de> | ||
*/ | ||
final class ErrorAssert | ||
{ | ||
/** | ||
* @param string[] $expectedMessages Expected deprecation messages | ||
* @param callable $testCode A callable that is expected to trigger the expected deprecation messages when being executed | ||
*/ | ||
public static function assertDeprecationsAreTriggered($expectedMessages, $testCode) | ||
{ | ||
if (!is_callable($testCode)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. ok fine for me :) |
||
throw new \InvalidArgumentException(sprintf('The code to be tested must be a valid callable ("%s" given).', gettype($testCode))); | ||
} | ||
|
||
self::assertErrorsAreTriggered(E_USER_DEPRECATED, $expectedMessages, $testCode); | ||
} | ||
|
||
/** | ||
* @param int $expectedType Expected triggered error type (pass one of PHP's E_* constants) | ||
* @param string[] $expectedMessages Expected error messages | ||
* @param callable $testCode A callable that is expected to trigger the expected messages when being executed | ||
*/ | ||
public static function assertErrorsAreTriggered($expectedType, $expectedMessages, $testCode) | ||
{ | ||
if (!is_callable($testCode)) { | ||
throw new \InvalidArgumentException(sprintf('The code to be tested must be a valid callable ("%s" given).', gettype($testCode))); | ||
} | ||
|
||
$triggeredMessages = array(); | ||
|
||
try { | ||
$prevHandler = set_error_handler(function ($type, $message, $file, $line, $context) use ($expectedType, &$triggeredMessages, &$prevHandler) { | ||
if ($expectedType !== $type) { | ||
return null !== $prevHandler && call_user_func($prevHandler, $type, $message, $file, $line, $context); | ||
} | ||
$triggeredMessages[] = $message; | ||
}); | ||
|
||
$testCode(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't it safer to use There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. @xabbuh indeed sorry, let's forget it, I thought the syntax There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest doing There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. I forgot it is static :) |
||
} finally { | ||
restore_error_handler(); | ||
} | ||
|
||
\PHPUnit_Framework_Assert::assertCount(count($expectedMessages), $triggeredMessages); | ||
|
||
for ($i = 0; $i < count($triggeredMessages); ++$i) { | ||
\PHPUnit_Framework_Assert::assertContains($expectedMessages[$i], $triggeredMessages[$i]); | ||
} | ||
} | ||
} |
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 making it a trait?
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.
Not sure, what's the advantage given that the methods are static?
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.
indeed, static methods look fine to me.