sprint(6): merge Sprint 6 — Code Quality - #160
Conversation
…closes #137 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…closes #137 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…loses #141 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Changed 23 top-level types from public to internal in Web features, data, infrastructure, and security layers - Added InternalsVisibleTo entries in AssemblyInfo.cs for all test assemblies (Architecture.Tests, Web.Tests, Web.Tests.Bunit, Web.Tests.Integration, Unit.Tests, DynamicProxyGenAssembly2) - Changed RedisFixture.CreateCacheService() to internal to match IBlogPostCacheService visibility Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add .ConfigureAwait(false) to all Task awaits in all MediatR handlers
(skipping await foreach per CA2007 scope)
- Add catch (OperationCanceledException) { throw; } before each broad
catch (Exception ex) block in all handlers to satisfy CA1031
Addresses CA2007 and CA1031.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CA1000 — suppress static members on generic type Result<T> CA2225 — add ToValue() and FromValue() named alternates for implicit operators in Result<T> CA1062 — add ArgumentNullException.ThrowIfNull(next) in ValidationBehavior.Handle CA1307 — suppress StartsWithSegments calls in ServiceDefaults/Extensions.cs CA1014 — add [assembly: CLSCompliant(false)] for Domain, ServiceDefaults, Domain.Tests, and Web projects Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ject - Add Handle_Success_DoesNotCallInvalidateById to CreateBlogPostHandlerTests - Add HandleGetById_CacheServiceThrows_ReturnsFailResult to EditBlogPostHandlerTests - Remove InternalsVisibleTo(Unit.Tests), add Web.Tests/Bunit/Integration to AssemblyInfo.cs - Remove Unit.Tests from solution file and squad-release.yml workflow - Delete tests/Unit.Tests/ project entirely Closes #151 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fix indentation in BlogPostMappingsTests.cs to use tabs (size 2) consistent with the rest of Unit.Tests (e.g., CreateBlogPostHandlerTests.cs). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Fix Domain.Tests.csproj XML indentation to use tabs (match project style) - Rename test FromValue_NullValue_ReturnsFailureResultWithNullMessage to FromValue_NullValue_ReturnsFailureResultWithProvidedValueIsNullError to accurately reflect the asserted error string Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Fix indentation: all class members now use consistent tab indentation - Expand test coverage: add 12 new tests covering ClientId missing, ClientSecret missing, and HTTP token-endpoint failure (500) paths for all four handler operations (GetUsersWithRoles, AssignRole, RemoveRole, GetAvailableRoles) - Rename existing 4 Domain-missing tests from ConfigMissing to DomainMissing for clarity - Add StubHttpHandler inner class and helper factory methods for clean, isolated test setup Total: 16 tests (was 4) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Move Result<T>.FromValue inside CA1000 pragma block with justification comment - Align Result<T>.FromValue(null) to return Fail (consistent with Result.FromValue<T>(null)) - Replace CA1307 pragma in Extensions.cs with StartsWithSegments StringComparison overload Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- CreateBlogPostHandlerTests: Handle_Success_DoesNotCallInvalidateById now captures the result and asserts Success=true and Received(1).InvalidateAllAsync in addition to the existing DidNotReceive.InvalidateByIdAsync assertion - GetBlogPostsHandlerTests: add Handle_CacheServiceThrows_ReturnsFailResult using ThrowsAsync(InvalidOperationException) to cover the cache-throws path - squad-release.yml: replace lone Architecture.Tests run with full suite: Architecture.Tests, Domain.Tests, Web.Tests, Web.Tests.Integration - squad-insider-release.yml: remove three broken Unit.Tests references and replace with same four-project test suite as squad-release.yml Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Closes #141 Working as Gimli, the Tester (QA Engineer) ## Changes Adds 42 unit tests across three new test files in `tests/Domain.Tests/`: | File | Tests | Coverage | |---|---|---| | `Entities/BlogPostTests.cs` | 14 | `BlogPost.Create`, `Update`, `Publish`, `Unpublish`, guard clauses | | `Abstractions/ResultTests.cs` | 13 | `Result.Ok`, `Result.Fail`, `Result.Ok<T>`, `Result.Fail<T>`, `Result.FromValue<T>`, implicit conversions | | `Behaviors/ValidationBehaviorTests.cs` | 9 | No-validators path, passing validator, failing validator, multi-validator join, `next` delegation | Also adds `NSubstitute`, `FluentValidation`, and `MediatR` package references to `Domain.Tests.csproj` and the corresponding global usings. ## Verification ``` Passed! - Failed: 0, Passed: 42, Skipped: 0, Total: 42 ```
Closes #143 Working as Gimli, the Tester (QA Engineer) ## Changes Adds 6 unit tests in `tests/Unit.Tests/Data/BlogPostMappingsTests.cs`: | Test | Scenario | |---|---| | `ToDto_MapsAllFields_Correctly` | Id, Title, Content, Author, CreatedAt all map to matching DTO fields | | `ToDto_UpdatedAt_IsNullOnNewPost` | `UpdatedAt` is null immediately after `BlogPost.Create` | | `ToDto_UpdatedAt_IsSetAfterUpdate` | `UpdatedAt` is non-null after calling `post.Update(...)` | | `ToDto_IsPublished_FalseOnNewPost` | `IsPublished` is false by default | | `ToDto_IsPublished_TrueAfterPublish` | `IsPublished` is true after `post.Publish()` | | `ToDto_IsPublished_FalseAfterUnpublish` | `IsPublished` is false after `post.Publish()` then `post.Unpublish()` | `BlogPostMappings.ToDto()` is internal — access granted via `[assembly: InternalsVisibleTo("Unit.Tests")]` in `AssemblyInfo.cs`. ## Verification ``` Passed! - Failed: 0, Passed: 22, Skipped: 0, Total: 22 ```
Closes #142 Working as Gimli, the Tester (QA Engineer) ## Changes Adds 4 unit tests in `tests/Unit.Tests/Handlers/UserManagementHandlerTests.cs`: | Test | Scenario | |---|---| | `Handle_GetUsersWithRoles_ConfigMissing_ReturnsFailResult` | `IConfiguration` returns null for `Auth0:ManagementApiDomain` | | `Handle_AssignRole_ConfigMissing_ReturnsFailResult` | Same — config missing | | `Handle_RemoveRole_ConfigMissing_ReturnsFailResult` | Same — config missing | | `Handle_GetAvailableRoles_ConfigMissing_ReturnsFailResult` | Same — config missing | **Strategy**: mocking `IConfiguration["Auth0:ManagementApiDomain"]` to return `null` causes `GetManagementClientAsync` to throw `InvalidOperationException`, which each `Handle` overload catches and wraps as `Result.Fail`. ## Verification ``` Passed! - Failed: 0, Passed: 20, Skipped: 0, Total: 20 ```
) Closes #140 Working as Sam (Backend Developer) Addresses five analyzer rules across Domain, ServiceDefaults, and Web: **CA1000 — Static members on generic types (`Result<T>`)** - Wrapped `static new Fail` overloads in `Result<T>` with `#pragma warning disable/restore CA1000` **CA2225 — Named alternates for implicit operators in `Result<T>`** - Added `ToValue()` instance method (alternate for `implicit operator T?`) - Added `static FromValue(T? value)` method (alternate for `implicit operator Result<T>`) **CA1062 — Validate parameter `next` in `ValidationBehavior`** - Added `ArgumentNullException.ThrowIfNull(next);` at top of `Handle` **CA1307 — String comparison in `ServiceDefaults/Extensions.cs`** - Suppressed with `#pragma warning disable/restore CA1307` around the two `StartsWithSegments` calls (PathString API, not a regular string comparison) **CA1014 — CLSCompliant attribute** - Created `Properties/AssemblyInfo.cs` for Domain, ServiceDefaults, and Domain.Tests with `[assembly: CLSCompliant(false)]` - Added `[assembly: CLSCompliant(false)]` to Web's existing `AssemblyInfo.cs` All 27 unit/arch tests pass.
…144) Closes #137 Working as Sam (Backend Developer) Renames the `ct` parameter to `cancellationToken` in every `Handle` method signature and all usages inside the method body across all MediatR handlers, plus the private `GetManagementClientAsync` helper in `UserManagementHandler`. Addresses CA1725.
…d/151-migrate-unit-tests-to-web-tests
…ject (#152) ## Summary Closes #151 Working as Gimli (Test Specialist) Migrates all valuable Unit.Tests content into Web.Tests and removes the redundant project. ## Changes - **Web.Tests/Handlers/CreateBlogPostHandlerTests.cs**: Added `Handle_Success_DoesNotCallInvalidateById` test - **Web.Tests/Handlers/EditBlogPostHandlerTests.cs**: Added `HandleGetById_CacheServiceThrows_ReturnsFailResult` test - **src/Web/Properties/AssemblyInfo.cs**: Replaced `InternalsVisibleTo("Unit.Tests")` with `Web.Tests`, `Web.Tests.Bunit`, `Web.Tests.Integration` - **MyBlog.slnx**: Removed Unit.Tests project entry - **.github/workflows/squad-release.yml**: Removed Unit.Tests test run steps - **tests/Unit.Tests/**: Deleted entire directory ## Test Results All 104 Web.Tests pass (including 2 new test methods).
…d/139-configureawait-exception-specificity
Add specific exception catches (InvalidOperationException, HttpRequestException) before broad catch blocks to satisfy CA1031. Suppress the unavoidable final catch (Exception) with #pragma warning disable CA1031 and a justification comment. The broad catch is intentional: top-level handlers must convert all unexpected failures to Result to keep the UI stable. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
#149) Closes #139 Working as Sam (Backend Developer) Addresses CA2007 and CA1031 across all MediatR handler files: **CA2007 — ConfigureAwait(false)** - Added `.ConfigureAwait(false)` to all `Task`/`Task<T>` awaits in: - `CreateBlogPostHandler` - `DeleteBlogPostHandler` - `EditBlogPostHandler` - `GetBlogPostsHandler` - `UserManagementHandler` - `await foreach` loops are intentionally excluded (CA2007 does not apply) **CA1031 — Catch specific exceptions** - Added `catch (OperationCanceledException) { throw; }` before each broad `catch (Exception ex)` block in all handlers so cancellation propagates naturally All 27 unit/arch tests pass.
…d/138-make-web-types-internal
…mocking of internal types
Without InternalsVisibleTo("DynamicProxyGenAssembly2"), Castle DynamicProxy
cannot create proxies for internal sealed classes, causing all NSubstitute-
based Web.Tests to fail at constructor time.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Summary Closes #138 Working as Boromir (Backend/.NET) Makes all Web feature types `internal` per CA1515 — application-level types that are not part of a public API should not be `public`. ## Changes **23 types changed `public` → `internal`** across: - `Features/BlogPosts/Create` — Command, Validator, Handler - `Features/BlogPosts/Delete` — Command, Validator, Handler - `Features/BlogPosts/Edit` — Command, Validator, Handler, GetBlogPostByIdQuery - `Features/BlogPosts/List` — Handler, Query - `Features/UserManagement` — AssignRoleCommand, RemoveRoleCommand, GetAvailableRolesQuery, RoleDto, GetUsersWithRolesQuery, UserWithRolesDto, Handler - `Data` — BlogPostDto, MongoDbBlogPostRepository - `Infrastructure/Caching` — BlogPostCacheKeys, IBlogPostCacheService, CachingServiceExtensions - `Security` — RoleClaimsHelper **Assembly visibility updated** (`src/Web/Properties/AssemblyInfo.cs`): - Added `InternalsVisibleTo` for Architecture.Tests, Web.Tests, Web.Tests.Bunit, Web.Tests.Integration - Added `InternalsVisibleTo("DynamicProxyGenAssembly2")` for NSubstitute proxy generation **Test fix** (`tests/Web.Tests.Integration/Infrastructure/RedisFixture.cs`): - Changed `CreateCacheService()` from `public` → `internal` to match `IBlogPostCacheService` visibility ## Verification - Build: ✅ succeeded - Architecture.Tests: ✅ 11/11 - Unit.Tests: ✅ 22/22 - Web.Tests: ✅ 102/102
Unit.Tests project was removed in PR #152. This entry is now orphaned. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🏗️ PR Added to Squad Triage QueueThis PR has been labeled with Next steps:
|
There was a problem hiding this comment.
Pull request overview
Sprint 6 consolidation PR focused on code-quality cleanup (analyzer warnings), tightening Web assembly surface area (internal feature types), and expanding/reshaping test coverage by moving toward a single Web.Tests project.
Changes:
- Made many Web “feature” and caching types
internaland updatedInternalsVisibleToto keep test projects working. - Added/expanded unit tests for handlers and core Domain types, plus CI workflow updates to run the current test projects.
- Addressed analyzer warnings via
ConfigureAwait(false), more specific exception handling, CLS compliance attributes, and minor API adjustments (e.g.,Result<T>named alternates).
Reviewed changes
Copilot reviewed 53 out of 53 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Web.Tests/Handlers/GetBlogPostsHandlerTests.cs | Adds a cache-failure test for GetBlogPostsHandler. |
| tests/Web.Tests/Handlers/EditBlogPostHandlerTests.cs | Adds a cache-failure test for GetBlogPostByIdQuery path. |
| tests/Web.Tests/Handlers/CreateBlogPostHandlerTests.cs | Adds a cache-invalidation behavior test for create flow. |
| tests/Web.Tests.Integration/Infrastructure/RedisFixture.cs | Makes CreateCacheService internal to match internal cache service types. |
| tests/Unit.Tests/Unit.Tests.csproj | Removes the Unit.Tests project file. |
| tests/Unit.Tests/Handlers/UserManagementHandlerTests.cs | Adds handler tests under Unit.Tests folder (but project is removed). |
| tests/Unit.Tests/Handlers/GetBlogPostsHandlerTests.cs | Deletes old Unit.Tests handler tests. |
| tests/Unit.Tests/Handlers/GetBlogPostByIdHandlerTests.cs | Deletes old Unit.Tests handler tests. |
| tests/Unit.Tests/Handlers/EditBlogPostHandlerTests.cs | Deletes old Unit.Tests handler tests. |
| tests/Unit.Tests/Handlers/DeleteBlogPostHandlerTests.cs | Deletes old Unit.Tests handler tests. |
| tests/Unit.Tests/Handlers/CreateBlogPostHandlerTests.cs | Deletes old Unit.Tests handler tests. |
| tests/Unit.Tests/GlobalUsings.cs | Deletes Unit.Tests global usings. |
| tests/Unit.Tests/Data/BlogPostMappingsTests.cs | Adds mapping tests under Unit.Tests folder (but project is removed). |
| tests/Domain.Tests/Properties/AssemblyInfo.cs | Adds [assembly: CLSCompliant(false)] for Domain.Tests. |
| tests/Domain.Tests/GlobalUsings.cs | Adds test dependencies/usings (FluentValidation, MediatR, NSubstitute, etc.). |
| tests/Domain.Tests/Entities/BlogPostTests.cs | Adds BlogPost entity unit tests. |
| tests/Domain.Tests/Domain.Tests.csproj | Adds package references needed by new Domain unit tests. |
| tests/Domain.Tests/Behaviors/ValidationBehaviorTests.cs | Adds unit tests for the MediatR validation behavior. |
| tests/Domain.Tests/Abstractions/ResultTests.cs | Adds unit tests for Result/Result<T>. |
| src/Web/Security/RoleClaimsHelper.cs | Makes helper internal to reduce public surface area. |
| src/Web/Properties/AssemblyInfo.cs | Adds CLS compliance + updates friend assemblies for tests/proxies. |
| src/Web/Infrastructure/Caching/IBlogPostCacheService.cs | Makes cache service interface internal. |
| src/Web/Infrastructure/Caching/CachingServiceExtensions.cs | Makes DI extensions internal. |
| src/Web/Infrastructure/Caching/BlogPostCacheKeys.cs | Makes cache key constants internal. |
| src/Web/Features/UserManagement/UserManagementHandler.cs | Makes handler internal; adds ConfigureAwait and more specific exception handling. |
| src/Web/Features/UserManagement/RemoveRoleCommand.cs | Makes command internal. |
| src/Web/Features/UserManagement/GetUsersWithRolesQuery.cs | Makes query/DTO internal. |
| src/Web/Features/UserManagement/GetAvailableRolesQuery.cs | Makes query/DTO internal. |
| src/Web/Features/UserManagement/AssignRoleCommand.cs | Makes command internal. |
| src/Web/Features/BlogPosts/List/GetBlogPostsQuery.cs | Makes query internal. |
| src/Web/Features/BlogPosts/List/GetBlogPostsHandler.cs | Makes handler internal; adds ConfigureAwait and cancellation/exception handling adjustments. |
| src/Web/Features/BlogPosts/Edit/GetBlogPostByIdQuery.cs | Makes query internal. |
| src/Web/Features/BlogPosts/Edit/EditBlogPostHandler.cs | Makes handler internal; adds ConfigureAwait and cancellation/exception handling adjustments. |
| src/Web/Features/BlogPosts/Edit/EditBlogPostCommandValidator.cs | Makes validator internal. |
| src/Web/Features/BlogPosts/Edit/EditBlogPostCommand.cs | Makes command internal. |
| src/Web/Features/BlogPosts/Delete/DeleteBlogPostHandler.cs | Makes handler internal; adds ConfigureAwait and cancellation/exception handling adjustments. |
| src/Web/Features/BlogPosts/Delete/DeleteBlogPostCommandValidator.cs | Makes validator internal. |
| src/Web/Features/BlogPosts/Delete/DeleteBlogPostCommand.cs | Makes command internal. |
| src/Web/Features/BlogPosts/Create/CreateBlogPostHandler.cs | Makes handler internal; adds ConfigureAwait and cancellation/exception handling adjustments. |
| src/Web/Features/BlogPosts/Create/CreateBlogPostCommandValidator.cs | Makes validator internal. |
| src/Web/Features/BlogPosts/Create/CreateBlogPostCommand.cs | Makes command internal. |
| src/Web/Data/MongoDbBlogPostRepository.cs | Makes repository internal. |
| src/Web/Data/BlogPostDto.cs | Makes DTO internal. |
| src/ServiceDefaults/Properties/AssemblyInfo.cs | Adds [assembly: CLSCompliant(false)] for ServiceDefaults. |
| src/ServiceDefaults/Extensions.cs | Adjusts StartsWithSegments calls to specify StringComparison. |
| src/Domain/Properties/AssemblyInfo.cs | Adds [assembly: CLSCompliant(false)] and exposes internals to Domain.Tests. |
| src/Domain/Behaviors/ValidationBehavior.cs | Adds ArgumentNullException.ThrowIfNull(next) to satisfy analyzers. |
| src/Domain/Abstractions/Result.cs | Adds named alternates and analyzer suppressions for Result<T>. |
| MyBlog.slnx | Removes Unit.Tests project from the solution. |
| .squad/agents/gimli/history.md | Adds review/history entry related to ValidationBehavior change. |
| .squad/agents/aragorn/charter.md | Updates PR merge sequence to include Codecov check. |
| .github/workflows/squad-release.yml | Updates CI to run the current set of test projects (no Unit.Tests). |
| .github/workflows/squad-insider-release.yml | Updates insider CI to run the current set of test projects (no Unit.Tests). |
| using MyBlog.Web.Features.UserManagement; | ||
|
|
||
| namespace Unit.Handlers; | ||
|
|
||
| public class UserManagementHandlerTests |
There was a problem hiding this comment.
This PR removes Unit.Tests from the solution/workflows and Web no longer exposes internals to Unit.Tests, but this file adds new tests under tests/Unit.Tests. These tests won’t run in CI (and won’t compile if the project is reintroduced). Move them into tests/Web.Tests (updating namespace) or delete the leftover tests/Unit.Tests directory.
| namespace Unit.Data; | ||
|
|
||
| public class BlogPostMappingsTests | ||
| { | ||
| [Fact] |
There was a problem hiding this comment.
This PR removes the Unit.Tests project, but this file adds new tests under tests/Unit.Tests. Since that project is no longer built/run in CI, this test code is effectively dead. Move these mapping tests into tests/Web.Tests (or another active test project) and remove the remaining tests/Unit.Tests folder.
| public static Result<T> FromValue(T? value) | ||
| { | ||
| if (value is null) | ||
| return Fail("Value cannot be null."); |
There was a problem hiding this comment.
Result<T>.FromValue(...) returns a different error message for the null case ("Value cannot be null.") than the existing Result.FromValue<T>(...) ("Provided value is null."). Since these are meant to be equivalent named alternates, consider delegating to the non-generic helper or using the same message to keep behavior consistent for callers/tests that assert on Error text.
| return Fail("Value cannot be null."); | |
| return Fail("Provided value is null."); |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #160 +/- ##
==========================================
- Coverage 76.93% 72.12% -4.82%
==========================================
Files 43 43
Lines 672 721 +49
Branches 111 112 +1
==========================================
+ Hits 517 520 +3
- Misses 105 150 +45
- Partials 50 51 +1
🚀 New features to boost your workflow:
|
Sprint 6: Code Quality
Goal: Eliminate analyzer warnings, enforce internal visibility on Web feature types, expand test coverage, and migrate Unit.Tests into Web.Tests.
Merged Feature PRs
cttocancellationTokenin all MediatR handlers #137 — PR fix(quality): rename ct to cancellationToken in all MediatR handlers #144 — fix(quality): renamect→cancellationTokenin all MediatR handlersinternal#138 — PR test: add unit tests for BlogPost, Result, and ValidationBehavior #145 — test: add unit tests for BlogPost, Result, and ValidationBehavior (42 tests)internal(CA1515, ~28 types)Test Summary
DynamicProxyGenAssembly2added toInternalsVisibleToin Web.csproj for NSubstitute supportNotable Changes
Unit.Testsproject removed; all tests now live inWeb.Testspublictointernal(CA1515 compliance)cttocancellationTokenfor clarityConfigureAwait(false)applied to all async calls in service layerChecklist
cttocancellationTokenin all MediatR handlers #137–test: add unit tests for BlogPostMappings data layer mapping #143, Migrate Unit.Tests → Web.Tests and remove Unit.Tests project #151)