Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Unit test OrderingScenarios.Cancel_basket_and_check_order_status_cancelled fails #1407

Closed
InstanceFactory opened this issue Aug 21, 2020 · 4 comments
Labels

Comments

@InstanceFactory
Copy link
Contributor

InstanceFactory commented Aug 21, 2020

Running the unit test OrderingScenarios.Cancel_basket_and_check_order_status_cancelled in my Windows 10 dev machine, it fails, showing the following error:

Message: 
    Autofac.Core.DependencyResolutionException : An exception was thrown while activating Swashbuckle.AspNetCore.SwaggerGen.SwaggerGenerator -> λ:Swashbuckle.AspNetCore.SwaggerGen.ISchemaGenerator -> Microsoft.Extensions.Options.OptionsManager`1[[Swashbuckle.AspNetCore.SwaggerGen.SchemaGeneratorOptions, Swashbuckle.AspNetCore.SwaggerGen, Version=5.0.0.0, Culture=neutral, PublicKeyToken=d84d99fb0135530a]] -> Microsoft.Extensions.Options.OptionsFactory`1[[Swashbuckle.AspNetCore.SwaggerGen.SchemaGeneratorOptions, Swashbuckle.AspNetCore.SwaggerGen, Version=5.0.0.0, Culture=neutral, PublicKeyToken=d84d99fb0135530a]] -> λ:Microsoft.Extensions.Options.IConfigureOptions`1[[Swashbuckle.AspNetCore.SwaggerGen.SchemaGeneratorOptions, Swashbuckle.AspNetCore.SwaggerGen, Version=5.0.0.0, Culture=neutral, PublicKeyToken=d84d99fb0135530a]][] -> Swashbuckle.AspNetCore.SwaggerGen.ConfigureSchemaGeneratorOptions.
    ---- Autofac.Core.DependencyResolutionException : An exception was thrown while invoking the constructor 'Void .ctor(System.IServiceProvider, Microsoft.Extensions.Options.IOptions`1[Swashbuckle.AspNetCore.SwaggerGen.SwaggerGenOptions])' on type 'ConfigureSchemaGeneratorOptions'.
    -------- System.UriFormatException : Invalid URI: The format of the URI could not be determined.

Doing some debugging sessions, I had to implement several changes to finally make this test pass. This is the list of issues I identified:

1. Missing Settings Value

In src\Tests\Services\Application.FunctionalTests\Services\Ordering\appsettings.json, the setting IdentityUrlExternal was missing. I added the key and set the value to http://localhost:5105.

2. Missing call to app.UseAuthorization

Same as described by #1404, the call to app.UseAuthorization()was missing in OrderingTestsStartup.ConfigureAuth. Accordingly, I added the call.

As my local code already contains the changes of PR #1406, I only had to add this call to OrderingTestsStartup.

3. Missing Claim having Type ClaimType.Name

In the test method, the call to await basketClient.PostAsync(BasketScenariosBase.Post.CheckoutOrder, … causes an exception inBasketController.CheckoutAsync line 73

var userName = this.HttpContext.User.FindFirst(x => x.Type == ClaimTypes.Name).Value;

Reason is that FindFirst returns null, as the user does not have a claim of the required type. As returning nullis not handled, a NullReference exeption is thrown instead of checking out the order.

To solve this, I added identity.AddClaim(new Claim(ClaimTypes.Name, IDENTITY_ID)); to the Invoke method of FunctionalTests.Middleware.AutoAuthorizeMiddleware.

4. OrderingScenarios.TryGetNewOrderCreated returns wrong Order Item

Before verifying if the current order (retrieved from orderClient.GetStringAsync(OrderingScenariosBase.Get.Orders)) is the one that was created by the unit test, OrderingScenarios.TryGetNewOrderCreated sets the city property of the current order to the city parameter passed for verification (the value of the newly created order). Then, OrderingScenarios.IsOrderCreated is called to verify if the current order is the created one, using the city value as the unique identifier. Of course, because the value was set before the call, the method always returns true, even in case the current order is not the created.

To make the method work properly, I removed the assignment of the cityproperty of the current order from OrderingScenarios.TryGetNewOrderCreated.

5. OrdersController.CancelOrderAsync does not cancel the Order

Even though passing the proper order number (or database id) to OrdersController.CancelOrderAsync via the appropriate HTTP POST request, the order was not cancelled.

Reason is that the JSON deserializer did not set the OrderNumber property of the Ordering.API.Application.Commands.CancelOrderCommand.

This was caused by the fact that the setter of this property is declared as private. Making it public enabled the deserializer to set the property.

Having all these changes done, the test case passed.

Left open: Exception thrown when trying to publish Integration Event

But still an exception was thrown during the process while trying to publish the integration event notifying an order was cancelled. As this does not impact the current implementation of the unit test, I will raise a different issue for that. (update: see #1408)

Maybe it’s a good idea to extend the unit test to also verify all expected integration events will be published and processed (if required). But that's not part of this issue.

Offer to create a PR

If you agree, I will create a PR to address this issue and fix the unit test..

Kind Regards,

Stefan

@sughosneo
Copy link
Contributor

Hi Stefan, thank you for providing all the descriptions for the change.

Apart from step 5. all the other steps look good to me. We have already CancelOrderCommand() constructor to set the correct OrderNumber for CancelOrderCommand class. We might need to deal with DTO rather than changing the private setter property of the actual command class itself. Thoughts ?

You can submit the PR, for detailed review and testing. If any other changes are required, then we can always discuss and update :)

@sughosneo sughosneo added the bug label Aug 25, 2020
@InstanceFactory
Copy link
Contributor Author

Hi Sumit,

Thanks for your feedback. I do understand that you do not want to change the visibility of the OrderNumber setter.

Accordingly, I've changed OrdersController.CancelOrderAsync to accept a WebMVC.Services.ModelDTOs.OrderDTO(as this already exists and is used by the test case). In consequence, a reference to Web\WebMVC\WebMVC.csproj was added to the Services\Ordering\Ordering.API\Ordering.API.csproj.

Not sure if this is your intention. To me, the DTO would fit better to Ordering.API.Application.ModelDTO (new namespace within the service), which would not require a reference to the MVC project. Or even an assembly that will be used by the MVC and service project, but this might be too much for this kind of change. Please let me know what you think / prefer.

To my understanding (and test of the MVC app), this change of the method parameter should not impact any client as the JSON representation does not change, means for external use this should not make any difference and should not require any changes on the client side (as long as the REST API is used).

Changing the method's parameter also required to update src\Services\Ordering\Ordering.UnitTests\Application\OrdersWebApiTest.cs, as it contains two direct calls to the method.

I will wait for your feedback before raising the PR.

Thanks,

Stefan

InstanceFactory added a commit to InstanceFactory/eShopOnContainers that referenced this issue Aug 26, 2020
…arios.Cancel_basket_and_check_order_status_cancelled pass

for details, see dotnet-architecture#1407
contains also a fix to FunctionalTests.Services.Basket.BasketTestsStartup.ConfigureAuth, described by dotnet-architecture#1404 , as these changes were not merged when implementing this fix.
@InstanceFactory
Copy link
Contributor Author

Hi @sughosneo ,

After finding another DTO class within the Ordering.API project (Microsoft.eShopOnContainers.Services.Ordering.API.Application.Commands.OrderDraftDTO, as part of file src\Services\Ordering\Ordering.API\Application\Commands\CreateOrderDraftCommandHandler.cs), I decided to create a "local" DTO and added it to the project.

You can find the result in PR #1414. Please let me know in case you prefer a different approach or like to have other implementation changes for this fix.

Thanks,

Stefan

@erjain
Copy link
Contributor

erjain commented Jun 3, 2022

Considering the original question has been answered, I am closing this issue as of now. Please feel free to reopen if needed. Thank you.

@erjain erjain closed this as completed Jun 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants