Open
Description
The extension marks markTestIncomplete
and markTestSkipped
as earlyTerminatingMethodCalls
. The effect is that PHPStan will emit a Unreachable statement - code above always terminates.
error for any code after them.
I found that this is not necessary useful. I mean, this error is useful for detecting unexpectedly unreachable statements, but the goal of markTestIncomplete
or markTestSkipped
, is to make some code unreachable/unexecuted without actually removing it.
Metadata
Metadata
Assignees
Labels
No labels
Activity
ondrejmirtes commentedon Oct 3, 2019
It's currently hard to have the one advantage (to correctly find undefined variables) and not the other (finding unreachable code). When I first ran this analysis on codebase at my dayjob, it found some useful cases - like having a redundant
return;
below calling these methods, and even a test that wasn't supposed to be always skipped. So I think it's even useful in this current form.I usually use markTestSkipped conditionally in tests, so this problem does not apply to my usage. But I'm gonna leave this open - it's theoretically possible to solve this by introducing some kind of option to NodeScopeResolver in PHPStan core.
ondrejmirtes commentedon Oct 3, 2019
Until then, it's easy for users to ignore this with
ignoreErrors
key in phpstan.neon.daniele-xp commentedon Mar 3, 2020
Hello 👋
We found this problem annoying too. Unreachable code analysis should not inspect code below
markTestSkipped
or other related phpunit early return methods.Our dirty temporary solution is to rename the test method:
It is not the same of course. Phpunit cannot recognize and report skipped test in this way.
grachevko commentedon Mar 27, 2020
If i write static::markTestSkipped then i expected that code below will be unreachable.
Adding every time this to baseline maybe force me to make tests not skipped, but it is not exactly)
https://github.com/phpstan/phpstan-phpunit/issues/52
Wirone commentedon Aug 16, 2021
@ondrejmirtes 2 cents from my experience: today one of our tests was skipped because of some problems and when I've rebased my changes on newest develop branch I got CI failure because of that even though my changes weren't related to that test (actually there weren't any changes in code, just technical stuff).. I don't see it useful to add it to baseline files every time something like this happens 🤔 Would be great to be able to turn this rule off and consider
markTestSkipped()
as a proper code flow.k0pernikus commentedon Jan 13, 2022
Please default to meaningful behavior. I put
$this->markTestSkipped('TODO: add sanity check');
at the beginning of a test case with the full knowledge that the code below it will never be reached.It's the point of that statement! Skipping but not deleting a test for it to be fixed at a later state.
An
Unreachable statement
error makes zero sense here. Instead of helping the phpstan gets in the way breaking the ci pipeline for no reason.I don't want to ignore these kind of errors. It should never be an error to begin with.
MarkVaughn commentedon Mar 22, 2022
Anyone have sensible workarounds? This is what I will do for now
nicodemuz commentedon Jul 15, 2022
You can wrap the method call in an
if
condition that phpstan might think will sometimes be false (even tho during test execution it would never be), e.g.:This could be a slightly ugly but easy way to make the error by phpstan to disappear.
VincentLanglet commentedon Nov 2, 2023
Should we add something like
earlyTerminatingMethodCallsToSkip
andearlyTerminatingFunctionCallsToSkip
@ondrejmirtes ? I could do it (this doesn't seems that hard) but I feel like the names are horrible.ondrejmirtes commentedon Nov 3, 2023
@VincentLanglet I'd rather invent a PHPDoc tag for this.
jscssphtml commentedon Nov 11, 2023
Since only the next line is reported, this works fine:
With this apporach you are able to ignore only the skips you want to be and not ignoring them globally
5 remaining items