TokensAnalyzer::isConstantInvocation - various fixes #5685
Conversation
kubawerlos
changed the title
This PR was squashed before being merged into the 2.19 branch. Discussion ---------- Calculate code coverage on PHP 8 It's almost a half a year since the release, we can use PHP 8 to calculate code coverage e.g. to avoid situation like in #5685 where it shows that coverage dropped. Commits ------- acc1806 Calculate code coverage on PHP 8
| } | ||
| } | ||
|
|
||
| public function provideIsConstantInvocationCases() | ||
| { | ||
| return [ |
There was a problem hiding this comment.
converting array to yielding just for sake of change the approach is making diff longer, review more tricky, and increases the chances for conflict while syncing this change into multiple lines. please avoid in future
There was a problem hiding this comment.
converting array to yielding just for sake of change
what about converting array to yielding for avoid code quadruplication?
Look at testIsConstantInvocation, testIsConstantInvocation70 and testIsConstantInvocation71 - exactly the same function content and in this fix, 4th copy - for PHP 8 - needs to be added. Also I want add assertion that all string tokens are checked to make it easier to read.
making diff longer, review more tricky
it is a burden someone has to bear, cleaning this up wasn't fun either.
increases the chances for conflict while syncing this change into multiple lines
how come? only changes for this file between 2.19 and 3.0 branches are migration rules.
| * @param string $source | ||
| * | ||
| * @dataProvider provideIsConstantInvocation70Cases | ||
| * @requires PHP 7.0 |
There was a problem hiding this comment.
👎
with original code, we got nice skipped test message.
now, we don't even know that some test got skipped, as it was never generated
There was a problem hiding this comment.
please recover the original way, it will also save a lot of conflicts while merging 2.19 <> 3.0
There was a problem hiding this comment.
with original code, we got nice
skipped testmessage.
what is nice about those messages? Running on PHP 8 182 are skipped, do you track them and verify if that number changes? it was always unnecessary noise to me
My reasoning is that data provider should provide test cases, and there is nothing wrong if it provides ones filtered for given PHP version.
There was a problem hiding this comment.
I agree with @keradus here. I think it's better to have a feedback about skipped tests. Otherwise, if some are skipped for wrong reasons, you won't notice it. IMO test cases generation logic should be avoided as much as possible in data providers.
There was a problem hiding this comment.
Did anyone ever benefit from those skipped tests messages?
The tests might always be skipped for wrong reasons, skipping them over if conditions in data provider won't save from it.
|
As spotted by @GrahamCampbell in #5720, union types in non-capturing catches were still considered constant usage, I allowed myself to push a fix here. |
|
Added another fix for PHP 8 attributes. |
| static::assertSame( | ||
| $expectedValue, | ||
| $tokensAnalyzer->isConstantInvocation($index), | ||
| sprintf('Token at index '.$index.' should match the expected value (%s).', $expectedValue ? 'true' : 'false') |
There was a problem hiding this comment.
if you use sprintf please add all vars (including $index) as arguments :)
| [18 => false], | ||
| ], | ||
| yield [ | ||
| '<?php #[Foo] function foo() {}', |
There was a problem hiding this comment.
please add '<?php #[\Foo] function foo() {}', , '<?php #[\A\Foo] function foo() {}' and '<?php #[A\Foo] function foo() {}',
julienfalque
changed the title
|
I'm not willing to make 4th copy of a function only to have some tests skipped in different PHP versions as those skipped message never gave any benefit. |
|
benefit may not be the message, but the fact that test was skipped. |
Fixes #5684