-
Notifications
You must be signed in to change notification settings - Fork 25
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
refactor: update to official packages, use aspnet minimal api, net60 #7
base: master
Are you sure you want to change the base?
Conversation
should be fine but did not do a dry-run. |
This is great, thank you! Let's see if we can get some qualified eyeballs on this |
HI @riezebosch I had a review of this last night, thanks for this. Some notes
All in all, solid effort though! Once I went through the workshop, I was able to apply the delta against the current master and get that working, it was quite small
I also took the time to fix up Regarding the outstanding issues raised #1 is fixed by using #5 occurred due to pact-net-beta failing if there was an existing pact, it wasn't set to overwrite, so needed deleting and the tests rerunning Could I ask, what is?
Do you have an example of hooking this up with the auth and provider state middleware? |
@@ -10,3 +10,8 @@ | |||
# build output | |||
**/bin | |||
**/obj | |||
|
|||
.idea/ | |||
*.sln.DotSettings.user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding .vscode
here would be useful for vscode users
{ | ||
private readonly Uri BaseUri; | ||
_baseUri = baseUri; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the change _baseUri = baseUri
is not reflected in the docs which still use private readonly Uri BaseUri;
updating the code snippet in the readme is an easy copy pasta
Provider/tests/ProductTest.cs
Outdated
} | ||
}; | ||
|
||
await using var app = Startup.WebApp(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assume we would inject our test startup to use provider state middleware here?
@@ -638,7 +633,7 @@ public async void ProductDoesNotExist() | |||
.WithStatus(HttpStatusCode.NotFound); | |||
|
|||
await pact.VerifyAsync(async ctx => { | |||
var response = await ApiClient.GetProduct(11); | |||
var response = await new ApiClient(ctx.MockServerUri).GetProduct(11); | |||
Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consumer expects a 404
here, but the new API provider returns a 200
where products are not found
Sorry, I wasn't aware of your review already. Just pushed some minor changes in standard C# naming conventions. But will make sure I will look into your comments. |
No rush at all chap! |
Tutorial: Create a minimal API with ASP.NET Core. So minimal api reduces the boilerblate and focuses more on the REST API endpoints. In normal scaffolding (and also that tutorial) everything is just places in the So what I tried to do: update to minimal API to create a simpler example, but refactor it into something that opens up for testing (something that Microsoft and its examples tend to forget). |
Yeah that makes total sense, it was easy enough to update just the test code, making for a smaller delta. I think there is value in showing an alternative API style, but assume that there would be a need for both styles. Looking at the examples, I think it should be easy enough to support both side by side, and gets me into the idea of having a canonical example and then having sub-workshops which can show the same thing but with a different framework style for the API, or a different testing framework for example (thinking wider than just .NET, but for all the workshops) |
} | ||
namespace Provider; | ||
|
||
public static class Startup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the startup file still required here with the MinimalApi? We have eliminated all Startup files for .Net 6.0 in our projects, and struggling where to define the Pact Verifier Middleware in the new solution without a Startup.
Fixes #6