Apply some test improvements in Boilerplate (#9259) #9260
Conversation
WalkthroughThe changes in this pull request enhance the functionality of video recording in the testing framework by introducing an optional parameter for specifying the test method name in 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: 0
🧹 Outside diff range and nitpick comments (5)
src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/IdentityPagesTests.cs (1)
52-52: LGTM! Consider adding XML documentation.The change to use
Context.FinalizeVideoRecording(TestContext)is a good improvement that aligns with the architectural changes inPageTestBase. This approach provides better encapsulation of video recording functionality.Consider adding XML documentation to explain the cleanup behavior:
+ /// <summary> + /// Performs cleanup after each test by finalizing video recording with test context information. + /// </summary> + /// <returns>A ValueTask representing the asynchronous cleanup operation.</returns> [TestCleanup] public async ValueTask Cleanup() => await Context.FinalizeVideoRecording(TestContext);src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Extensions/PlaywrightVideoRecordingExtensions.cs (3)
8-9: Enhance the documentation comment for clarity.While the comment explains the parameter's purpose, it would be more helpful to:
- Document the return type and behavior
- Provide an example usage
- Explain when one might want to explicitly pass the method name
Consider updating the comment to:
-//Pass full name of the test method to 'testMethodFullName' param or it will be inferred from the test context +/// <summary> +/// Finalizes video recording by closing the browser context and cleaning up recordings for passed tests. +/// </summary> +/// <param name="browserContext">The browser context containing the recording</param> +/// <param name="testContext">The test context providing test information</param> +/// <param name="testMethodFullName">Optional: The full test method name. If not provided, it will be inferred from the test context. +/// Useful when the test context's method name doesn't match the desired recording name.</param> +/// <returns>A task representing the asynchronous operation</returns>
14-17: Add error handling for directory operations.The directory deletion operation could fail due to various reasons (file locks, permissions, etc.). Consider adding try-catch block for better error handling.
var directory = GetVideoDirectory(testContext, testMethodFullName); -if (Directory.Exists(directory)) - Directory.Delete(directory, true); +if (Directory.Exists(directory)) +{ + try + { + Directory.Delete(directory, true); + } + catch (IOException ex) + { + testContext.WriteLine($"Warning: Failed to clean up video directory: {ex.Message}"); + } +}
Line range hint
29-34: LGTM! Consider extracting character replacement logic.The implementation is robust and uses modern C# features effectively. The path handling is done safely with proper character sanitization.
Consider extracting the character replacement logic to a separate method for better maintainability:
+private static string SanitizeFileName(string fileName) +{ + char[] notAllowedChars = [')', '"', '<', '>', '|', '*', '?', '\r', '\n', .. Path.GetInvalidFileNameChars()]; + return new string(fileName.Where(ch => !notAllowedChars.Contains(ch)) + .ToArray()) + .Replace('(', '_') + .Replace(',', '_'); +} private static string GetVideoDirectory(TestContext testContext, string? testMethodFullName = null) { testMethodFullName ??= $"{testContext.FullyQualifiedTestClassName}.{GetTestMethodName(testContext)}"; - char[] notAllowedChars = [')', '"', '<', '>', '|', '*', '?', '\r', '\n', .. Path.GetInvalidFileNameChars()]; - testMethodFullName = new string(testMethodFullName.Where(ch => !notAllowedChars.Contains(ch)).ToArray()).Replace('(', '_').Replace(',', '_'); + testMethodFullName = SanitizeFileName(testMethodFullName); var dir = Path.Combine(testContext.TestResultsDirectory!, "..", "..", "Videos", testMethodFullName); return Path.GetFullPath(dir); }src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/TestsInitializer.cs (1)
97-99: Consider implementing using statement for better resource disposal.While the current cleanup is functional, consider wrapping the context in a using statement to ensure deterministic disposal of resources.
Here's a suggested refactoring:
- var context = await playwrightPage.NewContextAsync(options); - await context.EnableBlazorWasmCaching(); - // ... rest of the code ... - await context.FinalizeVideoRecording(testContext, currentMethodFullName); - await context.Browser!.CloseAsync(); - await context.Browser!.DisposeAsync(); + await using var context = await playwrightPage.NewContextAsync(options); + await context.EnableBlazorWasmCaching(); + // ... rest of the code ... + await context.FinalizeVideoRecording(testContext, currentMethodFullName);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Extensions/PlaywrightVideoRecordingExtensions.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/IdentityPagesTests.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/PageTests/IdentityPagesTests.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/PageTests/PageModels/HomePage.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/PageTests/PageModels/Layout/IdentityLayout.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/PageTests/PageTestBase.cs(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/TestsInitializer.cs(1 hunks)
🔇 Additional comments (11)
src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/PageTests/PageModels/Layout/IdentityLayout.cs (1)
22-23: Improved element targeting strategy
The new locator strategy using .bit-crd.panel provides more precise targeting of the sign-out button within its container context. The addition of Exact = true for the dialog button also ensures exact text matching, reducing the risk of selecting an incorrect button.
Let's verify the consistency of this locator pattern across the codebase:
✅ Verification successful
Locator pattern is consistent across the codebase
The search results confirm that the new locator strategy is consistently implemented in both relevant locations where sign-out functionality is handled:
src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/PageTests/PageModels/HomePage.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Tests/PageTests/PageModels/Layout/IdentityLayout.cs
Both implementations use the identical pattern:
- First click:
.bit-crd.panelcontainer with button role and SignOut text - Second click: Dialog button with SignOut text and exact matching
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in sign-out button locator patterns
# Expected: Similar locator patterns in related test files
# Search for similar button locator patterns
rg -A 2 'GetByRole\(AriaRole\.Button.*SignOut'
# Search for usage of the .bit-crd.panel class with button roles
rg -A 2 '\.bit-crd\.panel.*GetByRole\(AriaRole\.Button'
Length of output: 4977
src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/IdentityPagesTests.cs (1)
52-52: Verify consistent usage across test classes
Let's ensure this pattern is consistently applied across other test classes.
✅ Verification successful
Let me check the implementation details of the video recording functionality to ensure everything is properly aligned.
Let me check the implementation in the PlaywrightVideoRecordingExtensions class since it was found in the first search but we haven't seen its contents yet.
Let me check the PageTestBase implementation since it's the base class that contains the other usage of FinalizeVideoRecording.
Video recording cleanup is consistently implemented
The implementation is consistent across the codebase:
- The
FinalizeVideoRecordingextension method is properly defined inPlaywrightVideoRecordingExtensions.cs - It's correctly used in both
IdentityPagesTests.csandPageTestBase.cscleanup methods - The implementation handles video cleanup based on test outcomes and uses proper directory management
- All test classes inheriting from
PageTestBaseautomatically get this cleanup behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for inconsistent video recording cleanup patterns across test classes
# Find test classes that might need similar updates
rg -l "FinalizeVideoRecording" --type cs
# Check for direct calls to FinalizeVideoRecording that might need updating
rg "this\.FinalizeVideoRecording\(\)" --type cs
# Check for updated pattern usage
rg "Context\.FinalizeVideoRecording\(TestContext\)" --type cs
Length of output: 781
Script:
#!/bin/bash
# Check the implementation of FinalizeVideoRecording and related methods
ast-grep --pattern 'public $_ FinalizeVideoRecording($_) {
$$$
}'
# Check for any other video recording cleanup patterns
rg -A 5 "Cleanup\(\)" --type cs
Length of output: 344
Script:
#!/bin/bash
# Check the content of PlaywrightVideoRecordingExtensions class
cat src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Extensions/PlaywrightVideoRecordingExtensions.cs
# Check the inheritance hierarchy of test classes
rg -B 2 "class.*Tests.*:" --type cs
Length of output: 16725
Script:
#!/bin/bash
# Check the content of PageTestBase
cat src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/PageTests/PageTestBase.cs
Length of output: 4908
src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/PageTests/PageModels/HomePage.cs (2)
47-48: LGTM! More specific element locators improve test reliability.
The changes make the element selection more precise:
- Using
.bit-crd.panelcontainer helps avoid ambiguity with other potential sign-out buttons - Adding
Exact = trueensures exact text match for the confirmation dialog button
45-45: Verify the base class implementation of SignOut.
The new keyword explicitly hides the base class implementation. Ensure this is intentional and that the base implementation in IdentityLayout is not needed for this specific page model.
✅ Verification successful
Based on the shell script results, I can now generate the final response:
The new keyword usage is correct and intentional
The base class IdentityLayout has a different return type (SignInPage) for its SignOut() method compared to the derived class's return type (MainHomePage). The new keyword is appropriately used here to indicate this intentional method hiding, as both implementations have different navigation flows:
- Base class (
IdentityLayout): Returns toSignInPage - Derived class (
HomePage): Returns toMainHomePage
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find the base class implementation of SignOut to verify the hiding is intentional
# Search for the SignOut method in IdentityLayout
ast-grep --pattern 'class IdentityLayout {
$$$
SignOut() {
$$$
}
$$$
}'
# Backup: Use ripgrep to find any SignOut method
rg -A 5 "SignOut\(" "src/Templates/Boilerplate/Bit.Boilerplate/src"
Length of output: 14020
src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Extensions/PlaywrightVideoRecordingExtensions.cs (1)
Line range hint 1-56: Verify path resolution and conditional compilation.
A few architectural considerations:
- The use of relative paths (
..) might be fragile in different test environments - The conditional compilation directive (
#if (advancedTests == true)) seems incomplete
Let's verify the usage and configuration:
Consider:
- Using configuration for video storage paths instead of hard-coded relative paths
- Documenting the purpose and configuration of the
advancedTestsconditional compilation symbol
src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/TestsInitializer.cs (2)
97-97: LGTM! Video recording finalization is correctly placed.
The video recording finalization is appropriately placed just before browser cleanup, ensuring that the authentication process is fully captured while maintaining proper resource management.
94-96: Verify authentication state replacement is consistent across tests.
The authentication state replacement logic assumes a specific format for the server address. Ensure this replacement pattern is consistently handled across all test scenarios.
✅ Verification successful
Let me check the actual implementation in both files to verify consistency.
Authentication state replacement is correctly and consistently implemented
The verification shows that the authentication state replacement is handled consistently across the codebase:
- In
TestsInitializer.cs: Server address is replaced with "[ServerAddress]" placeholder - In
PageTestBase.cs: The placeholder is correctly replaced back with the actual server address
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of authentication state manipulation
rg -l "AuthenticationState.*Replace.*ServerAddress" --type cs
Length of output: 213
Script:
#!/bin/bash
# Get the context of AuthenticationState replacement in both files
rg "AuthenticationState.*Replace.*ServerAddress" --type cs -B 2 -A 2
Length of output: 1304
src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/PageTests/PageTestBase.cs (2)
47-47: LGTM! Good refactoring of video recording finalization.
The change to use Context.FinalizeVideoRecording centralizes the video recording logic and maintains consistency across test classes. The cleanup sequence is also correct, ensuring video recording is finalized before test server disposal.
65-65: Verify the impact of Uri.ToString() on authentication.
The change from OriginalString to ToString() might affect authentication if the server address format is sensitive. While ToString() returns a canonicalized form, OriginalString preserves the exact string used to construct the Uri.
Let's verify the server address format usage:
✅ Verification successful
Based on the search results, I can now provide a final response since I have enough context about how the server address is used in authentication:
The change from OriginalString to ToString() is safe for authentication
The codebase shows that server addresses are consistently handled through Uri objects and their string representations across both authentication and general usage. Key findings:
-
The authentication state replacement is used in a two-way process:
- During storage:
AuthenticationState = state.Replace(testServer.WebAppServerAddress.OriginalString.TrimEnd('/'), "[ServerAddress]") - During restoration:
options.StorageState = TestsInitializer.AuthenticationState.Replace("[ServerAddress]", WebAppServerAddress.ToString())
- During storage:
-
The server address is used consistently with
ToString()across the codebase for all authentication-related URLs and API endpoints -
No special format requirements are enforced - both relative and absolute URLs are supported through proper Uri handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how the server address is used in authentication-related code
# Search for server address usage in authentication
rg -l "ServerAddress|WebAppServerAddress" | xargs rg -A 5 "ServerAddress|WebAppServerAddress"
# Check for any hardcoded URL formats in auth state
rg -A 5 "AuthenticationState.*Replace"
Length of output: 87515
src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/PageTests/IdentityPagesTests.cs (2)
Line range hint 1-324: Well-structured and comprehensive test coverage
The test class demonstrates excellent testing practices:
- Comprehensive coverage of identity flows
- Good use of DataRow for multiple test scenarios
- Consistent test structure and patterns
- Proper resource management with async/await
18-20: 🛠️ Refactor suggestion
Consider adding conditional assertions based on pre-render mode
While the status code difference between pre-render modes is noted, removing the assertion entirely might miss potential issues. Consider:
- Adding conditional assertions based on the pre-render mode
- Verifying both status codes (200 and 401) are valid in their respective modes
Consider this approach:
-//NOTE: Status code differs between pre-render Disabled (200) and Enabled(401)
-//Once it resolved we can uncomment this line
-//Assert.AreEqual(StatusCodes.Status200OK, response.Status);
+// Verify status code based on pre-render mode
+var isPreRenderEnabled = TestServer.WebApp.Services
+ .GetRequiredService<IConfiguration>()
+ .GetValue<bool>("PreRenderEnabled");
+
+Assert.IsTrue(
+ response.Status == StatusCodes.Status200OK && !isPreRenderEnabled ||
+ response.Status == StatusCodes.Status401Unauthorized && isPreRenderEnabled,
+ $"Unexpected status code {response.Status} for PreRenderEnabled={isPreRenderEnabled}"
+);✅ Verification successful
Let me gather more information about the test class and pre-rendering configuration.
Let me gather more information about the test class and pre-rendering configuration.
Let me search for more context about the pre-rendering configuration and test setup.
Let me check for any configuration settings related to pre-rendering and the test server setup.
Based on the gathered information, I can now provide a final response:
Consider using conditional assertions based on pre-render configuration
The status code behavior is correctly documented in the comment and is determined by the PrerenderEnabled setting in the application configuration. Instead of removing the assertion, we should verify the status code based on the current pre-render mode:
// Get pre-render setting from configuration
var isPrerenderEnabled = TestServer.WebApp.Services
.GetRequiredService<ClientWebSettings>()
.WebAppRender.PrerenderEnabled;
// Assert expected status code based on pre-render mode
Assert.AreEqual(
isPrerenderEnabled ? StatusCodes.Status401Unauthorized : StatusCodes.Status200OK,
response.Status,
$"Unexpected status code {response.Status} for PrerenderEnabled={isPrerenderEnabled}");This approach:
- Maintains test coverage for both pre-render modes
- Makes the test behavior explicit and self-documenting
- Validates the correct status code based on the current configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for pre-render configuration to determine the default mode
rg -l "UsePrerendering|AddPrerendering" --type cs
# Search for similar status code assertions in other test files
rg "Assert.*Status.*OK|response.Status" --type cs
Length of output: 855
Script:
#!/bin/bash
# Check the test class implementation for context
cat "src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/PageTests/IdentityPagesTests.cs"
# Look for pre-rendering configuration in Program.cs or Startup.cs
fd -e cs -E "**/bin/**" -E "**/obj/**" "Program|Startup" --exec cat {}
Length of output: 92961
Script:
#!/bin/bash
# Check the test class implementation for context around the assertion
rg -A 5 -B 5 "UnauthorizedUser_Should_RedirectToSignInPage" --type cs
# Look for pre-rendering configuration in startup/configuration files
rg -A 3 -B 3 "PreRenderEnabled|UsePrerendering|AddPrerendering" --type cs
Length of output: 2539
Script:
#!/bin/bash
# Look for pre-rendering configuration in appsettings or other config files
fd -e json -E "**/bin/**" -E "**/obj/**" . --exec grep -l "PreRenderEnabled"
# Check the base test class for any pre-rendering setup
fd -e cs -E "**/bin/**" -E "**/obj/**" "PageTestBase.cs" --exec cat {}
# Look for any other test files that might have similar assertions
rg "Status.*OK.*response.Status" --type cs
Length of output: 5172
Script:
#!/bin/bash
# Check AppTestServer implementation for pre-rendering configuration
fd -e cs -E "**/bin/**" -E "**/obj/**" "AppTestServer.cs" --exec cat {}
# Look for WebAppRender configuration in settings files
rg -A 5 "WebAppRender|BlazorMode|PrerenderEnabled" --type cs --type json
Length of output: 24754
msynk
changed the title
Closes #9259
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores