Severity: Warning
File: tests/Servy.Infrastructure.UnitTests/Data/ServiceRepositoryTests.cs lines 37–127 (pattern repeats)
Description:
The repository tests verify Dapper parameter objects with It.Is<object>(obj => GetPropertyValue(obj, "Password") == "p_enc" && GetPropertyValue(obj, "Parameters") == "args_enc" && ...). GetPropertyValue is a reflection helper that looks up properties by string name. Each assertion chains nine or more string-keyed property reads:
It.Is<object>(obj =>
GetPropertyValue(obj, "Password") == "p_enc" &&
GetPropertyValue(obj, "Parameters") == "args_enc" &&
GetPropertyValue(obj, "EnvironmentVariables") == "env_enc" &&
GetPropertyValue(obj, "PreLaunchParams") == "pre_args_enc" &&
// …5+ more …
)Problems this creates:
- Any rename in
ServiceDto (e.g., PreLaunchParams → PreLaunchArguments) breaks every test even if production behaviour is unchanged.
- A missing property in the anonymous object silently returns
null from the reflection helper, which silently makes the predicate false, which presents as a generic "Verify() expected 1 call, got 0" error without telling the reviewer that the real problem is a name mismatch.
- The test couples to the string shape of the encrypted-DTO wire format rather than the contract — "this call encrypts Password, Parameters and envVars before persisting".
This is related to #552 (mock-only tests verify Moq dispatch) but is a specifically worse variant: it is both mock-only and stringly-typed.
Suggested fix:
Replace the property-by-property reflection predicate with a typed capture + comparer:
ServiceDto? captured = null;
_dapperMock.Setup(d => d.ExecuteAsync(It.IsAny<string>(),
Capture.In(captured, x => x as ServiceDto), // or use Moq's Capture.With
It.IsAny<CancellationToken>()))
.ReturnsAsync(1);
// act …
Assert.NotNull(captured);
Assert.Equal("p_enc", captured.Password);
Assert.Equal("args_enc", captured.Parameters);
// … etcIf Dapper is being called with an anonymous type (no ServiceDto), consider encapsulating the encryption step in a seam that does return a typed object, and test that seam directly. That way the repository test can focus on "did we ask the seam to encrypt this DTO?" rather than "what fields did the anonymous parameter bag end up with?".
Severity: Warning
File:
tests/Servy.Infrastructure.UnitTests/Data/ServiceRepositoryTests.cslines 37–127 (pattern repeats)Description:
The repository tests verify Dapper parameter objects with
It.Is<object>(obj => GetPropertyValue(obj, "Password") == "p_enc" && GetPropertyValue(obj, "Parameters") == "args_enc" && ...).GetPropertyValueis a reflection helper that looks up properties by string name. Each assertion chains nine or more string-keyed property reads:Problems this creates:
ServiceDto(e.g.,PreLaunchParams→PreLaunchArguments) breaks every test even if production behaviour is unchanged.nullfrom the reflection helper, which silently makes the predicate false, which presents as a generic "Verify() expected 1 call, got 0" error without telling the reviewer that the real problem is a name mismatch.This is related to #552 (mock-only tests verify Moq dispatch) but is a specifically worse variant: it is both mock-only and stringly-typed.
Suggested fix:
Replace the property-by-property reflection predicate with a typed capture + comparer:
If Dapper is being called with an anonymous type (no
ServiceDto), consider encapsulating the encryption step in a seam that does return a typed object, and test that seam directly. That way the repository test can focus on "did we ask the seam to encrypt this DTO?" rather than "what fields did the anonymous parameter bag end up with?".