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

When creating HttpTest in async setup, real calls made in unit test #375

Closed
Rippletank opened this issue Oct 1, 2018 · 8 comments

Comments

Projects
4 participants
@Rippletank
Copy link

commented Oct 1, 2018

I've found what I think is a bug in HttpTest. When the HttpTest instance is setup inside an async call, calls to GetAsync actually try to call the real url. There is a test project attached below. The async setup is needed for some db calls.

Here is a test example done in MSTest:

using Microsoft.VisualStudio.TestTools.UnitTesting;
using System.Threading.Tasks;
using Flurl;
using Flurl.Http;
using Flurl.Http.Testing;

namespace TestFlurl.Tests
{
    [TestClass]
    public class UnitTests
    {
        string baseUri = "https://aaa.bbbbb.ccccc/api";
        HttpTest ht;

        public async Task AsyncTestSetup()
        {
            ht = new HttpTest();
        }
        public void TestSetup()
        {
            ht = new HttpTest();
        }

        [TestMethod]
        public async Task Test1()
        {
            await AsyncTestSetup();// This causes calls to the actual Url
            //TestSetup(); //This work fine

            var res = await baseUri.GetStringAsync();
            ht.ShouldHaveCalled(baseUri);
            ht.Dispose();
        }
    }
}

This results in this error:
Flurl.Http.FlurlHttpException: Call failed. No such host is known GET https://aaa.bbbbb.ccccc/api ---> System.Net.Http.HttpRequestException: No such host is known ---> System.Net.Sockets.SocketException: No such host is known

If the setup method is changed to a non-async method (commented out line above), there is no error.

It is possible to work around once the existence of the error is known but I'm sure this isn't intended behaviour so I'm bringing it to your attention.

Thanks for a great library!

Nick

TestProject.zip

@TomBruyneel

This comment has been minimized.

Copy link

commented Oct 4, 2018

I'm pretty sure your compiler gives you warnings about that async function as you await nothing in it ;). And I'm also pretty sure that that might be the problem

@Rippletank

This comment has been minimized.

Copy link
Author

commented Oct 5, 2018

Firstly, the compiler warning is that the the method will run synchronously, implying it should be equivalent to the second method, and doesn't mention anything about not being in test mode.

Secondly, adding "await Task.Delay(100);" to the method doesn't change the test error but removes the warning.

Finally, as background, in the actual code that first produced the error, the setup method was part of an integration test class and had the [TestInitialize] attribute to run it automatically. It contained code to set up database client and secure storage classes which require async calls. The code above is the minimum to reproduce the error. The real tests produced strange 401 errors and after investigation I realised, embarrassingly, I had been repeatedly pinging the actual REST service with garbage username, password and api keys. The work-around is fairly obviously to create two setups, one async and one not and call them in each test. However, it took an hour or more to strip the original code down until I could isolate what was breaking it.

@tmenier

This comment has been minimized.

Copy link
Owner

commented Oct 7, 2018

@Rippletank Thank you for reporting this, you're definitely on to something here.

In order for HttpTest to do its magic without DI/mocks, it needs to store the "current" test on some logical call context that is threadsafe and spans async calls. I use AsyncLocal for this on all platforms that support it and generally it works great - you can make async calls to Flurl within the context of a test and even run tests in parallel and things work as expected. But you've shown here that it does not work as expected when the HttpTest itself is created in an async method. I'm not sure why and I'll have to dig into this a bit more. In the mean time, I think you'll just have to avoid using it this way, unfortunately.

@TomBruyneel As as side note, his example is called an MVCE. Presumably his real code does something asynchronous in that method, but it's not relevant to repro this bug. It's perfect the way it is.

@tmenier tmenier added the bug label Oct 7, 2018

@Rippletank

This comment has been minimized.

Copy link
Author

commented Oct 8, 2018

Many thanks again. Yes, as you say it is straightforward to work around, once you know and it does work really well for testing.

@alexsorokoletov

This comment has been minimized.

Copy link

commented Nov 7, 2018

I would be interested in this as well. We have async fixture to setup auth tokens before the test and it often fails with the same error. If you then rerun failed tests only, it works just fine.

@tmenier

This comment has been minimized.

Copy link
Owner

commented Jan 25, 2019

I've confirmed that AsyncLocal (the mechanism HttpTest uses to maintain a "current" test context) doesn't seem to work as expected in conjunction with the async setup mechanism in multiple test frameworks. I opened in issue with XUnit with a simple repro. The maintainer thinks it looks like a legit issue so I'll wait to hear back.

In the mean time, the work-around is pretty simple: if you want a class-level HttpTest instance, instantiate it in the constructor or synchronous setup, not async setup.

tmenier added a commit that referenced this issue Feb 14, 2019

@tmenier

This comment has been minimized.

Copy link
Owner

commented Feb 14, 2019

I don't believe this is fixable. As proven here and explained here, a caller can set a value on AsyncLocal and it will persist down the logical call stack, but if an async method somewhere down that stack changes it (or sets some other value), the caller(s) up the call stack will not see it.

That's what's happening here. Flurl uses AsyncLocal to create a "test context" that flows though the SUT and into the Flurl HTTP method, telling it to fake and record the call. But if that test context is created in an async setup method, the caller of that method (the test runner) won't see it when it returns, so it'll never flow into the test. Unless there's some alternative to AsyncLocal for maintaining a logical context across async calls that I'm not aware of, I don't think this can be fixed.

The work-around is pretty simple: create the HttpTest instance in the test class's constructor or other synchronous setup mechanism. This is a gotcha though, and I should probably mention it in the docs.

@tmenier tmenier added wont-fix and removed reviewed labels Feb 14, 2019

@tmenier tmenier closed this Feb 14, 2019

tmenier added a commit that referenced this issue Feb 14, 2019

@tmenier tmenier removed the wont-fix label Apr 19, 2019

@tmenier

This comment has been minimized.

Copy link
Owner

commented Apr 19, 2019

I evaluated this work-around and concluded it's not applicable here. Any way you look at it, something would still need to set a value on AsyncLocal before the async setup method is called, and the only thing that can assure that happens is your test code, not the library. So I think we're back to this being a know/unfixable issue. I've made note of it in the docs.

@tmenier tmenier reopened this Apr 19, 2019

@tmenier tmenier added this to Backlog in Default via automation Apr 26, 2019

@tmenier tmenier moved this from Backlog to In Progress in Default Apr 26, 2019

@tmenier tmenier changed the title HttpTest being bypassed and actual call made in unit test When creating HttpTest in async setup, real calls made in unit test Apr 26, 2019

@tmenier tmenier closed this Apr 28, 2019

Default automation moved this from In Progress to Released Apr 28, 2019

@tmenier tmenier added the wont-fix label Apr 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.