pact-foundation / pact-workshop-dotnet-core-v1

A workshop for Pact using .NET Core
Apache License 2.0
76 stars 209 forks source link

Provider test could be improved slightly #7

Open mattfrear opened 6 years ago

mattfrear commented 6 years ago

Hello

Firstly thanks for a great resource, it has been very helpful. I have a suggestion. When you run the provider tests, you suggest running the api using dotnet run and then in another command window, running the tests using dotnet test.

My suggestion is that the provider test could instead start the API under test using Webhost.CreateDefaultBuilder()

Then you wouldn't need to dotnet run, you could test it all using dotnet test. Example:

public ProviderApiTests(ITestOutputHelper output)
{
    _outputHelper = output;

    _pactServiceUri = "http://localhost:9001";
    _providerStateHost = WebHost.CreateDefaultBuilder()
        .UseUrls(_pactServiceUri)
        .UseStartup<TestStartup>()
        .Build();

    _providerStateHost.Start();

    _providerUri = "http://localhost:9000";
    _sut = WebHost.CreateDefaultBuilder()
        .UseUrls(_providerUri)
        .UseStartup<Startup>()
        .Build();

    _sut.Start();
}

I actually just wasted half a day trying to get it to run using the new ASP.NET Core 2.1 WebApplicationFactory, ala https://docs.microsoft.com/en-us/aspnet/core/test/integration-tests?view=aspnetcore-2.1

The problem with using WebApplicationFactory is that when you create a client via var client = _factory.CreateClient(), it seems that only that client can access the TestServer - it's not accessible to other clients such as your local browser, or the PactVerifier. Therefore I had to use WebHost.CreateDefaultBuilder() as you have done.

tdshipley commented 6 years ago

Hey, Matt thanks for raising this. Sorry about the delay getting back to you.

So I have been thinking about your suggestion. On the one hand, it makes running the tests more fluid as you say by reducing commands needed to run tests. But on the other, if this was a production test pack with lots of services would you want the tests to start services under test? I know some organisations do this and others that don't. I guess it depends on the organisation itself.

Also, I am worried about too much magic happening when people are learning something new. I don't know about you but when I am learning a new piece of tech I prefer to do most steps manually even if in a production environment I would automate them. It helps me understand the process of the technology I am learning about.

So on balance, I would prefer to keep the commands separated. Partly because I am unsure if in a production environment you would combine them but mostly not to confuse less experienced engineers who are learning about this tech.

Hopefully, that makes sense but thanks for taking the time to create a detailed issue I do appreciate it. Always good to get some feedback to think about!

mattfrear commented 6 years ago

Yeah, there are quite a lot of moving parts to get all this working. My line of thinking was you're already starting the provider state webhost in the test constructor, so you might as well start the system under test too.

I think the most confusing bit is the provider state webhost and what it is for.

Thanks again for a good resource.

tdshipley commented 6 years ago

No problem glad you found it useful! I will take a look at the provider state webhost bit and see if there is something I can do to make it clearer.

sergiusignacius commented 5 years ago

All other examples I find around this topic do start the server in the test itself (https://github.com/pact-foundation/pact-net). From an automation standpoint, delegating the startup to something else (a script or even having it in a container) seems to be a lot of heavy lifting for a test right?

tdshipley commented 5 years ago

Yeah it is and it could be improved. I’m a bit busy right now but I would like to iterate on this workshop a bit - it was written a year ago in a bit of a rush! PRs are gratefully received however ;)

sergiusignacius commented 5 years ago

Ok! Wasn't sure you wanted people helping. I'm using the workshop to learn and write my own tests. I'll create a PR to try and update it!

On Sat, 1 Jun 2019, 11:32 Thomas Shipley, notifications@github.com wrote:

Yeah it is and it could be improved. I’m a bit busy right now but I would like to iterate on this workshop a bit - it was written a year ago in a bit of a rush! PRs are gratefully received however ;)

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/tdshipley/pact-workshop-dotnet-core-v1/issues/7?email_source=notifications&email_token=AADNNR4WXX7Y5GAX6NE2KXDPYJF45A5CNFSM4FQXVSU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWW5WAA#issuecomment-497933056, or mute the thread https://github.com/notifications/unsubscribe-auth/AADNNRYAOR6OVNXQ3EMLNPLPYJF45ANCNFSM4FQXVSUQ .

tdshipley commented 5 years ago

Awesome well I hope it’s helpful and absolutely any contributions would be great!