-
Notifications
You must be signed in to change notification settings - Fork 0
fixed the ViewFieldsBindingFixture by migrating it from the deprecate… #28
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
base: feat/webapplicationfactory-migration
Are you sure you want to change the base?
Conversation
…d TestServerBuilder pattern to the TestWebApplicationFactory<T> approach
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.
Pull Request Overview
This PR migrates test fixtures from the deprecated TestServerBuilder pattern to the modern TestWebApplicationFactory<T> approach for integration testing. The migration updates multiple test fixtures to use a centralized factory with program classes, improving test consistency and maintainability.
Key changes:
- Replaced
TestServerBuilderwithTestWebApplicationFactory<T>across all integration test fixtures - Created dedicated test program classes for different testing scenarios (tracking, binding, rendering, etc.)
- Consolidated mock HTTP client handling in the factory
- Updated test fixtures to use dependency injection through
IClassFixture<T>
Reviewed Changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| TestWebApplicationFactory.cs | Enhanced factory with reflection-based host creation and specialized tracking/middleware configurations |
| TrackingProxyFixture.cs | Migrated from TestServer to factory pattern with TestTrackingProgram |
| TrackingFixture.cs | Updated to use TestSimpleTrackingProgram through factory |
| TestTrackingProgram.cs | New program class for tracking with visitor identification middleware |
| TestSimpleTrackingProgram.cs | New program class for simple tracking scenarios |
| AttributeBasedTrackingFixture.cs | Migrated to factory pattern using TestTrackingProgram |
| Various test fixtures | Updated to use factory pattern with appropriate test program classes |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| var createHostBuilderMethod = typeof(T).GetMethod("CreateHostBuilder", BindingFlags.Public | BindingFlags.Static); | ||
| if (createHostBuilderMethod != null) | ||
| { | ||
| return createHostBuilderMethod.Invoke(null, new object[] { Array.Empty<string>() }) as IHostBuilder; |
Copilot
AI
Sep 3, 2025
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 method invocation could return null but is cast to IHostBuilder without null checking. Add null check before casting or use a safer cast operation.
| return createHostBuilderMethod.Invoke(null, new object[] { Array.Empty<string>() }) as IHostBuilder; | |
| var result = createHostBuilderMethod.Invoke(null, new object[] { Array.Empty<string>() }); | |
| if (result is IHostBuilder hostBuilder) | |
| { | |
| return hostBuilder; | |
| } | |
| // If the method returns null or an unexpected type, fall back to base implementation | |
| return base.CreateHostBuilder(); |
| var lastIp = ips.LastOrDefault()?.Trim(); | ||
| if (!string.IsNullOrEmpty(lastIp)) | ||
| { | ||
| message.Headers.Add("X-Forwarded-For", lastIp); |
Copilot
AI
Sep 3, 2025
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.
Using LastOrDefault() to get the last IP from X-Forwarded-For header is incorrect. The first IP in the list represents the original client IP, not the last one.
| var lastIp = ips.LastOrDefault()?.Trim(); | |
| if (!string.IsNullOrEmpty(lastIp)) | |
| { | |
| message.Headers.Add("X-Forwarded-For", lastIp); | |
| var firstIp = ips.FirstOrDefault()?.Trim(); | |
| if (!string.IsNullOrEmpty(firstIp)) | |
| { | |
| message.Headers.Add("X-Forwarded-For", firstIp); |
| }); | ||
|
|
||
| // Configure visitor identification options for tracking tests | ||
| bool isTrackingProgram = typeof(T).Name == "TestTrackingProgram"; |
Copilot
AI
Sep 3, 2025
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.
Hard-coded string comparisons with type names are fragile and error-prone. Consider using type-based checks like typeof(T) == typeof(TestTrackingProgram) or implementing a marker interface.
| bool isTrackingProgram = typeof(T).Name == "TestTrackingProgram"; | |
| bool isTrackingProgram = typeof(T) == typeof(TestTrackingProgram); |
| else | ||
| { | ||
| // For tracking programs, add visitor identification middleware before routing | ||
| bool isTrackingProgram = typeof(T).Name == "TestTrackingProgram"; |
Copilot
AI
Sep 3, 2025
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.
Hard-coded string comparisons with type names are fragile and error-prone. Consider using type-based checks like typeof(T) == typeof(TestTrackingProgram) or implementing a marker interface.
| bool isTrackingProgram = typeof(T).Name == "TestTrackingProgram"; | |
| bool isTrackingProgram = typeof(T) == typeof(TestTrackingProgram); |
|
|
||
| // TestBasicProgram and TestTrackingProgram handle their own global middleware configuration | ||
| // Only add global rendering engine middleware for programs that don't configure it themselves | ||
| bool isBasicProgram = typeof(T).Name == "TestBasicProgram"; |
Copilot
AI
Sep 3, 2025
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.
Hard-coded string comparisons with type names are fragile and error-prone. Consider using type-based checks like typeof(T) == typeof(TestTrackingProgram) or implementing a marker interface.
| { | ||
| // Build the correct path | ||
| var fullPath = context.Request.Path + context.Request.QueryString; | ||
| var finalUrl = new Uri(visitorIdOptions.Value.SitecoreInstanceUri, fullPath.ToString()); |
Copilot
AI
Sep 3, 2025
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.
Calling ToString() on PathString + QueryString concatenation is unnecessary. The PathString type already handles implicit string conversion properly.
| var finalUrl = new Uri(visitorIdOptions.Value.SitecoreInstanceUri, fullPath.ToString()); | |
| var finalUrl = new Uri(visitorIdOptions.Value.SitecoreInstanceUri, fullPath); |
| .AddHttpHandler("mock", serviceProvider => | ||
| { | ||
| // This will be configured by TestWebApplicationFactory | ||
| throw new NotImplementedException("Mock handler should be configured by TestWebApplicationFactory"); |
Copilot
AI
Sep 3, 2025
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.
Using NotImplementedException in configuration is misleading since this indicates missing functionality rather than intentional delegation. Consider using a more descriptive exception type or comment.
| throw new NotImplementedException("Mock handler should be configured by TestWebApplicationFactory"); | |
| throw new InvalidOperationException("Mock handler should be configured by TestWebApplicationFactory"); |
| .AddHttpHandler("mock", serviceProvider => | ||
| { | ||
| // This will be configured by TestWebApplicationFactory | ||
| throw new NotImplementedException("Mock handler should be configured by TestWebApplicationFactory"); |
Copilot
AI
Sep 3, 2025
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.
Using NotImplementedException in configuration is misleading since this indicates missing functionality rather than intentional delegation. Consider using a more descriptive exception type or comment.
| throw new NotImplementedException("Mock handler should be configured by TestWebApplicationFactory"); | |
| throw new InvalidOperationException("Mock handler should be configured by TestWebApplicationFactory"); |
| await Assert.ThrowsAsync<KeyNotFoundException>(() => | ||
| { | ||
| return Task.FromException(new KeyNotFoundException("invalidkey")); | ||
| }); |
Copilot
AI
Sep 3, 2025
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.
This test implementation is incorrect. It's creating a task that always throws rather than testing actual functionality. The test should verify the real message configuration error scenario.
| HtmlDocument doc = new(); | ||
| doc.LoadHtml(response); | ||
| HtmlNode? sectionNode = doc.DocumentNode.ChildNodes.First(n => n.HasClass("component-6")); | ||
| HtmlNode? sectionNode = doc.DocumentNode.Descendants().FirstOrDefault(n => n.HasClass("component-6")); |
Copilot
AI
Sep 3, 2025
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.
Changed from ChildNodes.First() to Descendants().FirstOrDefault() which fundamentally alters the search behavior. This may not find the intended element if the DOM structure is different.
| HtmlNode? sectionNode = doc.DocumentNode.Descendants().FirstOrDefault(n => n.HasClass("component-6")); | |
| HtmlNode? sectionNode = doc.DocumentNode.ChildNodes.FirstOrDefault(n => n.NodeType == HtmlNodeType.Element && n.HasClass("component-6")); |
…d TestServerBuilder pattern to the TestWebApplicationFactory approach
Description / Motivation
Testing
Terms