Skip to content

markTestIncomplete and markTestSkipped as earlyTerminatingMethodCalls #52

Open
@arnaud-lb

Description

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

Activity

ondrejmirtes

ondrejmirtes commented on Oct 3, 2019

@ondrejmirtes
Member

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

ondrejmirtes commented on Oct 3, 2019

@ondrejmirtes
Member

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

daniele-xp

daniele-xp commented on Mar 3, 2020

@daniele-xp

Hello 👋

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

grachevko commented on Mar 27, 2020

@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

Wirone commented on Aug 16, 2021

@Wirone
Contributor

@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

k0pernikus commented on Jan 13, 2022

@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

MarkVaughn commented on Mar 22, 2022

@MarkVaughn

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

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

nicodemuz commented on Jul 15, 2022

@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

VincentLanglet commented on Nov 2, 2023

@VincentLanglet
Contributor

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

ondrejmirtes commented on Nov 3, 2023

@ondrejmirtes
Member

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

jscssphtml

jscssphtml commented on Nov 11, 2023

@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

5 remaining items

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions

      markTestIncomplete and markTestSkipped as earlyTerminatingMethodCalls · Issue #52 · phpstan/phpstan-phpunit