Merged
Conversation
Owner
zribktad
commented
Apr 4, 2026
zribktad
commented
- Added InternalsVisibleTo attribute in Webhooks.csproj to allow access for APITemplate.Tests.
- Refactored WebhookTestHelper to utilize HmacHelper for computing HMAC signatures.
- Removed redundant WebhookTestHelper from Integration tests and updated references in Unit tests.
- Improved validation logic in SseStreamRequestValidationTests by consolidating validation methods.
- Updated various test files to enhance structure and maintainability.
added 2 commits
April 4, 2026 14:42
- Added FluentValidation package version 12.1.1 to Directory.Packages.props for enhanced validation capabilities. - Included InternalsVisibleTo attribute in Notifications.csproj to facilitate testing. - Updated APITemplate.Tests.csproj with new package references for ErrorOr, FluentValidation, Microsoft.EntityFrameworkCore.InMemory, MockQueryable.Moq, and SystemTextJsonPatch. - Refactored various test files to improve structure, including updates to BackgroundJobsOptionsValidatorTests and InMemoryIdempotencyStoreTests for better validation logic and test coverage. - Enhanced HmacWebhookPayloadValidatorTests and OutgoingWebhookSsrfTests with additional validation scenarios and test data integration.
- Added InternalsVisibleTo attribute in Webhooks.csproj to allow access for APITemplate.Tests. - Refactored WebhookTestHelper to utilize HmacHelper for computing HMAC signatures. - Removed redundant WebhookTestHelper from Integration tests and updated references in Unit tests. - Improved validation logic in SseStreamRequestValidationTests by consolidating validation methods. - Updated various test files to enhance structure and maintainability.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly expands the unit test coverage across multiple modules, including Identity, Notifications, FileStorage, Reviews, and Chatting. It introduces shared theory data classes and helper factories to streamline test creation, while refactoring existing tests for better isolation and maintainability. One issue was identified in the EmailValueObjectTests where a null-coalescing operator masks a null test case, preventing the domain logic from being verified against actual null inputs.
… in SsrfTheoryData - Deleted the unused AssemblyFixtures class from the test project. - Updated the MapProhibitedIpv4 method in SsrfTheoryData to include a type constraint for better type safety.
There was a problem hiding this comment.
Pull request overview
This PR expands and refactors the test suite, adding shared theory data, introducing additional unit tests across multiple modules, and updating project/module configuration to support the new tests (including access to internal APIs where needed).
Changes:
- Added new unit tests for several modules (Identity, Reviews, Notifications, FileStorage, Chatting, ProductCatalog) and improved existing ones.
- Introduced shared test data helpers (theory data + helper factories) and refactored webhook HMAC test utilities to reuse module HMAC logic.
- Updated project configuration/package references and added
InternalsVisibleToto enable testing internal implementations.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/APITemplate.Tests/Unit/Webhooks/OutgoingWebhookSsrfTests.cs | Replaced inline SSRF IP cases with shared MemberData. |
| tests/APITemplate.Tests/Unit/Webhooks/HmacWebhookPayloadValidatorTests.cs | Updated imports/helpers and added new negative validation theory cases. |
| tests/APITemplate.Tests/Unit/Webhooks/HmacWebhookPayloadSignerTests.cs | Updated imports to module contracts/security. |
| tests/APITemplate.Tests/Unit/Validators/CreateProductRequestValidatorTests.cs | Switched negative price cases to shared theory data. |
| tests/APITemplate.Tests/Unit/TestData/SsrfTheoryData.cs | New shared SSRF IP address test inputs. |
| tests/APITemplate.Tests/Unit/TestData/PriceTheoryData.cs | New shared price amount theory data. |
| tests/APITemplate.Tests/Unit/TestData/EmailTheoryData.cs | New shared email inputs for value-object tests. |
| tests/APITemplate.Tests/Unit/StoredProcedures/StoredProcedureExecutorTests.cs | Simplified EF setup and switched to shared-kernel stored procedure executor. |
| tests/APITemplate.Tests/Unit/Reviews/RatingValueObjectTests.cs | New unit tests for Rating creation/persistence behavior. |
| tests/APITemplate.Tests/Unit/Reviews/ProductReviewFilterValidatorTests.cs | New validator tests for rating ranges and sorting fields/directions. |
| tests/APITemplate.Tests/Unit/ProductCatalog/ProductSoftDeleteCascadeRuleTests.cs | New unit tests for soft-delete cascade rule type handling. |
| tests/APITemplate.Tests/Unit/ProductCatalog/PriceValueObjectTests.cs | New unit tests for Price creation/zero/persistence behavior. |
| tests/APITemplate.Tests/Unit/Notifications/FluidEmailTemplateRendererTests.cs | New tests for template rendering and unknown template behavior. |
| tests/APITemplate.Tests/Unit/Notifications/FailedEmailErrorNormalizerTests.cs | New tests for error normalization/truncation logic. |
| tests/APITemplate.Tests/Unit/Logging/RedactionConfigurationTests.cs | Updated logging namespace dependency. |
| tests/APITemplate.Tests/Unit/Identity/TenantCodeValueObjectTests.cs | New tests for TenantCode validation and persistence factory. |
| tests/APITemplate.Tests/Unit/Identity/TenantClaimValidatorTests.cs | New tests for tenant claim validation using a claims principal factory. |
| tests/APITemplate.Tests/Unit/Identity/EmailValueObjectTests.cs | New tests for email validation/normalization and implicit conversion. |
| tests/APITemplate.Tests/Unit/Idempotency/InMemoryIdempotencyStoreTests.cs | Added additional acquisition scenarios and centralized store setup. |
| tests/APITemplate.Tests/Unit/Helpers/WebhookTestHelper.cs | New shared helper to compute HMAC signatures via module HmacHelper. |
| tests/APITemplate.Tests/Unit/Helpers/TestClaimsPrincipalFactory.cs | New helper to create principals with/without tenant claims. |
| tests/APITemplate.Tests/Unit/FileStorage/LocalFileStorageServiceTests.cs | New unit tests covering tenant scoping and traversal protection. |
| tests/APITemplate.Tests/Unit/Chatting/SseStreamRequestValidationTests.cs | New validation tests for SSE request data annotations. |
| tests/APITemplate.Tests/Unit/Cache/TenantAwareOutputCachePolicyTests.cs | Updated to set a tenant user principal in the output cache context. |
| tests/APITemplate.Tests/Unit/BackgroundJobs/BackgroundJobsOptionsValidatorTests.cs | Refactored/expanded options validation tests against new validator behavior. |
| tests/APITemplate.Tests/Integration/Helpers/WebhookTestHelper.cs | Removed redundant integration webhook HMAC helper. |
| tests/APITemplate.Tests/Integration/Features/WebhooksControllerTests.cs | Updated integration test to reuse the unit webhook signature helper. |
| tests/APITemplate.Tests/AssemblyFixtures.cs | Removed prior assembly-level integration fixture wiring. |
| tests/APITemplate.Tests/APITemplate.Tests.csproj | Added packages/references and removed compilation of legacy/integration test folders. |
| src/Modules/Webhooks/Webhooks.csproj | Added InternalsVisibleTo for test access to internal APIs. |
| src/Modules/Notifications/Notifications.csproj | Added InternalsVisibleTo for test access to internal APIs. |
| Directory.Packages.props | Added centralized version for FluentValidation. |
Comments suppressed due to low confidence (1)
tests/APITemplate.Tests/AssemblyFixtures.cs:4
- Removing the assembly-level
AssemblyFixture(previously used for the shared Postgres container) changes integration test initialization semantics. If integration tests are still intended to run from this project, the fixture removal will likely break or significantly slow tests; either restore the fixture or document/move the integration suite so this file accurately reflects the intended test setup.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+29
to
+33
| <!-- Legacy suite referenced removed APITemplate.Application / Infrastructure; migrate separately --> | ||
| <ItemGroup> | ||
| <Compile Include="Unit\Build\PackageReferencePolicySupport.cs"/> | ||
| <Compile Include="Unit\Build\PackageReferencePolicyTests.cs"/> | ||
| <Compile Include="Unit\Common\ConfigurationExtensionsTests.cs"/> | ||
| <Compile Include="Unit\ExceptionHandling\ApiExceptionHandlerTests.cs"/> | ||
| <Compile Include="Unit\Middleware\RequestContextMiddlewareTests.cs"/> | ||
| <Compile Include="Unit\ProductCatalog\PatchProductCommandHandlerTests.cs"/> | ||
| <Compile Include="Unit\ProductCatalog\ProductCatalogModuleTests.cs"/> | ||
| <Compile Include="Unit\Security\PermissionAuthorizationHandlerTests.cs"/> | ||
| <Compile Include="Unit\Security\StaticRolePermissionMapTests.cs"/> | ||
| <Compile Include="Unit\BackgroundJobs\Jobs\JobExecutionTests.cs"/> | ||
| <Compile Include="Unit\BackgroundJobs\Jobs\GetJobStatusQueryHandlerTests.cs"/> | ||
| <Compile Include="Unit\BackgroundJobs\Jobs\SubmitJobCommandHandlerTests.cs"/> | ||
| <Compile Include="Unit\Idempotency\InMemoryIdempotencyStoreTests.cs"/> | ||
| <Compile Include="Unit\Webhooks\OutgoingWebhookSsrfTests.cs"/> | ||
| <Compile Remove="GlobalUsings.ApplicationFeatures.cs"/> | ||
| <Compile Remove="Integration\**\*.cs"/> | ||
| <Compile Remove="Unit\Handlers\**\*.cs"/> |
There was a problem hiding this comment.
The test project is excluding all Integration\**\*.cs from compilation. This effectively disables the entire integration test suite (including the modified Integration/Features/WebhooksControllerTests.cs) and is a significant behavior change that isn’t called out in the PR description. If the intent is to keep integration tests running, remove this Compile Remove and fix any legacy references instead; if the intent is to migrate integration tests elsewhere, consider moving them to a separate test project and/or deleting the unused integration sources to avoid confusion.
Comment on lines
+48
to
+56
| [Theory] | ||
| [InlineData("", "sig", "1")] | ||
| [InlineData("{}", "", "1")] | ||
| [InlineData("{}", "sig", "")] | ||
| public void IsValid_WhenTimestampOrSignatureMissing_ReturnsFalse( | ||
| string payload, | ||
| string signature, | ||
| string timestamp | ||
| ) |
There was a problem hiding this comment.
This theory’s name says “TimestampOrSignatureMissing”, but the first case uses a non-empty timestamp ("1") and a non-empty signature ("sig"). Consider either removing that case or renaming/adjusting the test data so each InlineData actually exercises the stated condition (e.g., empty/whitespace timestamp/signature, or add a separate test for empty payload if that’s what you want to cover).
Comment on lines
+5
to
+16
| /// <summary>Private / link-local IPv4 strings for SSRF guard tests.</summary> | ||
| public static class SsrfTheoryData | ||
| { | ||
| public static readonly string[] ProhibitedIpv4List = | ||
| [ | ||
| "127.0.0.1", | ||
| "10.0.0.1", | ||
| "172.16.0.1", | ||
| "172.31.255.255", | ||
| "192.168.1.1", | ||
| "169.254.1.1", | ||
| ]; |
There was a problem hiding this comment.
The doc comment says this data is “Private / link-local IPv4”, but the list also includes loopback (127.0.0.1) and RFC1918 private ranges. Consider rewording the summary to something like “prohibited local IPv4 (loopback/private/link-local)” so it matches the actual contents.
added 5 commits
April 4, 2026 14:55
- Modified the Create method call in EmailValueObjectTests to use the null-forgiving operator, ensuring proper handling of null inputs during test execution.
- Enhanced comments in APITemplate.Tests.csproj to clarify the exclusion of integration tests and migration plans. - Updated summary in SsrfTheoryData to provide clearer context on prohibited local IPv4 strings for SSRF guard tests. - Renamed test method in HmacWebhookPayloadValidatorTests for improved clarity on its purpose regarding payload validation.
…tests - Improved the configuration of APITemplate.Tests.csproj to include new package references for JasperFx and Testcontainers. - Refactored CustomWebApplicationFactory to utilize PostgreSQL and MongoDB containers for integration tests. - Enhanced IntegrationAuthHelper to support new identity structures and improved token generation. - Updated WebhooksControllerTests to ensure proper client initialization and added integration traits for better categorization. - Cleaned up TestServiceHelper by removing obsolete methods and improving service registration logic.
- Added JasperFx package reference to Directory.Packages.props and APITemplate.Tests.csproj for improved command handling. - Refactored DatabaseStartupExtensions to conditionally ensure schema creation for TickerQSchedulerDbContext based on registration status. - Updated AuthBootstrapSeeder to utilize TenantCode for tenant lookups, improving code clarity and correctness. - Enhanced WebhooksControllerTests to ensure proper lazy initialization of the HttpClient, improving test reliability.
- Updated Directory.Packages.props to ensure proper package versioning. - Refactored CustomWebApplicationFactory to streamline container initialization for PostgreSQL and MongoDB. - Simplified validation logic in SseStreamRequestValidationTests by utilizing a helper method for property validation. - Removed obsolete methods and improved type usage in various test files, enhancing overall maintainability and clarity.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.