Roslyn warning for missing Skip.If()

[thomaseyde]

Would it be possible to create a Roslyn warning for missing calls to Skip.If(...)?

[AArnott]

Would the warning be on a [SkippableFact] method specifically, that lacks a Skip.If call?

I think that would generate a lot of false positives, because Skip.If just throws a specific exception type, which triggers the test to be recognized as skipped. Any code may throw this, whether in the test method or in a test helper that the test method calls. It's therefore quite common for such a test method to not directly make this call.

[thomaseyde]

Still, a SkippableFact without conditions isn’t really skippable, is it? In those cases it should be a Fact. The point isn’t the exception thrown. That’s an implementation detail. SkippableFact and Skip.If() belongs together. Calling one without the other should be an error. I raise this issue because a fellow developer have introduced tests that runs fine in CI, but fails on my machine. I suffer too much task switching fixing irrelevant code.

…

On 4 Sep 2022, at 17:07, Andrew Arnott ***@***.***> wrote:  Would the warning be on a [SkippableFact] method specifically, that lacks a Skip.If call? I think that would generate a lot of false positives, because Skip.If just throws a specific exception type, which triggers the test to be recognized as skipped. Any code may throw this, whether in the test method or in a test helper that the test method calls. It's therefore quite common for such a test method to not directly make this call. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.

[AArnott]

SkippableFact and Skip.If() belongs together. Calling one without the other should be an error.

I disagree. This is a perfectly valid test:

[SkippableFact]
public void SomeTest()
{
   if (!good) throw new SkipException();
   // more test;
}

But more commonly, this is also perfectly valid:

[SkippableFact]
public void SomeTest()
{
   SetUpEnvironment();
   // more test;
}

void SetUpEnvironment()
{
   Skip.IfNot(OS.IsWindows);
}

See how in that later example, the Skip method call is not in the test method, but is nevertheless called from the test indirectly? We need the attribute to support the test helper method's ability to skip the test.

I raise this issue because a fellow developer have introduced tests that runs fine in CI, but fails on my machine. I suffer too much task switching fixing irrelevant code.

How does that relate to this issue? I can't imagine how a failure on your machine that passes in CI has to do with using the attribute without a Skip.If method call.
Now maybe I can imagine use of Skip.If in a [Fact] method being problematic. Adding a roslyn analyzer that would flag such a method as needing an attribute change may be worthwhile. But the attribute without a visible method call within the test method itself is not an error.

[thomaseyde]

The main problem here is I have running tests in my dev environment which shouldn't be running there. From my point of view, a Skippable without Skip.If() is the same as Fact. It also says "skippable" when it isn't, which to me is an error. But if that is not your design goal, then it's ok. And there is no point in debating this further.

…

On Sun, Sep 4, 2022 at 10:32 PM Andrew Arnott ***@***.***> wrote: SkippableFact and Skip.If() belongs together. Calling one without the other should be an error. I disagree. This is a perfectly valid test: [SkippableFact]public void SomeTest() { if (!good) throw new SkipException(); // more test; } But more commonly, this is also perfectly valid: [SkippableFact]public void SomeTest() { SetUpEnvironment(); // more test; } void SetUpEnvironment() { Skip.IfNot(OS.IsWindows); } See how in that later example, the Skip method call is not in the test method, but is nevertheless called from the test indirectly? We need the attribute to support the test helper method's ability to skip the test. I raise this issue because a fellow developer have introduced tests that runs fine in CI, but fails on my machine. I suffer too much task switching fixing irrelevant code. How does that relate to this issue? I can't imagine how a failure on your machine that passes in CI has to do with using the attribute without a Skip.If method call. Now maybe I can imagine use of Skip.If in a [Fact] method being problematic. Adding a roslyn analyzer that would flag such a method as needing an attribute change may be worthwhile. But the attribute without a visible method call within the test method itself is *not* an error. — Reply to this email directly, view it on GitHub <#26 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAD3IMYVXHSRBQL5QAXMFB3V4UBMPANCNFSM6AAAAAAQC6X654> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[AArnott]

If you were interested in sending a PR that added an analyzer that does this, I don't mind taking it, provided the default behavior is off. And we'd need to of course accept Skip.IfNot as well.