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

Abstract away the TestServer dependency from WebApplicationBuilder #60247

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mkArtakMSFT
Copy link
Member

@mkArtakMSFT mkArtakMSFT commented Feb 7, 2025

Description

  • Replaces (avoiding breaking changes the TestServer dependency with a new ITestServer abstraction in the WebApplicationFactory.
  • Updated the WebApplicationFactory to depend on the ITestServer abstraction
  • Updated the GenerateMvcTestManifestTask to generate relative contentRoot paths in test manifests.

Validation

How do I know if this change didn't break something? Well, at first it did and many integration tests in the repo started failing. So these, naturally become a validation ground for this change.

Fixes #4892

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Feb 7, 2025
@mkArtakMSFT mkArtakMSFT force-pushed the mkArtakMSFT/issue_4892 branch 2 times, most recently from 8cf15f6 to 9c0fb14 Compare February 12, 2025 05:35
@mkArtakMSFT mkArtakMSFT marked this pull request as ready for review February 12, 2025 05:35
@Copilot Copilot bot review requested due to automatic review settings February 12, 2025 05:35
@mkArtakMSFT mkArtakMSFT requested review from a team and halter73 as code owners February 12, 2025 05:35

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 12 changed files in this pull request and generated no comments.

Files not reviewed (7)
  • src/Hosting/TestHost/src/PublicAPI.Unshipped.txt: Language not supported
  • src/Mvc/Mvc.Testing/src/PublicAPI.Unshipped.txt: Language not supported
  • src/Mvc/Mvc.Testing/src/Resources.resx: Language not supported
  • src/Mvc/test/Mvc.FunctionalTests/ApiExplorerTest.cs: Evaluated as low risk
  • src/Mvc/test/Mvc.FunctionalTests/Infrastructure/MvcTestFixture.cs: Evaluated as low risk
  • src/Identity/test/Identity.FunctionalTests/Infrastructure/ServerFactory.cs: Evaluated as low risk
  • src/Mvc/test/Mvc.FunctionalTests/ComponentRenderingFunctionalTests.cs: Evaluated as low risk
Comments suppressed due to low confidence (1)

src/Mvc/Mvc.Testing/src/WebApplicationFactory.cs:161

  • [nitpick] The method name 'Initialize' might be too generic. Consider renaming it to 'InitializeServer' for better clarity.
public void Initialize()
@mkArtakMSFT
Copy link
Member Author

mkArtakMSFT commented Feb 12, 2025

Still battling the issue introduced by the relative contentRoot change.
Looks like I've got the tests working now. Just some leftover issues to react / cleanup and the PR is ready to go.

@captainsafia
Copy link
Member

@mkArtakMSFT We'll need to do an API review for this since it includes a changes to the WAF interface.

Have you had the chance to use these changes with a Playwright-based setup?

@mkArtakMSFT mkArtakMSFT force-pushed the mkArtakMSFT/issue_4892 branch from e5aff71 to bfa3c20 Compare February 12, 2025 23:50
@mkArtakMSFT
Copy link
Member Author

Have you had the chance to use these changes with a Playwright-based setup?

Will do!

…ency with from in the WebApplicationFactory.

- Updated the WebApplicationFactory to depend on the ITestServer abstraction
- Fixed the public API declarations
- React to changes in the WebApplicationFactory
- Use solution-relative contentRoot path, when the path from metadata either is not available or doesn't exist.
- If none of the paths exist, use the AppContext.BaseDirectory instead.
- Fix tests with outdated expectations
@mkArtakMSFT mkArtakMSFT force-pushed the mkArtakMSFT/issue_4892 branch from bfa3c20 to 1f62155 Compare February 13, 2025 00:57
@mkArtakMSFT
Copy link
Member Author

I have discovered an issue where the ITestServer creation happens through two different flows. I'm looking into whether this needs to be unified or not. Will update the PR once that's cleared.

@mkArtakMSFT mkArtakMSFT force-pushed the mkArtakMSFT/issue_4892 branch from c0a16cc to 1f62155 Compare February 14, 2025 22:47
…er as obsolete and introduce a new CreateTestServer method.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve automated browser testing with real server
2 participants