Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Non Async Execution causes False Positives #1331

Open
rburnham-def opened this issue Oct 29, 2024 · 13 comments
Open

Non Async Execution causes False Positives #1331

rburnham-def opened this issue Oct 29, 2024 · 13 comments

Comments

@rburnham-def
Copy link

Describe the bug

Calling Verifier.Verify synchronously can cause False Positive test results.

Minimal Repro

We recently converted from ApprovalTests and everything was compiling and passing. The Problem though is Verifier.Verify is async and it wasn't obvious. So our conversion was calling it synchronously like;

    /**
    * This will falsely pass if the verified file does not exist or match
    */
    [Test]
    public void Verify_FalsePositive()
    {
        var data = new { Value = new Random().Next(1, 100000) };
        Verifier.Verify(data);
    }

It's subtle and causes no Compiler Errors or Failing tests. But the above code Passes with a False Positive if the verified file does not exist or the output does not match. This is because Verify should be called async like;

    /**
     * This will correctly fail if the verified file does not exist or match
     */
    [Test]
    public Task Verify_ValidFailure()
    {
        var data = new { Value = new Random().Next(1, 100000) };
        return Verifier.Verify(data);
    }

We've obviously made a mistake with the conversion by my concern is how do we prevent less experienced dev's reintroducing the same mistake, thinking their snapshot tests are working fine. Perhaps at least naming the method VerifyAsync might help.

@SimonCropp
Copy link
Member

for me, in rider, i get a warning in your sample code
image

@rburnham-def
Copy link
Author

That doesn't seem to be the case for me. Is it an NUnit issue?

image

@rburnham-def
Copy link
Author

rburnham-def commented Oct 29, 2024

There does appear to be some settings around it

https://youtrack.jetbrains.com/issue/RSRP-487319/Rider-Resharper-doesnt-warn-when-not-awaiting-an-async-call

mine are set to warning

image

Im trying to see what the default settings are like but even with a new project and restoring rider settings i can't seem to get a warning.

image

I can't even figure out what setting controls that

@SimonCropp
Copy link
Member

add this to your .editorconfig

[*.cs]
resharper_return_value_of_pure_method_is_not_used_highlighting = error

@rburnham-def
Copy link
Author

rburnham-def commented Oct 30, 2024

No matter what i do it doesn't seem to want to pick up that setting from the .editorconfig. that i have at the solution level.

root=true

[*.cs]
resharper_return_value_of_pure_method_is_not_used_highlighting = error

I can still see that setting under Settings > Editor > Inspection Settings > C#. But even setting it to error there does not seem to change anything

@SimonCropp
Copy link
Member

I can still see that setting under Settings > Editor > Inspection Settings > C#. But even setting it to error there does not seem to change anything

i think you need to raise that one with jetbrains

@rburnham-def
Copy link
Author

rburnham-def commented Oct 31, 2024

I'll checkout why the warnings aren't showing with JetBrains. From what i can tell though it does look like they don't appear with default settings. I had another dev check their Rider setup and had the same problem.

I'm not sure "return value of pure method is not used" is the right option, but the "async method without an await" seems better. wouldn't the option you have as an error trigger errors for non-async methods with a return value if it's not used. Some time you don't care about the result.

@SimonCropp
Copy link
Member

Some time you don't care about the result.

for example?

@rburnham-def
Copy link
Author

For example if you wanted to test that creating a User sends an Invite email you might have something like

var mockEmailService = new Mock<IEmailService>()
 mockEmailService.Setup(mock => mock.SendNewUserEmail());

var userService = new UserService(mockEmailService.Object);

// could normally return a user or id
userService.CreateUser("Name", "[email protected]");


mockSomeClass.Verify(mock => mock.SendNewUserEmail(), Times.Once());

In this case we don't care about the return value of CreateUser.

@SimonCropp
Copy link
Member

is CreateUser marked with a [Pure] attribute?

@rburnham-def
Copy link
Author

Oh i guess not, I didn't know about that attribute. But would that apply to all Async methods or just this one?

@SimonCropp
Copy link
Member

the verify methods are marked with [Pure] attribute, so will be detected by R# and Rider

@SimonCropp
Copy link
Member

and no, i dont think most async members are marked with with [Pure]

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

No branches or pull requests

2 participants