Further refactorings to PHPUnit namespaces #21688
Conversation
| return \PHPUnit_Util_ErrorHandler::handleError($errorNumber, $message, $file, $line); | ||
| } | ||
|
|
||
| return \PHPUnit\Util\ErrorHandler::handleError($errorNumber, $message, $file, $line); |
There was a problem hiding this comment.
Should probably be using a use statement (same goes for the rest)
There was a problem hiding this comment.
You mean just for the namespaced versions? I was unsure about it and therefore kept it as is. I can easily update that if wanted.
There was a problem hiding this comment.
I personal have a preference to keep the FQCN inline with no use - makes things more readable (especially true on the other cases below).
| @@ -24,7 +24,11 @@ public static function handle($errorNumber, $message, $file, $line, $context) | |||
| return true; | |||
| } | |||
|
|
|||
| return \PHPUnit_Util_ErrorHandler::handleError($errorNumber, $message, $file, $line); | |||
| if (class_exists('\PHPUnit_Util_ErrorHandler')) { | |||
There was a problem hiding this comment.
the leading backslash should be removed in all strings (this one and below)
|
@nicolas-grekas Updated. One concern is left as setExpectedException is deprecated. I could also check if a class not in the FC layer exists and therefore conditionally use setException or expectException? |
|
If needed I think I would use |
| @@ -20,7 +20,11 @@ class TranslationFilesTest extends TestCase | |||
| */ | |||
| public function testTranslationFileIsValid($filePath) | |||
| { | |||
| \PHPUnit_Util_XML::loadfile($filePath, false, false, true); | |||
| if (class_exists('PHPUnit\Util\XML')) { | |||
There was a problem hiding this comment.
to prevent any issue with the BC layer, we should check for the underscore version here
| */ | ||
| public function testFormatTypeCurrency($formatter, $value) | ||
| { | ||
| if (class_exists('PHPUnit_Framework_Error_Warning')) { | ||
| $this->setExpectedException('\PHPUnit_Framework_Error_Warning'); |
| @@ -20,7 +20,11 @@ class TranslationFilesTest extends TestCase | |||
| */ | |||
| public function testTranslationFileIsValid($filePath) | |||
| { | |||
| \PHPUnit_Util_XML::loadfile($filePath, false, false, true); | |||
| if (class_exists('PHPUnit\Util\XML')) { | |||
There was a problem hiding this comment.
check underscore version
| @@ -20,7 +20,11 @@ class TranslationFilesTest extends TestCase | |||
| */ | |||
| public function testTranslationFileIsValid($filePath) | |||
| { | |||
| \PHPUnit_Util_XML::loadfile($filePath, false, false, true); | |||
| if (class_exists('PHPUnit\Util\XML')) { | |||
| public function testParseTypeDefault() | ||
| { | ||
| if (class_exists('PHPUnit_Framework_Error_Warning')) { | ||
| $this->setExpectedException('\PHPUnit_Framework_Error_Warning'); |
There was a problem hiding this comment.
you said this method is deprecated? should be do a method_exists check and use the new way when possible?
|
Updated. |
| @@ -789,13 +780,24 @@ public function parseTypeDoubleProvider() | |||
| ); | |||
| } | |||
|
|
|||
| public function testParseTypeCurrency() | |||
| private function handleExpectedWarningException() | |||
There was a problem hiding this comment.
because the test suite is full of "setExpectedException", I think we should remove this method and do the check inline instead (unfortunately...).
the most concise way I can think of is:
$this->{method_exists($this, $_ = 'expectException') ? $_ : 'setExpectedException'}(...)
can you update the test suite in the same PR also please?
There was a problem hiding this comment.
That was quit some work... 😄 Now it should be okay. On to the next one I would say...
| @@ -46,7 +46,13 @@ public function testUnknownFragmentRenderer() | |||
| ; | |||
| $renderer = new FragmentHandler(array(), false, $context); | |||
|
|
|||
| $this->setExpectedException('InvalidArgumentException', 'The "inline" renderer does not exist.'); | |||
| if (method_exists($this, 'expectException')) { | |||
| $this->expectException('InvalidArgumentException'); | |||
There was a problem hiding this comment.
Possible to use annotation instead here? That's the last thing remaining, thanks for the work.
There was a problem hiding this comment.
That could work in the cases where we dont need any parameters but this is then more in line with new PHPUnit prefered way. I would prefer to keep it as this but your call.
There was a problem hiding this comment.
For reference: https://thephp.cc/news/2016/02/questioning-phpunit-best-practices
Let's keep it this way then.
|
Thank you @peterrehm. |
This PR was squashed before being merged into the 2.7 branch (closes #21688). Discussion ---------- Further refactorings to PHPUnit namespaces | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | no | Fixed tickets | - | License | MIT | Doc PR | - Continued work to make Symfony PHPUnit 6 compatible Commits ------- de8106f Further refactorings to PHPUnit namespaces
Continued work to make Symfony PHPUnit 6 compatible