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

Use JSON source generator in WebApplicationFactory #55595

Merged

Conversation

martincostello
Copy link
Member

@martincostello martincostello commented May 8, 2024

Use JSON source generator in WebApplicationFactory

Use JSON source generator to deserialize dictionary.

Description

Use JSON source generator to deserialize JSON to prevent InvalidOperationException in a test project with JsonSerializerIsReflectionEnabledByDefault=false.

I wasn't sure how to test this without having to add an entirely new test project - happy to add something if someone can give a suggestion on how to go about that in a way that would be acceptable to the team.

Fixes #55586

@martincostello martincostello requested a review from a team as a code owner May 8, 2024 07:13
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label May 8, 2024
@mkArtakMSFT mkArtakMSFT added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label May 8, 2024
@amcasey amcasey added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates and removed area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions labels May 15, 2024
@amcasey
Copy link
Member

amcasey commented May 15, 2024

Just noticed that the old labels were set manually. Why networking, @mkArtakMSFT?

@amcasey
Copy link
Member

amcasey commented May 15, 2024

FYI @captainsafia

@captainsafia
Copy link
Member

@martincostello TY for the PR.

We reference the Mvc.Testing package in other tests in the repo (Identity.FunctionalTests, MvcFunctionalTest, for example). We can probably verify whether or not this fix applies by setting the JsonSerializerIsReflectionEnabledByDefault property in those projects and checking if the change resolves any IOE.

@martincostello
Copy link
Member Author

martincostello commented May 18, 2024

Playing around with Microsoft.AspNetCore.Mvc.FunctionalTests locally I get the following results:

JSON source generator? JsonSerializerIsReflectionEnabledByDefault Passed Failed
No true 1579 2
No false 4 1577
Yes true 1579 2
Yes false 1469 112

The tests that fail for me regardless of the settings are from AntiforgeryMiddlewareTest for Works_WithAntiforgeryMetadata_ValidToken and Works_WithAntiforgeryMetadata_ValidToken_DisableRequestSizeLimits - I'm not sure why.

@martincostello
Copy link
Member Author

The tests that fail for me regardless of the settings are from AntiforgeryMiddlewareTest for Works_WithAntiforgeryMetadata_ValidToken and Works_WithAntiforgeryMetadata_ValidToken_DisableRequestSizeLimits - I'm not sure why.

#55779

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label May 25, 2024
@dotnet-policy-service dotnet-policy-service bot added this to the 9.0-preview6 milestone May 25, 2024
@martincostello martincostello removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label May 26, 2024
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jun 3, 2024
@martincostello martincostello reopened this Jun 3, 2024
@martincostello martincostello removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jun 3, 2024
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jun 10, 2024
@martincostello martincostello removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jun 10, 2024
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jun 17, 2024
@martincostello martincostello removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jun 17, 2024
@martincostello
Copy link
Member Author

@captainsafia Anything more you need me to do here?

Copy link
Member

@captainsafia captainsafia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM -- the test failures that have been plaguing this PR are related to our ongoing issues with the Mvc.FunctionalTests and Mvc.TemplateTests.

@captainsafia captainsafia enabled auto-merge (squash) June 25, 2024 01:20
@captainsafia
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Use JSON source generator to deserialize dictionary.
Resolves dotnet#55586.
@captainsafia
Copy link
Member

captainsafia commented Jun 26, 2024

Mvc.TemplateTests are skipped and OpenAPI test fix is merged. Hopefully this goes through smoothly 🤞🏽

Edit: to that end I'll optimistically set auto-merge on this.

@captainsafia captainsafia enabled auto-merge (squash) June 26, 2024 17:25
@captainsafia captainsafia merged commit 9425978 into dotnet:main Jun 26, 2024
26 checks passed
@martincostello martincostello deleted the gh-55586-waf-json-source-gen branch June 27, 2024 08:13
@mgravell
Copy link
Member

mgravell commented Jun 27, 2024

build-ops; CI post-merge failure, but nothing to do with this PR; I'll try and poke some things

(CI failure is about changing "shipped" files inappropriately, which this PR does not even remotely do)

also an unrelated secondary failure about feel failures; I guess CI is having a very bad day

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unit test projects with JsonSerializerIsReflectionEnabledByDefault=false cannot use WebApplicationFactory
6 participants