Add tests for pre-rendered html in Boilerplate.Tests (#9277) #9278
Conversation
WalkthroughThe changes in this pull request introduce new functionality to the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/PageTests/BlazorServerPreRenderingTests.cs (1)
20-28: Consider enhancing test coverage with additional assertions.While the current test provides a good foundation for verifying pre-rendered content, consider these improvements:
- Add assertions for HTML structure (e.g., verify content is within expected elements)
- Add negative test cases (e.g., verify unauthorized content is not pre-rendered)
- Add test documentation explaining the purpose and expectations
Here's a suggested enhancement:
[TestMethod] [AutoAuthenticate] +/// <summary> +/// Verifies that authenticated user information is correctly pre-rendered in the initial HTML, +/// ensuring server-side rendering works as expected before client-side hydration. +/// </summary> public async Task PreRenderedHtml() { await Page.GotoAsync($"view-source:{WebAppServerAddress}"); - await Assertions.Expect(Page.GetByText(TestData.DefaultTestEmail).First).ToBeVisibleAsync(); - await Assertions.Expect(Page.GetByText(TestData.DefaultTestFullName).First).ToBeVisibleAsync(); + // Verify user information is pre-rendered + var emailElement = Page.GetByText(TestData.DefaultTestEmail).First; + var nameElement = Page.GetByText(TestData.DefaultTestFullName).First; + + await Assertions.Expect(emailElement).ToBeVisibleAsync(); + await Assertions.Expect(nameElement).ToBeVisibleAsync(); + + // Verify content is within expected elements + await Assertions.Expect(Page.GetByRole("main") + .Filter(new() { HasText = TestData.DefaultTestEmail })) + .ToBeVisibleAsync(); + + // Verify sensitive information is not pre-rendered + await Assertions.Expect(Page.GetByText("password")) + .ToBeHiddenAsync(); }src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Extensions/PlaywrightHydrationExtensions.cs (2)
16-18: LGTM! Consider adding XML documentation.The implementation correctly mirrors the EnableHydrationCheck methods and provides a clean way to disable hydration checks.
Consider adding XML documentation to explain when and why these methods should be used:
+ /// <summary> + /// Disables hydration checking for the browser context by removing the route handler that modifies page titles. + /// Use this method to restore normal page behavior after testing hydration. + /// </summary> public static Task DisableHydrationCheck(this IBrowserContext context) => context.UnrouteAsync("**/*", ChangeTitleHandler); + /// <summary> + /// Disables hydration checking for a specific page by removing the route handler that modifies page titles. + /// Use this method to restore normal page behavior after testing hydration. + /// </summary> public static Task DisableHydrationCheck(this IPage page) => page.UnrouteAsync("**/*", ChangeTitleHandler);
Line range hint
1-43: Consider enhancing the hydration testing infrastructureThe current approach using title modification is clever and non-intrusive. Consider these enhancements to make the testing infrastructure more robust:
- Add a method to check if hydration is currently enabled
- Consider implementing a fluent interface for better test readability
- Add support for custom hydration markers beyond just title changes
Example of a fluent interface:
// Example usage: await using var hydrationCheck = await page .BeginHydrationCheck() .WithCustomMarker("data-hydrated") .StartAsync(); // Test pre-rendered content await hydrationCheck.WaitForHydrationAsync();Would you like me to create an issue to track these enhancements?
src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/PageTests/PageModels/HomePage.cs (2)
18-23: Add defensive programming checksThe method could be more robust with additional error handling:
public override async Task AssertOpen() { - var (displayName, culture) = CultureInfoManager.SupportedCultures.Single(x => x.Culture.Name == CultureInfoManager.DefaultCulture.Name); + var defaultCulture = CultureInfoManager.SupportedCultures + .FirstOrDefault(x => x.Culture.Name == CultureInfoManager.DefaultCulture.Name) + ?? throw new InvalidOperationException("Default culture not found in supported cultures"); + var (displayName, culture) = defaultCulture; var localizer = StringLocalizerFactory.Create<AppStrings>(culture.Name); await AssertLocalized(localizer, culture.Name, displayName); }
36-41: Consider using more maintainable selectorsThe current implementation mixes different selector strategies and uses a potentially fragile XPath selector.
Consider refactoring to use more maintainable selectors:
public async Task AssertCultureCombobox(string cultureName, string cultureDisplayName) { - var cultureDropdown = Page.GetByRole(AriaRole.Combobox).Locator($"//img[contains(@src, 'flags/{cultureName}')]"); + // Add data-testid attributes to elements and use them for selection + var cultureDropdown = Page.GetByTestId($"culture-selector-{cultureName}"); await Assertions.Expect(cultureDropdown).ToBeVisibleAsync(); await cultureDropdown.ClickAsync(); await Assertions.Expect(Page.Locator($"button[role='option']:has-text('{cultureDisplayName}')")).ToHaveAttributeAsync("aria-selected", "true"); }This would require adding appropriate data-testid attributes to the HTML elements, but would make the tests more resilient to UI changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Extensions/PlaywrightHydrationExtensions.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/PageTests/BlazorServerPreRenderingTests.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/PageTests/BlazorWebAssemblyPreRenderingTests.cs(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/PageTests/BlazorWebAssemblyTests.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/PageTests/LocalizationTests.cs(4 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/PageTests/PageModels/HomePage.cs(2 hunks)
🔇 Additional comments (14)
src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/PageTests/BlazorWebAssemblyPreRenderingTests.cs (3)
1-3: LGTM: Namespace and using directive are properly structured
The namespace hierarchy correctly reflects the test category, and the required using directive is in place.
17-21: Verify base class implementation and consider additional test scenarios
The class correctly inherits from the server pre-rendering tests and specifies WebAssembly mode. However, we should verify the base class implementation and consider additional test scenarios.
Consider adding specific WebAssembly-related test scenarios, such as:
- Initial loading state verification
- JavaScript initialization checks
- WebAssembly-specific resource loading
✅ Verification successful
Let me gather more information about the base class implementation since the previous search didn't show the complete picture.
Base class implementation is complete and sufficient
The implementation is correct and well-structured. The base PreRenderingTests class already includes the essential pre-rendering test that verifies the presence of user data in the pre-rendered HTML. The WebAssembly variant properly inherits this test while specifying its render mode, which is the correct approach since:
- The base test verifies the core pre-rendering functionality
- The test runs with
PreRenderEnabled = truefrom the base class - The WebAssembly mode is correctly specified via
BlazorRenderModeoverride
The suggestion for additional WebAssembly-specific scenarios was overly cautious, as the pre-rendering behavior should be consistent across render modes, which is exactly what this inheritance pattern ensures.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify base class implementation and existing test coverage
# Check base class implementation
echo "Checking base PreRenderingTests class..."
ast-grep --pattern 'class PreRenderingTests {
$$$
public $_ PreRenderedHtml($$$) {
$$$
}
$$$
}'
# Look for similar test patterns in the codebase
echo "Checking for similar test patterns..."
rg -A 5 "PreRenderedHtml|BlazorRenderMode" --type cs
Length of output: 5249
Script:
#!/bin/bash
# Let's check the base class implementation in BlazorServer.PreRendering.PreRenderingTests
echo "Checking BlazorServer PreRendering tests..."
fd PreRendering --type f --exec cat {}
# Check for any WebAssembly-specific test patterns
echo "Checking WebAssembly-specific test patterns..."
rg -A 5 "WebAssembly.*Test" --type cs
Length of output: 3200
Line range hint 5-15: Verify the base test classes implementation
The inheritance and pre-rendering configuration look correct. Let's verify the base classes to ensure proper test coverage.
✅ Verification successful
Let's gather more specific information about the base classes and their implementation.
Let's get one final piece of information to verify the base class implementations.
Base classes and pre-rendering configuration verified successfully
The inheritance chain and pre-rendering configuration are correctly implemented:
- Base classes exist in the BlazorWebAssembly namespace
- PreRenderEnabled property is properly overridden from PageTestBase (which defaults to false)
- The inheritance hierarchy follows the correct pattern: BlazorWebAssembly.IdentityPagesTests and BlazorWebAssembly.LocalizationTests inherit from BlazorServer counterparts, which in turn inherit from PageTestBase
- The pre-rendering configuration aligns with the test requirements, properly enabling pre-rendering in the derived classes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and implementation of base test classes
# Check for base test classes
echo "Checking base test classes..."
rg -l "class IdentityPagesTests|class LocalizationTests" --type cs
# Check for PreRenderEnabled property in base classes
echo "Checking PreRenderEnabled property implementation..."
ast-grep --pattern 'class $_ {
$$$
public $_ PreRenderEnabled $$$
$$$
}'
Length of output: 973
Script:
#!/bin/bash
# Check the content of base test classes
echo "Checking BlazorWebAssembly.IdentityPagesTests implementation..."
rg "class.*BlazorWebAssembly\.IdentityPagesTests" -B 2 -A 10 --type cs
echo "Checking BlazorWebAssembly.LocalizationTests implementation..."
rg "class.*BlazorWebAssembly\.LocalizationTests" -B 2 -A 10 --type cs
echo "Checking PreRenderEnabled property in base classes..."
rg "PreRenderEnabled" -B 2 -A 2 --type cs
Length of output: 8682
Script:
#!/bin/bash
# Check the content of base test classes in BlazorWebAssembly namespace
echo "Checking base class implementations..."
rg "namespace.*BlazorWebAssembly" -A 20 --type cs
# Also check for any abstract base classes
echo "Checking for abstract base classes..."
ast-grep --pattern 'abstract class $_ {
$$$
public $_ PreRenderEnabled $$$
$$$
}'
Length of output: 5521
src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/PageTests/BlazorServerPreRenderingTests.cs (2)
15-19: LGTM! Good separation of concerns.
The new test class follows the established pattern of other pre-rendering test classes in the file, with clear inheritance and pre-rendering configuration.
14-29: Implementation aligns well with PR objectives.
The added test class successfully addresses the issue of missing tests for pre-rendered HTML (#9277) by:
- Providing specific tests for pre-rendered content
- Verifying critical user information is correctly pre-rendered
- Following the established testing patterns in the codebase
Let's verify this is the only pre-rendering test file:
✅ Verification successful
Pre-rendering tests are properly structured for both Blazor Server and WebAssembly modes
The implementation is well-organized with separate test classes for both Blazor Server and WebAssembly modes:
- Server tests implement the base functionality in
BlazorServerPreRenderingTests.cs - WebAssembly tests inherit from Server tests and override the render mode appropriately
- Both implementations cover identity pages, localization, and general pre-rendering scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other pre-rendering test files
rg -l "PreRenderEnabled.*=>.*true" --type cs
Length of output: 241
Script:
#!/bin/bash
# Let's check the content of both pre-rendering test files to understand their relationship
echo "=== BlazorWebAssemblyPreRenderingTests.cs ==="
cat src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/PageTests/BlazorWebAssemblyPreRenderingTests.cs
echo -e "\n=== BlazorServerPreRenderingTests.cs ==="
cat src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/PageTests/BlazorServerPreRenderingTests.cs
Length of output: 1830
src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Extensions/PlaywrightHydrationExtensions.cs (1)
16-18: Verify complete cleanup of route handlers
While the implementation looks correct, we should ensure that UnrouteAsync properly cleans up all resources.
Let's verify if there are any other places in the codebase where route handlers need cleanup:
✅ Verification successful
Let me gather more context about the ChangeTitleHandler and route handling implementation.
Route handler cleanup is properly implemented
The implementation is correct and complete. The code shows a symmetrical pattern where:
- Each
EnableHydrationCheckhas a correspondingDisableHydrationCheckmethod - Both IBrowserContext and IPage implementations use the same pattern and handler
- The ChangeTitleHandler is properly unregistered using the same route pattern "**/*"
- No other uses of this specific handler were found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for route handler registrations that might need cleanup
rg -l "RouteAsync.*ChangeTitleHandler" | xargs -I {} rg -l "UnrouteAsync" {} || echo "No matching files found"
# Search for similar hydration-related patterns
ast-grep --pattern 'RouteAsync($_, $_)'
Length of output: 334
Script:
#!/bin/bash
# Get the content of the file to understand the complete implementation
cat src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Extensions/PlaywrightHydrationExtensions.cs
# Look for any other route handling patterns
rg -A 5 "RouteAsync|UnrouteAsync"
Length of output: 6269
src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/PageTests/PageModels/HomePage.cs (2)
2-2: LGTM: Required namespace for culture management
The added namespace is necessary for accessing culture-related services and properly scoped.
18-41: Verify coverage of pre-rendered HTML testing
While these changes improve localization testing, they don't explicitly verify pre-rendered HTML behavior as mentioned in the PR objectives (#9277).
src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/PageTests/LocalizationTests.cs (4)
20-20: LGTM! The additional assertion enhances test coverage.
The new assertion logically complements the existing localization check by verifying the culture selection UI state.
33-33: Well-structured test coverage across different culture-setting mechanisms.
The systematic addition of UI state verification across all test methods ensures consistent coverage of the culture selection feature, regardless of how the culture is set (dropdown, query string, or URL segment).
Also applies to: 45-45, 57-57
Line range hint 64-67: Verify comprehensive culture coverage.
While the dynamic culture data source is maintainable, we should ensure CultureInfoManager.SupportedCultures includes all required cultures for testing.
✅ Verification successful
Culture coverage is comprehensive and well-configured
The CultureInfoManager provides a well-defined set of supported cultures that are consistently used across the application:
- en-US (English US, default)
- en-GB (English UK)
- nl-NL (Dutch)
- fa-IR (Persian/Farsi)
The implementation shows proper integration:
- All cultures are properly configured in the request localization options
- Special handling exists for right-to-left scripts (fa-IR)
- The cultures are consistently used in UI components, routing, and API endpoints
- Test data dynamically uses all configured cultures
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check culture configurations across the codebase
# Search for culture-related configurations
rg -A 5 "SupportedCultures|DefaultCulture" --type cs
# Look for hardcoded culture strings that might need testing
rg -A 2 '"[a-z]{2}-[A-Z]{2}"' --type cs
Length of output: 16860
20-20: Verify the implementation of AssertCultureCombobox.
The new assertion method is used consistently across all tests. Let's verify its implementation to ensure robust UI state verification.
Also applies to: 33-33, 45-45, 57-57
✅ Verification successful
Implementation of AssertCultureCombobox is robust and well-designed
The method implementation in HomePage.cs follows good testing practices:
- Verifies visibility of the culture flag image with correct culture code
- Checks the selected state of the culture option in the dropdown
- Uses proper aria roles and selectors for accessibility
- Aligns with the application's multilingual infrastructure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of AssertCultureCombobox
# Search for the method definition
ast-grep --pattern 'AssertCultureCombobox($_, $_) {
$$$
}'
# Look for related UI element selectors
rg "AssertCultureCombobox|culture.*combobox|culture.*dropdown" --type cs
Length of output: 961
Script:
#!/bin/bash
# Let's check the implementation of AssertCultureCombobox in HomePage.cs
cat "src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/PageTests/PageModels/HomePage.cs"
# Also search for any culture-related selectors or elements
rg "Culture|culture" --type cs -A 3 -B 3
Length of output: 101705
src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/PageTests/BlazorWebAssemblyTests.cs (2)
24-27: Verify the intended test behavior after condition inversion.
The condition has been inverted from checking !MultilingualEnabled to checking MultilingualEnabled. This changes when the test is skipped - now it skips when multilingual is enabled rather than when it's disabled. Please confirm this aligns with the intended test behavior.
✅ Verification successful
The condition inversion in the test is correct and aligns with the codebase behavior
Based on the codebase analysis, the inversion of the condition in MultilingualDisabled test is correct because:
- The test is specifically designed to verify behavior when multilingual is disabled
- Throughout the codebase, the pattern consistently shows that when
MultilingualEnabledis true, multilingual-specific logic is executed - The test should skip when multilingual is enabled because it would interfere with testing the non-multilingual behavior
- The error message correctly guides developers to disable multilingual support via the configuration when needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other test methods that might be affected by this change in behavior
rg -A 5 "MultilingualEnabled" --type cs
Length of output: 15888
Line range hint 1-100: Verify alignment with PR objectives.
The PR objectives mention adding tests for pre-rendered HTML, but the changes in this file primarily focus on multilingual support testing. Could you clarify how these changes relate to testing pre-rendered HTML, or if additional changes are needed to fulfill the PR objectives?
src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/PageTests/BlazorWebAssemblyTests.cs
Show resolved
Hide resolved
Closes #9277
Summary by CodeRabbit
New Features
Bug Fixes