markTestIncomplete and markTestSkipped as earlyTerminatingMethodCalls

[arnaud-lb]

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.

[ondrejmirtes]

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]

Until then, it's easy for users to ignore this with ignoreErrors key in phpstan.neon.

[daniele-xp]

Hello :wave:

We found this problem annoying too. Unreachable code analysis should not inspect code below markTestSkipped or other related phpunit early return methods.

public function testService(): void
{
    $this->markTestSkipped('Skipped due temporary unavailable service');
    // ..... unreachable test code here ....
}

Our dirty temporary solution is to rename the test method:

// Skipped due temporary unavailable service
public function skipTestService(): void
{
    // ..... unreachable test code here ....
}

It is not the same of course. Phpunit cannot recognize and report skipped test in this way.

[grachevko]

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)

[Wirone]

@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]

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]

Anyone have sensible workarounds? This is what I will do for now

- 
   message: "#^Unreachable statement - code above always terminates.$#"
   path: tests

[nicodemuz]

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.:

if (time() > 1000) {
    $this->markTestIncomplete('This test is still incomplete.');
}

This could be a slightly ugly but easy way to make the error by phpstan to disappear.

[VincentLanglet]

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.

Should we add something like earlyTerminatingMethodCallsToSkip and earlyTerminatingFunctionCallsToSkip @ondrejmirtes ? I could do it (this doesn't seems that hard) but I feel like the names are horrible.

[ondrejmirtes]

@VincentLanglet I'd rather invent a PHPDoc tag for this.

[jscssphtml]

Since only the next line is reported, this works fine:

$this->markTestSkipped('Not implemented yet');
// @phpstan-ignore-next-line

With this apporach you are able to ignore only the skips you want to be and not ignoring them globally