Add several automated test to bit Boilerplate (#12800) - #12801
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe boilerplate template adds automated coverage for template configuration, attachments, error responses, force-update headers, identity authorization, token separation, WebAuthn ownership, JSON options, tenant names, and query-string handling. It also updates test exclusions and assertion syntax. ChangesBoilerplate coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #12800 by adding a set of unit and integration tests to the Bit.Boilerplate template, covering several previously unpinned correctness/security contracts (URL query-string handling, tenant-reservation rules, JSON options consistency, token separation, role-claim allow-listing, attachment replacement, and ProblemDetails wire format). It also updates the template configuration so these tests are correctly included/excluded based on template options.
Changes:
- Added new unit/integration tests for URL parsing/encoding, reserved tenant naming/domain rules, JSON option consistency, token-class separation, role-claim allow-list enforcement, WebAuthn credential ownership, attachment re-upload correctness, and ProblemDetails wire contract.
- Added template self-consistency tests to detect stale/incorrect
.template.config/template.jsonsymbols and paths. - Updated existing tests to use newer MSTest assertion helpers and updated
.template.config/template.jsonto include/exclude the new test files appropriately.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Urls/AppQueryStringCollectionTests.cs | Adds unit coverage for repeated keys and single-encode/decode round-tripping behavior. |
| src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Tenants/ReservedTenantNameTests.cs | Adds unit coverage to prevent reserved-name/domain takeover scenarios in multitenant setups. |
| src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/TemplateConfig/TemplateConfigurationTests.cs | Adds unit tests ensuring template conditionals reference declared symbols and template rules reference existing paths. |
| src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Json/JsonOptionsContractTests.cs | Pins JSON serializer option invariants (dictionary key policy + camelCase contract) across option “shapes”. |
| src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/WebAuthnCredentialOwnershipTests.cs | Adds integration tests to ensure credential deletion is scoped to the credential owner. |
| src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/TokenClassSeparationTests.cs | Adds integration coverage ensuring access/refresh tokens are not interchangeable across endpoints. |
| src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/RoleClaimGrantAllowListTests.cs | Adds integration tests to enforce an allow-list for role-claim grant/update endpoints. |
| src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Attachments/AttachmentReplacementTests.cs | Adds integration tests for safe profile-picture replacement and validation-failure non-mutation behavior. |
| src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/ErrorHandling/ProblemDetailsWireContractTests.cs | Adds integration tests pinning ProblemDetails JSON shape and preventing duplicate top-level members. |
| src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/ForceUpdate/ForceUpdateHeaderHardeningTests.cs | Adds (currently ignored) integration test cases documenting ForceUpdate header parsing hardening requirements. |
| src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/TestAccountUtils.cs | Minor test assertion modernization. |
| src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/RoleAdministrationGuardTests.cs | Minor test assertion modernization. |
| src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/IdentityEmailDeliveryTests.cs | Minor test assertion modernization. |
| src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/EmailUniquenessIndexTests.cs | Minor test assertion modernization. |
| src/Templates/Boilerplate/Bit.Boilerplate/.template.config/template.json | Updates template exclusion rules to account for the newly added test files. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/Templates/Boilerplate/Bit.Boilerplate/.template.config/template.json (1)
567-567: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant exclusion entry.
"src/Tests/Features/Caching/ProductResponseCacheTests.cs"is already excluded unconditionally at Line 456 of this file, with a comment explaining that the file can never be included in any generated configuration ("module" is a single choice, so Admin and Sales cannot both be selected). Adding it again under(advancedTests != true)has no functional effect and looks like a leftover from copying an existing line.Remove the duplicate entry to keep the exclusion list accurate.
🧹 Remove the redundant entry
"exclude": [ "src/Tests/Infrastructure/Components/BitOtpInputUtils.cs", ... "src/Tests/Features/ForceUpdate/ForceUpdateUITests.cs", - "src/Tests/Features/Caching/ProductResponseCacheTests.cs", "src/Tests/Features/Identity/TokenClassSeparationTests.cs",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Templates/Boilerplate/Bit.Boilerplate/.template.config/template.json` at line 567, Remove the duplicate ProductResponseCacheTests.cs exclusion entry from the advancedTests conditional list in template.json, while preserving its existing unconditional exclusion.src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/TemplateConfig/TemplateConfigurationTests.cs (1)
108-159: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value"rename" is never actually validated by this test.
ruleNameincludes"rename"(Line 131), but Line 133 skips any rule whoseValueKindis notJsonValueKind.Array. Per dotnet templating's documentedrenamemodifier format,renameis an object where "Each key is a path to a file in the source, each value is a path to the target location," not an array. Any realrenamemodifier will therefore be silently skipped by this check, even though the method's own doc comment (Line 108-113) claims to validate "every exclusion / copy-only rule that names a concrete path," implyingrenamesources should be checked too.There is no
renamemodifier in the currently visibletemplate.json, so this gap has no effect today. If one is added later, this test gives false confidence that its path is validated.♻️ Handle the object-shaped `rename` modifier separately
- foreach (var ruleName in (string[])["exclude", "copyOnly", "include", "rename"]) + foreach (var ruleName in (string[])["exclude", "copyOnly", "include"]) { if (modifier.TryGetProperty(ruleName, out var rule) is false || rule.ValueKind is not JsonValueKind.Array) continue; foreach (var pathElement in rule.EnumerateArray()) { var path = pathElement.GetString(); if (string.IsNullOrWhiteSpace(path)) continue; pathsChecked++; if (ResolvesToSomething(templateRoot, path) is false) missing.Add($"{condition} / {ruleName}: '{path}'"); } } + + if (modifier.TryGetProperty("rename", out var rename) && rename.ValueKind is JsonValueKind.Object) + { + foreach (var renameEntry in rename.EnumerateObject()) + { + var sourcePath = renameEntry.Name; + + if (string.IsNullOrWhiteSpace(sourcePath)) + continue; + + pathsChecked++; + + if (ResolvesToSomething(templateRoot, sourcePath) is false) + missing.Add($"{condition} / rename: '{sourcePath}'"); + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/TemplateConfig/TemplateConfigurationTests.cs` around lines 108 - 159, Update EveryConcretePathInATemplateRule_Should_StillExist to handle rename modifiers as JSON objects rather than skipping them as non-arrays. Iterate each rename object's source-path keys, validate them with ResolvesToSomething, and report missing entries using the existing condition/ruleName context while preserving array handling for exclude, copyOnly, and include.src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/ForceUpdate/ForceUpdateHeaderHardeningTests.cs (1)
6-39: 🩺 Stability & Availability | 🔵 TrivialOffer to fix the pinned
ForceUpdateMiddlewaredefect (BP-079).The doc comment already specifies the fix: read
X-App-PlatformwithFirstOrDefault()instead ofSingle(), useEnum.TryParseinstead ofEnum.Parse, and useVersion.TryParseinstead ofVersion.Parse, then skip the version check when the pair is unusable. Do you want me to implement this fix inForceUpdateMiddlewareand remove the[Ignore]attribute from this test?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/ForceUpdate/ForceUpdateHeaderHardeningTests.cs` around lines 6 - 39, Implement the BP-079 fix in ForceUpdateMiddleware: read X-App-Platform with FirstOrDefault, replace Enum.Parse and Version.Parse with TryParse, and skip the update check when either header is missing or invalid. After the middleware no longer throws for these inputs, remove the [Ignore] attribute from ForceUpdateHeaderHardeningTests while preserving its existing test cases.src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Attachments/AttachmentReplacementTests.cs (1)
58-59: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
Assertsequence assertions for byte-arrays.The template uses
MSTest/Microsoft.Playwright.MSTest.v4, whereAssert.AreSequenceEqualandAssert.AreNotSequenceEqualare the recommended sequence assertions. ReplaceCollectionAssert.AreEqualat line 99 andCollectionAssert.AreNotEqualat line 58 with thoseAssertmethods to align with current guidance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Attachments/AttachmentReplacementTests.cs` around lines 58 - 59, Replace the byte-array sequence comparisons in AttachmentReplacementTests with MSTest Assert.AreSequenceEqual and Assert.AreNotSequenceEqual, including the comparison around Download(httpClient, userId). Remove the corresponding CollectionAssert usages while preserving the existing expected values, actual values, and assertion messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/Templates/Boilerplate/Bit.Boilerplate/.template.config/template.json`:
- Line 567: Remove the duplicate ProductResponseCacheTests.cs exclusion entry
from the advancedTests conditional list in template.json, while preserving its
existing unconditional exclusion.
In
`@src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Attachments/AttachmentReplacementTests.cs`:
- Around line 58-59: Replace the byte-array sequence comparisons in
AttachmentReplacementTests with MSTest Assert.AreSequenceEqual and
Assert.AreNotSequenceEqual, including the comparison around Download(httpClient,
userId). Remove the corresponding CollectionAssert usages while preserving the
existing expected values, actual values, and assertion messages.
In
`@src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/ForceUpdate/ForceUpdateHeaderHardeningTests.cs`:
- Around line 6-39: Implement the BP-079 fix in ForceUpdateMiddleware: read
X-App-Platform with FirstOrDefault, replace Enum.Parse and Version.Parse with
TryParse, and skip the update check when either header is missing or invalid.
After the middleware no longer throws for these inputs, remove the [Ignore]
attribute from ForceUpdateHeaderHardeningTests while preserving its existing
test cases.
In
`@src/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/TemplateConfig/TemplateConfigurationTests.cs`:
- Around line 108-159: Update EveryConcretePathInATemplateRule_Should_StillExist
to handle rename modifiers as JSON objects rather than skipping them as
non-arrays. Iterate each rename object's source-path keys, validate them with
ResolvesToSomething, and report missing entries using the existing
condition/ruleName context while preserving array handling for exclude,
copyOnly, and include.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7561b121-a127-4302-a9cf-3b19bd590a8f
📒 Files selected for processing (15)
src/Templates/Boilerplate/Bit.Boilerplate/.template.config/template.jsonsrc/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Attachments/AttachmentReplacementTests.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/ErrorHandling/ProblemDetailsWireContractTests.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/ForceUpdate/ForceUpdateHeaderHardeningTests.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/EmailUniquenessIndexTests.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/IdentityEmailDeliveryTests.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/RoleAdministrationGuardTests.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/RoleClaimGrantAllowListTests.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/TestAccountUtils.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/TokenClassSeparationTests.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Identity/WebAuthnCredentialOwnershipTests.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Json/JsonOptionsContractTests.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/TemplateConfig/TemplateConfigurationTests.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Tenants/ReservedTenantNameTests.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Tests/Features/Urls/AppQueryStringCollectionTests.cs
closes #12800
Summary by CodeRabbit