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

New diagnostic: Consider using Assert.ThatAsync instead of Assert.That #759

Open
Bartleby2718 opened this issue Jun 9, 2024 · 3 comments · May be fixed by #804
Open

New diagnostic: Consider using Assert.ThatAsync instead of Assert.That #759

Bartleby2718 opened this issue Jun 9, 2024 · 3 comments · May be fixed by #804
Assignees

Comments

@Bartleby2718
Copy link
Contributor

Bartleby2718 commented Jun 9, 2024

Arguably, one of the biggest advantages of using NUnit 4 is: Proper async/await, you can now await the Asserts. However, it appears that NUnit.Analyzers doesn't have an analyzer or a codefix for that.

Consider the following sync-over-async tests:

[Test]
public async Task Before()
{
    Assert.That(await GetFourtyTwoAsync(), Is.EqualTo(42));
    Assert.That(await GetFourtyTwoAsync("hello world"), Is.EqualTo(42));
}

private static Task<int> GetFourtyTwoAsync() => Task.FromResult(42);

private static Task<int> GetFourtyTwoAsync(string stringToPrintBeforehand)
{
    Console.WriteLine(stringToPrintBeforehand);
    return Task.FromResult(42);
}

It'd be great if these could be fixed to:

[Test]
public async Task After()
{
    await Assert.ThatAsync(() => GetFourtyTwoAsync(), Is.EqualTo(42)); // the first argument could be simplified to just GetFourtyTwoAsync
    await Assert.ThatAsync(() => GetFourtyTwoAsync("hello world"), Is.EqualTo(42));
}

Personally, I think it'd be neat if we could bundle all NUnit 4-related changes to NUnit4XXX. Thoughts? (It does raise the question of whether we should reserve 5XXX or above as well... 🤷)

@manfred-brands
Copy link
Member

The original test more likely looks like:

[Test]
public void Before()
{
    Assert.That(await GetFourtyTwoAsync(), Is.EqualTo(42));
    Assert.That(await GetFourtyTwoAsync("hello world"), Is.EqualTo(42));
}

So any CodeFix would also possibly have to change the method declaration.

@Bartleby2718 Bartleby2718 mentioned this issue Dec 3, 2024
Closed
Bartleby2718 added a commit to Bartleby2718/nunit.analyzers that referenced this issue Dec 6, 2024
Bartleby2718 added a commit to Bartleby2718/nunit.analyzers that referenced this issue Dec 6, 2024
Filter by NUnit version, support ConfigureAwait and non-first argument

Cleanup
Bartleby2718 added a commit to Bartleby2718/nunit.analyzers that referenced this issue Dec 6, 2024
@Bartleby2718 Bartleby2718 linked a pull request Dec 6, 2024 that will close this issue
@manfred-brands
Copy link
Member

Note that these are bad examples as the Assert.That doesn't do any async-over-sync.
The method is called asynchronously and the result is passed to the assert.

The only place where you would want to use Assert.ThatAsync is when NUnit is calling the delegate.
This would only be the case for two asserts:

  1. ThrowsConstraints where NUnit need to call the method in order to catch any exception.
  2. DelayedConstraint where NUnit needs to call the method repeatedly to verify the constraint.

@manfred-brands
Copy link
Member

Personally, I think it'd be neat if we could bundle all NUnit 4-related changes to NUnit4XXX. Thoughts?

I don't mind putting NUnit4 only diagnostics in the 4XXX range.

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

Successfully merging a pull request may close this issue.

2 participants