Skip to content

Conversation

@Krishanthaudayakumara
Copy link
Collaborator

@Krishanthaudayakumara Krishanthaudayakumara commented Sep 19, 2025

Description / Motivation

This PR removes the legacy TestServerBuilder usage from the test and benchmark suites and migrates integration tests to the WebApplicationFactory<TestWebApplicationProgram> pattern (TestWebApplicationFactory). It preserves each fixture’s middleware and service ordering (rendering engine, experience editor, tracking, localization, forwarded headers, visitor identification, etc.) and keeps per-test substitutes/mock handlers where tests rely on specific layout responses.

Key changes

  • Replaced TestServerBuilder/TestServer with WebApplicationFactory<TestWebApplicationProgram> in all integration tests and benchmarks.
  • Preserved and validated middleware ordering per-fixture (kept attribute-based rendering behavior by omitting global UseSitecoreRenderingEngine where necessary).
  • Replaced direct TestServer.CreateClient()/Services usage with factory.CreateClient()/factory.Services and updated disposal.
    Added/kept per-test ISitecoreLayoutClient and ISitecoreLayoutSerializer substitutes where fixtures require specific SitecoreLayoutResponse content (e.g., Experience Editor cases that map databaseName = "master").
  • Migrated benchmarks to use WebApplicationFactory and registered mock HttpMessageHandlers via HttpClientFactory for consistent handler injection.
  • Fixed compilation issues and runtime startup ordering (ensuring TestServerDefaults runs before per-fixture mock registrations so mocks are not overwritten).
  • Minor style/analyzer warnings remain (XML docs/usings/ordering); these are low-risk and can be cleaned in a follow-up.

Fixes #36

Testing

  • The Unit & Intergration tests are passing.
  • I have added the necessary tests to cover my changes.

Terms

robearlam and others added 30 commits September 12, 2025 15:05
Copilot AI review requested due to automatic review settings September 19, 2025 08:21
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR migrates integration tests from the legacy TestServerBuilder pattern to the standard WebApplicationFactory<TestWebApplicationProgram> pattern for ASP.NET Core integration testing. The migration preserves middleware ordering and per-test service configuration while modernizing the test infrastructure.

  • Replaced TestServerBuilder/TestServer with WebApplicationFactory<TestWebApplicationProgram> across all integration tests and benchmarks
  • Preserved middleware and service ordering for each test fixture while updating configuration patterns
  • Migrated benchmarks to use the new factory pattern with proper dependency injection

Reviewed Changes

Copilot reviewed 47 out of 47 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
TestWebApplicationProgram.cs New test program entry point for WebApplicationFactory
TrackingProxyFixture.cs Migrated tracking proxy tests with preserved middleware ordering
TrackingFixture.cs Updated tracking tests to use factory pattern
AttributeBasedTrackingFixture.cs Converted attribute-based tracking tests
Various TagHelper fixtures Migrated all tag helper test fixtures to factory pattern
Various other test fixtures Updated remaining test fixtures including localization, multisite, and experience editor tests
Benchmark files Updated benchmark classes to use WebApplicationFactory pattern

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 47 out of 47 changed files in this pull request and generated 4 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

});

HttpClient client = _server.CreateClient();
HttpClient client = BuildTextFieldTagHelperWebApplicationFactory().CreateClient();
Copy link

Copilot AI Sep 22, 2025

Choose a reason for hiding this comment

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

The factory is being built and disposed on every test method call. Consider caching the factory instance or using it as a class fixture to avoid repeated initialization overhead.

Suggested change
HttpClient client = BuildTextFieldTagHelperWebApplicationFactory().CreateClient();
HttpClient client = factory.CreateClient();

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

@Krishanthaudayakumara Krishanthaudayakumara Sep 23, 2025

Choose a reason for hiding this comment

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

Made those changes here - https://github.com/Krishanthaudayakumara/ASP.NET-Core-SDK/pull/37/files
Should get reviewed before merge

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 47 out of 47 changed files in this pull request and generated 2 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines 67 to 82
ISitecoreLayoutClient layoutClientSub = Substitute.For<ISitecoreLayoutClient>();
SitecoreLayoutRequest sampleRequest = [];
SitecoreLayoutResponse sampleResponse = new(sampleRequest)
{
Content = new SitecoreLayoutResponseContent
{
Sitecore = new SitecoreData
{
Route = new Route
{
DatabaseName = "master"
}
},
ContextRawData = string.Empty
}
};
Copy link

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

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

[nitpick] This mock setup is duplicating logic that could be extracted to a helper method or constant. Consider creating a shared test data factory method to reduce duplication and improve maintainability.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Used the same approach TestServerBuilder used: register a simple mock ISitecoreLayoutClient singleton instead of constructing a sampleRequest/sampleResponse.- ExperienceEditorCustomRoutingFixture.cs - Krishanthaudayakumara@8779577

builder.ConfigureServices(services =>
{
services.AddRouting();
services.AddSitecoreLayoutService();
Copy link

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

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

Similar to the tracking benchmarks, the layout service registration is missing the connection to the 'mock' HttpClient. The layout service needs to be configured to use the mock handler via .AddHttpHandler() method chain.

Suggested change
services.AddSitecoreLayoutService();
services.AddSitecoreLayoutService().AddHttpHandler("mock");

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This won't work, but I implemented register a mock HttpMessageHandler and configure the Sitecore layout service like previous here - Krishanthaudayakumara@16b6bbc

…ISitecoreLayoutClient singleton instead of constructing a sampleRequest/sampleResponse.- ExperienceEditorCustomRoutingFixture.cs
- Use the same approach TestServerBuilder used: register a simple mock ISitecoreLayoutClient singleton instead of constructing a sampleRequest/sampleResponse.- ExperienceEditorCustomRoutingFixture.cs
- wire layout service to mock HttpClient in RenderingEngine benchmarks
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

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


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines 15 to 16
app.Run();

Copy link

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

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

The app.Run() call will block the thread indefinitely in test scenarios. Consider using app.Start() or removing this line since WebApplicationFactory handles application lifecycle management.

Suggested change
app.Run();

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Used app.Start(); - Krishanthaudayakumara@b585376

Comment on lines +73 to +74
services.AddRouting();
services.AddControllersWithViews();
Copy link

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

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

[nitpick] These service registrations are duplicated across multiple test fixtures. Consider extracting common service registration logic into a shared helper method to reduce code duplication.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess it’s fine to leave those two lines duplicated in fixtures when any of the following apply and better readability, and avoid further complexity.

});
});
});

Copy link

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

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

The variable startedServer is assigned but never used. This appears to be a pattern to force server startup - consider adding a comment explaining this purpose or removing if unnecessary.

Suggested change
// Accessing _factory.Server forces the TestServer to start. The variable is unused; this is intentional.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

@Krishanthaudayakumara Krishanthaudayakumara Sep 30, 2025

Choose a reason for hiding this comment

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

[Theory]
[AutoNSubstituteData(nameof(InvalidHttpMessageConfiguration))]
public async Task HttpMessageConfigurationError_Returns_SitecoreLayoutServiceMessageConfigurationException(TestServer server, MockHttpMessageHandler clientHandler)
public async Task HttpMessageConfigurationError_Returns_SitecoreLayoutServiceMessageConfigurationException(TestWebApplicationFactory<TestWebApplicationProgram> factory, MockHttpMessageHandler clientHandler)
Copy link

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

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

The method parameter factory is not used directly in the method body. The actual factory configuration happens inline within the method, making this parameter misleading.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's incorrect, the factory parameter is used, it’s the base factory the method calls WithWebHostBuilder on to create configuredFactory there -
WebApplicationFactory<TestWebApplicationProgram> configuredFactory = factory.WithWebHostBuilder(builder =>


builder.AddSitecoreRenderingEngine(options =>
services.AddSitecoreLayoutService();
services.AddHttpClient("mock").ConfigurePrimaryHttpMessageHandler(() => _mockClientHandler!);
Copy link

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

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

[nitpick] The HTTP client registration pattern is inconsistent with the integration tests which use AddHttpHandler. This inconsistency could lead to different behavior between tests and benchmarks.

Suggested change
services.AddHttpClient("mock").ConfigurePrimaryHttpMessageHandler(() => _mockClientHandler!);
services.AddHttpHandler(_mockClientHandler!);

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Used the same layout service builder pattern which used previously - Krishanthaudayakumara@5a6dd01

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

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


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.


_server = testHostBuilder.BuildServer(new Uri("http://localhost"));
// Accessing _factory.Server forces the TestServer to start. The variable is unused; this is intentional.
TestServer startedServer = _factory.Server;
Copy link

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

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

[nitpick] The comment explanation is helpful, but consider using a more standard approach like _ = _factory.Server; to clearly indicate the variable is intentionally unused.

Suggested change
TestServer startedServer = _factory.Server;
_ = _factory.Server;

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

@Krishanthaudayakumara Krishanthaudayakumara Sep 30, 2025

Choose a reason for hiding this comment

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

made it to use _ = _factory.Server; - Krishanthaudayakumara#40

Comment on lines +191 to +192
services.AddRouting();
services.AddControllersWithViews();
Copy link

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

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

[nitpick] These service registrations are repeated across many fixtures. Consider extracting common service registration patterns into a shared helper method to reduce duplication.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess it’s fine to leave those two lines duplicated in fixtures when any of the following apply and better readability, and avoid further complexity.

]
SiteInfo = new SiteInfo
{
Sitemap = new[] { _edgeSitemapUrl.ToString() }
Copy link

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

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

[nitpick] The array initialization syntax has been changed from collection expression [...] to array initialization new[] {...}. Consider using the more modern collection expression syntax for consistency.

Suggested change
Sitemap = new[] { _edgeSitemapUrl.ToString() }
Sitemap = [ _edgeSitemapUrl.ToString() ]

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

@Krishanthaudayakumara Krishanthaudayakumara Sep 30, 2025

Choose a reason for hiding this comment

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

made it to use this approach - Krishanthaudayakumara#40

namespace Sitecore.AspNetCore.SDK.RenderingEngine.Integration.Tests.Fixtures;

public class RequestHeadersValidationFixture : IDisposable
public class RequestHeadersValidationFixture(TestWebApplicationFactory<TestWebApplicationProgram> factory) : IClassFixture<TestWebApplicationFactory<TestWebApplicationProgram>>, IDisposable
Copy link

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

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

[nitpick] Using primary constructor syntax here creates inconsistency with other fixtures that use traditional constructors. Consider standardizing the constructor pattern across all fixtures.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

@Krishanthaudayakumara Krishanthaudayakumara Sep 30, 2025

Choose a reason for hiding this comment

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

Refactored it to use similar constructor pattern - Krishanthaudayakumara#40

};

[Theory]
[AutoNSubstituteData(nameof(InvalidHttpMessageConfiguration))]
Copy link

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

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

The test method parameter type has changed from TestServer to TestWebApplicationFactory<TestWebApplicationProgram> but still uses the old AutoNSubstituteData attribute. Verify that the test data generation is compatible with the new parameter type.

Suggested change
[AutoNSubstituteData(nameof(InvalidHttpMessageConfiguration))]
[AutoData]

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

AutoNSubstituteData(nameof(InvalidHttpMessageConfiguration)) injects the MockHttpMessageHandler and TestWebApplicationFactory (via the named fixture action).
The injected factory is used (factory.WithWebHostBuilder(...) -> configuredFactory), so the parameter is required.
No attribute or signature change needed.

- use _ = _factory.Server; to clearly indicate the variable is intentionally 
- using the more modern collection expression syntax for consistency in EdgeSitemapProxyFixture
- Refactor RequestHeadersValidationFixture to implement similar constructor pattern like other fixtures
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update Tests to follow updated mocking approach

2 participants