Bug in fixer for xUnit2014 when handling nested methods

[carlossanlop]

I encountered this unit test code in an old project using a nested method to assert a Task:

using System;
using System.Threading.Tasks;
using Xunit;

public class MyTestClass
{
    private ValueTask GetValueTaskAsync()
    {
        return ValueTask.CompletedTask;
    }

    [Fact]
    public void MyTestMethod()
    {
        ValueTask vt = GetValueTaskAsync();
        MyNestedMethod(vt);

        void MyNestedMethod(ValueTask vt)
        {
            Assert.Throws<ArgumentOutOfRangeException>(() => vt.AsTask());
        }
    }
}

I got these two failures:

• xUnit2014: Do not use Assert.Throws() to check for asynchronously thrown exceptions. Use Assert.ThrowsAsync instead.
• CS0619: Assert.Throws<T>(Func<Task>) is obsolete: You must call Assert.ThrowsAsync<T> (and await the result) when testing async code.

Actual

The code fix was suggested by xUnit2014. Unfortunately, it is wrong: it adds the missing method modifiers to the top method:

    [Fact]
    public async Task MyTestMethod()
    {
        ValueTask vt = GetValueTaskAsync();
        MyNestedMethod(vt);

        void MyNestedMethod(ValueTask vt)
        {
            await Assert.ThrowsAsync<ArgumentOutOfRangeException>(() => vt.AsTask());
        }
    }

Expected

The correct fix should have added the modifiers to the nested method, and then should've added the await keyword to the nested method invocation:

    [Fact]
    public async Task MyTestMethod()
    {
        ValueTask vt = GetValueTaskAsync();
        await MyNestedMethod(vt);

        async Task MyNestedMethod(ValueTask vt)
        {
            await Assert.ThrowsAsync<ArgumentOutOfRangeException>(() => vt.AsTask());
        }
    }

Or alternatively, not make the nested method async, return the value directly, but add await to the nested method invocation:

 [Fact]
    public async Task MyTestMethod()
    {
        ValueTask vt = GetValueTaskAsync();
        await MyNestedMethod(vt);

        Task MyNestedMethod(ValueTask vt)
        {
            return Assert.ThrowsAsync<ArgumentOutOfRangeException>(() => vt.AsTask());
        }
    }

[bradwilson]

The two failures are expected (you're calling a method marked with Obsolete so that the compiler will catch misuse rather than relying on the analyzer, but the analyzer is still required to be able to enable the fixer).

The incorrect method being modified is definitely a bug.

[jared-chevalier]

Hi. I would like to start contributing to this project, and I am currently working on a solution for this issue.

Adding the possibility of nested functions (local functions and/or anonymous functions, each of which may be invoked 0 or more times, possibly including recursive invocations) complicates the application of this rule. The documentation for xUnit2014 does not specify how these cases should be handled. I can see multiple ways they might be handled, including the two alternatives mentioned by @carlossanlop. Before continuing, I would like to confirm what the expected correct fix would be, and whether I should prioritize correctness or simplicity.

1. Should fixing a deeply nested Assert.Throws invocation cascade upwards, adding async/await to all of the parent functions and invocations within the method (stopping only when a function has no invocations or when the outermost method level is reached)? Should we fully apply this to any recursive invocations that might be present?
2. Or should fixing a deeply nested Assert.Throws invocation only fix the innermost level, which might result in possibly broken code, such as compiler errors or a Task being discarded instead of awaited?
3. Should async/await be added at all levels? Or, as @carlossanlop suggested as the second alternative expected fix, should async/await not be added to the inner levels, with Tasks being returned directly, and with async/await only added at the outermost method level?

Consider TestMethod:

[Fact]
public void TestMethod()
{
    int OuterLocalFunction()
    {
        Func<bool> outerDelegate = () =>
        {
            string InnerLocalFunction()
            {
                Action innerDelegate = () =>
                {
                    Assert.Throws<Exception>(() => Task.Delay(0));
                };

                innerDelegate();
                innerDelegate();
                string message = InnerLocalFunction(); // Recursion
                return string.Empty;
            }

            string message = InnerLocalFunction();
            InnerLocalFunction();
            return false;
        };

        bool condition = outerDelegate();
        outerDelegate();
        return 0;
    }

    int number = OuterLocalFunction();
    OuterLocalFunction();
}

If we fully fix the method, with async/await at all levels, then the result should be FullyFixedTestMethod:

[Fact]
public async Task FullyFixedTestMethod()
{
    async Task<int> OuterLocalFunction()
    {
        Func<Task<bool>> outerDelegate = async () =>
        {
            async Task<string> InnerLocalFunction()
            {
                Func<Task> innerDelegate = async () =>
                {
                    await Assert.ThrowsAsync<Exception>(() => Task.Delay(0));
                };

                await innerDelegate();
                await innerDelegate();
                string message = await InnerLocalFunction(); // Recursion
                return string.Empty;
            }

            string message = await InnerLocalFunction();
            await InnerLocalFunction();
            return false;
        };

        bool condition = await outerDelegate();
        await outerDelegate();
        return 0;
    }

    int number = await OuterLocalFunction();
    await OuterLocalFunction();
}

However, if we only fix the innermost level, then the result would be PartlyFixedTestMethod. The invocation would become Assert.ThrowsAsync, await would added to the invocation, async would be added to the lambda expression, and the type of innerDelegate would become Func<Task>. However, InnerLocalFunction and everything above would remain synchronous, and the invocations of innerDelegate would result in possibly broken code due to the returned Tasks being discarded instead of awaited.

[Fact]
public void PartlyFixedTestMethod()
{
    int OuterLocalFunction()
    {
        Func<bool> outerDelegate = () =>
        {
            string InnerLocalFunction()
            {
                Func<Task> innerDelegate = async () =>
                {
                    await Assert.ThrowsAsync<Exception>(() => Task.Delay(0));
                };

                innerDelegate(); // Task is discarded and not awaited
                innerDelegate(); // Task is discarded and not awaited
                string message = InnerLocalFunction(); // Recursion
                return string.Empty;
            }

            string message = InnerLocalFunction();
            InnerLocalFunction();
            return false;
        };

        bool condition = outerDelegate();
        outerDelegate();
        return 0;
    }

    int number = OuterLocalFunction();
    OuterLocalFunction();
}

Any guidance would be much appreciated. Thanks.

[bradwilson]

Should fixing a deeply nested Assert.Throws invocation cascade upwards, adding async/await to all of the parent functions and invocations within the method (stopping only when a function has no invocations or when the outermost method level is reached)? Should we fully apply this to any recursive invocations that might be present?

You give two examples, but I think there's an in-between third option that actually provides a lot of value without a lot of extra work.

What if you add the async to innerDelegate and await the assertion (which is the original problem that xUnit2014 highlighted), and then just do the async piece all the way up (without adding any awaits)? Then the compiler will help them out indicating where all the remaining issues are.

The fixed code would look something like this:

[Fact]
public async Task PartlyFixedTestMethod()
{
    async Task<int> OuterLocalFunction()
    {
        Func<Task<bool>> outerDelegate = async () =>
        {
            async Task<string> InnerLocalFunction()
            {
                Func<Task> innerDelegate = async () =>
                {
                    await Assert.ThrowsAsync<Exception>(() => Task.Delay(0));
                };

                innerDelegate(); // Task is discarded and not awaited
                innerDelegate(); // Task is discarded and not awaited
                string message = InnerLocalFunction(); // Recursion
                return string.Empty;
            }

            string message = InnerLocalFunction();
            InnerLocalFunction();
            return false;
        };

        bool condition = outerDelegate();
        outerDelegate();
        return 0;
    }

    int number = OuterLocalFunction();
    OuterLocalFunction();
}

And now here's what I see inside VS code:

The yellow lines are CS1998 (This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.) and CS4014 (Because this call is not awaited, execution of the current method continues before the call is completed. Consider applying the 'await' operator to the result of the call.), while the red lines are CS0029 (type incompatibilities like calling InnerLocalFunction and expecting to get back a string, but getting back a Task<string> instead because of the lack of await). This leads the developer to be able to fix the rest by themselves, with assistance from the compiler/IDE.

I think trying to do the rest of the fixing for them is over-ambitious and subject to a lot of edge cases. We can just let them do it themselves; we found the fundamental problem, and they can fix the rest.

Should async/await be added at all levels? Or, as @carlossanlop suggested as the second alternative expected fix, should async/await not be added to the inner levels, with Tasks being returned directly, and with async/await only added at the outermost method level?

Microsoft's advice is "always await, never just return", and while we agree to disagree (I think there are valid times to optimize), I think the decision to optimize is one to be left up to the developer. I'd say: always await.

[jared-chevalier]

What if you add the async to innerDelegate and await the assertion (which is the original problem that xUnit2014 highlighted), and then just do the async piece all the way up (without adding any awaits)? Then the compiler will help them out indicating where all the remaining issues are.

Yes, this makes sense and would certainly simplify the fixer compared to the other approach.

Microsoft's advice is "always await, never just return", and while we agree to disagree (I think there are valid times to optimize), I think the decision to optimize is one to be left up to the developer. I'd say: always await.

Yes, I agree that it would be better to use await here and allow the developer to decide on potential optimizations.

Thanks for your help. I will proceed to work on implementing this as you have described.

[bradwilson]

Available in 1.11.0-pre.12 https://xunit.net/docs/using-ci-builds