Skip to content

[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

Merged
merged 1 commit into from
Jun 15, 2016
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 66 additions & 0 deletions src/Symfony/Bridge/PhpUnit/ErrorAssert.php
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
Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Member

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.

{
/**
* @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)) {
Copy link
Contributor

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

Copy link
Member Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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();
Copy link
Contributor

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 use call_user_func here as well ?

Copy link
Member

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.

Copy link
Contributor

@GuilhemN GuilhemN Jun 14, 2016

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 to closure

Copy link
Member Author

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.

Copy link
Contributor

@GuilhemN GuilhemN Jun 14, 2016

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...

Copy link
Member

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

Copy link
Contributor

@GuilhemN GuilhemN Jun 14, 2016

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 ?

Copy link
Member

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 :)

} 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]);
}
}
}