Skip to content

Commit

Permalink
Backport fix for #2731 from PHPUnit 6.4 to PHPUnit 5.7
Browse files Browse the repository at this point in the history
  • Loading branch information
@sebastianbergmann
sebastianbergmann committed Oct 14, 2017
1 parent 905862c commit 567ca23
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 7 deletions.
15 changes: 15 additions & 0 deletions src/Framework/Constraint/ExceptionMessage.php
View file
Expand Up @@ -34,6 +34,10 @@ public function __construct($expected)
*/
protected function matches($other)
{
if ($this->expectedMessage === '') {
return $other->getMessage() === '';
}

return strpos($other->getMessage(), $this->expectedMessage) !== false;
}

Expand All @@ -49,6 +53,13 @@ protected function matches($other)
*/
protected function failureDescription($other)
{
if ($this->expectedMessage === '') {
return sprintf(
"exception message is empty but is '%s'",
$other->getMessage()
);
}

return sprintf(
"exception message '%s' contains '%s'",
$other->getMessage(),
Expand All @@ -61,6 +72,10 @@ protected function failureDescription($other)
*/
public function toString()
{
if ($this->expectedMessage === '') {
return 'exception message is empty';
}

return 'exception message contains ';
}
}
12 changes: 5 additions & 7 deletions src/Framework/TestCase.php
View file
Expand Up @@ -141,21 +141,21 @@ abstract class PHPUnit_Framework_TestCase extends PHPUnit_Framework_Assert imple
*
* @var string
*/
private $expectedExceptionMessage = '';
private $expectedExceptionMessage = null;

/**
* The regex pattern to validate the expected Exception message.
*
* @var string
*/
private $expectedExceptionMessageRegExp = '';
private $expectedExceptionMessageRegExp = null;

/**
* The code of the expected Exception.
*
* @var int|string
*/
private $expectedExceptionCode;
private $expectedExceptionCode = null;

/**
* The name of the test case.
Expand Down Expand Up @@ -1085,8 +1085,7 @@ protected function runTest()
)
);

if (is_string($this->expectedExceptionMessage) &&
!empty($this->expectedExceptionMessage)) {
if ($this->expectedExceptionMessage !== null) {
$this->assertThat(
$e,
new PHPUnit_Framework_Constraint_ExceptionMessage(
Expand All @@ -1095,8 +1094,7 @@ protected function runTest()
);
}

if (is_string($this->expectedExceptionMessageRegExp) &&
!empty($this->expectedExceptionMessageRegExp)) {
if ($this->expectedExceptionMessageRegExp !== null) {
$this->assertThat(
$e,
new PHPUnit_Framework_Constraint_ExceptionMessageRegExp(
Expand Down

5 comments on commit 567ca23

@keradus
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, what about setExpectedException now ?
if I set ->setExpectedException(MyException::class, '') it wont trigger checking if exception msg is empty.
Sadly, default value for $message param is '', but expected behaviour is when one pass only exception name, we shall not test message. To not break interface, it shall be checked via number of arguments to not break BC by changing default value of param

@sebastianbergmann
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setExpectedException() no longer exists in PHPUnit 6, that is why I forgot about it when backporting the fix to PHPUnit 5.7.

Part of the motivation for expectException() etc. was the ugly API of setExpectedException() that used optional parameters.

What do you suggest should be done now for PHPUnit 5.7, @keradus? And can you send a pull request? :-)

@keradus
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally agree about dropping setExpectedException 👍
yet, PHPUnit 5 has it, and after this change it's problematic.
That's why I like to introduce any change via PR, even if it means I sent PR to my own project ;) More ppl have chance to look at change before, not after it's merged.

What do you suggest should be done now for PHPUnit 5.7,

As I wrote:

To not break interface, it shall be checked via number of arguments to not break BC by changing default value of param

So, keep prototype of this method as is (setExpectedException($exception, $message = '', $code = null)), but if one called method only with one argument, assume that $message=null instead of empty string. This one, if one call it with 2 arguments and $message='', we shall respect it.

If you agree with my proposal, I could sent a PR, sure!

@sebastianbergmann
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me. Looking forward to your pull request!

@keradus
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#2807

Please sign in to comment.