[PhpUnitBridge] Replace ErrorAssert by @expectedDeprecation
#20255
Conversation
|
|
nicolas-grekas
force-pushed
the
phpunit-expect-deprec
branch
from
a46f9f9
to
48687eb
Compare
|
I let @xabbuh review this PR. One remark though: In https://thephp.cc/news/2016/02/questioning-phpunit-best-practices, Sebastian talks about the annotation and "deprecated" it as a best practice. So, I'm wondering if we don't also need to follow along and at least have the option to not use an annotation here, something like |
|
There is no way today to add methods to the base phpunit TestCase class (no |
|
ok, not worth it then. |
|
|
||
| return $h ? $h($type, $msg, $file, $line, $context) : false; | ||
| } | ||
| if (error_reporting()) { |
There was a problem hiding this comment.
0 !== error_reporting() as we are dealing with integers here?
There was a problem hiding this comment.
I disagree here: this practice makes the code more fragile, because it needs one more assumption to be correct. When I read this kind of code, I have to verify how null, false and array() are handled. Resorting to behavioral statements saying "there is no pb here because whatever", is the hint that this is bad practice.
| @@ -566,19 +565,17 @@ public function testSerializerCacheDisabled() | |||
|
|
|||
| /** | |||
| * @group legacy | |||
| * @requires function Symfony\Bridge\PhpUnit\ErrorAssert::assertDeprecationsAreTriggered | |||
| * @expectedDeprecation The "framework.serializer.cache" option is deprecated %s. | |||
There was a problem hiding this comment.
Before, this test wouldn't have been executed, when the PhpUnitBridge was too old. Is there any risk in always running tests now (except that the expected report may be wrong)?
There was a problem hiding this comment.
The test will be run, and if it doesn't do any assertion, it might be flagged as "risky", which is good. The deprecation-related assertion won't be run, but neither will it be when the test is skipped (and at least, the test suite will run, vs fatal error with the imperative style when not skipped).
| restore_error_handler(); | ||
| try { | ||
| $prefix = "@expectedDeprecation:\n "; | ||
| $test->assertStringMatchesFormat($prefix.implode("\n ", $this->expectedDeprecations), $prefix.implode("\n ", $this->gatheredDeprecations)); |
There was a problem hiding this comment.
This now differs in one detail from the current implementation: Previously, the order in which you declared multiple expected deprecations did not matter. Now you need to know in which order they will be triggered.
| @@ -86,17 +85,15 @@ public function testParseValidNameWithNotFoundBundle() | |||
| /** | |||
| * @group legacy | |||
| * @dataProvider provideAbsolutePaths | |||
| * @requires function Symfony\Bridge\PhpUnit\ErrorAssert::assertDeprecationsAreTriggered | |||
| * @expectedDeprecation Absolute template path support is deprecated since Symfony 3.1 and will be removed in 4.0. | |||
There was a problem hiding this comment.
We will need to update tests in older branches too (see https://github.com/symfony/symfony/blob/3.1/src/Symfony/Bundle/FrameworkBundle/Tests/Templating/TemplateNameParserTest.php#L89, for an example). Updating them should be done in separate PRs (except for deprecations introduced in 3.2).
There was a problem hiding this comment.
Sure, and all at once (there is no way to make things always green step by step)
|
Except for the few comments I much better like this implementation. 👍 |
nicolas-grekas
force-pushed
the
phpunit-expect-deprec
branch
from
48687eb
to
74a43d9
Compare
nicolas-grekas
force-pushed
the
phpunit-expect-deprec
branch
from
74a43d9
to
c344203
Compare
|
This is now mergeable asis: ErrorAssert is not removed in this PR, and still used for deprecations introduced in 3.1. Those are removed in #20268. |
|
Thank you @nicolas-grekas. |
…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#7074 For 3.2, that's what the feat. freeze is for in this case. ping @xabbuh See https://github.com/symfony/symfony/pull/20255/files?w=1 Commits ------- c344203 [PhpUnitBridge] Drop ErrorAssert in favor of @expectedDeprecation
…tedDeprecation (nicolas-grekas) This PR was merged into the 3.1 branch. Discussion ---------- [3.1][PhpUnitBridge] Drop ErrorAssert in favor of @expectedDeprecation | Q | A | ------------- | --- | Branch? | 3.1 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | no | Fixed tickets | - | License | MIT | Doc PR | - Needs #20255 first Commits ------- 5735255 [3.1][PhpUnitBridge] Drop ErrorAssert in favor of @expectedDeprecation
…iguration env vars (nicolas-grekas) This PR was merged into the master branch. Discussion ---------- [PhpUnitBridge] Doc for @expectedDeprecation & new configuration env vars Related to symfony/symfony#20255 & symfony/symfony#20256 Commits ------- a1682de [PhpUnitBridge] Doc for @expectedDeprecation & new configuration env vars
For 3.2, that's what the feat. freeze is for in this case. ping @xabbuh
See https://github.com/symfony/symfony/pull/20255/files?w=1