Cover change converter and config/adapter guard paths - #95
Conversation
Adds coverage for the bespoke IChange converter and the config/adapter guard branches, which were largely untested: - PeekThenConcreteChangeConverter malformed-input throws (non-object root, empty object, non-string $type) and OpaqueChange EntityId parsing (present, missing-defaults-to-empty, nested-json round-trip). - OpaqueChange runtime contract: EntityType and NewEntity throw, ApplyChange is a no-op. - ChangeTypeListBuilder duplicate-add idempotency and frozen guard. - ObjectTypeListBuilder frozen guard, DefaultAdapter singleton reuse, and Adapt dispatch (success, non-IObjectBase throw, no-provider-matches throw). - DerivedTypeHelper duplicate-type throw and wrong-base throw. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 46 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe pull request adds tests for JSON change conversion, opaque changes, configuration builder state, object adaptation, and derived-type registration and lookup. ChangesBehavior coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The lambda inferred Func<Type>, which FluentAssertions did not reliably invoke (passed in Debug, failed in Release CI with 'no exception thrown'). Use an explicit Action with a discard so the throwing getter always runs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/SIL.Harmony.Tests/Helpers/DerivedTypeHelperTests.cs (2)
13-18: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse a different discriminator for the duplicate registration.
Both calls use
"Word". This test cannot prove thatAddDerivedTyperejects the duplicateDerivedTyperather than only rejecting a duplicate discriminator. Keeptypeof(Word)and change the second discriminator.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/SIL.Harmony.Tests/Helpers/DerivedTypeHelperTests.cs` around lines 13 - 18, Update the duplicate-registration test around AddDerivedType so the second call still uses typeof(Word) but supplies a different discriminator from "Word". Keep the existing exception assertion, ensuring the test verifies duplicate derived-type rejection rather than duplicate-discriminator rejection.
24-28: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the stored registration values.
The count assertion can pass when the list contains incorrect
JsonDerivedTypevalues. Assert that the list containsWordwith"Word"andDefinitionwith"Definition".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/SIL.Harmony.Tests/Helpers/DerivedTypeHelperTests.cs` around lines 24 - 28, Update the test around the dictionary registration and its count assertion to verify the stored JsonDerivedType values, asserting entries for Word with "Word" and Definition with "Definition" rather than only checking the list count.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/SIL.Harmony.Tests/Helpers/DerivedTypeHelperTests.cs`:
- Around line 13-18: Update the duplicate-registration test around
AddDerivedType so the second call still uses typeof(Word) but supplies a
different discriminator from "Word". Keep the existing exception assertion,
ensuring the test verifies duplicate derived-type rejection rather than
duplicate-discriminator rejection.
- Around line 24-28: Update the test around the dictionary registration and its
count assertion to verify the stored JsonDerivedType values, asserting entries
for Word with "Word" and Definition with "Definition" rather than only checking
the list count.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 59005f4a-b8b9-4437-8d3e-0d4773d84c8b
📒 Files selected for processing (3)
src/SIL.Harmony.Tests/ChangeConverterTests.cssrc/SIL.Harmony.Tests/ConfigTests.cssrc/SIL.Harmony.Tests/Helpers/DerivedTypeHelperTests.cs
The FluentAssertions delegate form did not reliably invoke the getter under the CI Release SDK (passed locally, failed in CI). Assert.Throws' Func overload returns and consumes the property value, guaranteeing the throwing getter runs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- duplicate-registration test now uses a different discriminator for the second call, proving the guard rejects a duplicate DerivedType rather than a duplicate discriminator. - different-types test asserts the stored JsonDerivedType values instead of only the count. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed both CodeRabbit nitpicks on
🤖 Addressed by Claude Code |
Every delegate-wrapped access of the throw-bodied getter (FluentAssertions Func/Action and Assert.Throws' Func overload) failed to observe the exception under the CI SDK. Access the getter directly in the method body inside a try/catch and consume the value via GC.KeepAlive so the call cannot be elided. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
AI-generated summary
Fourth batch from the test-suite review, covering the bespoke
IChangeconverter and the config/adapter guard branches — the converter owns allIChange(de)serialization yet its error paths and theOpaqueChangeentity-id handling were untested.ChangeConverterTestsExpected StartObject), empty object (Expected property name), non-string$typediscriminator.OpaqueChangeentity-id parsing: parsed when present, defaults toGuid.Emptywhen missing, and nested JSON survives a round-trip.OpaqueChangeruntime contract:EntityTypeandNewEntitythrowNotSupportedException;ApplyChangeis a no-op.ConfigTestsChangeTypeListBuilder: duplicateAddis idempotent;Addafter freeze throws.ObjectTypeListBuilder:Add/DefaultAdapterafter freeze throws;DefaultAdapter()returns the same instance on repeat calls;Adaptdispatch covered for success, a non-IObjectBaseargument (throws), and no-provider-matches with two providers registered (throws).DerivedTypeHelperTests(new file) — duplicate derived-type registration throws;GetEntityDiscriminatorthrows when the instance type is not assignable to the base. This helper is the guard behind adapter double-registration and had zero references.Test-only; no production code changed.
Test plan
dotnet teston the affected classes passes (29/29 locally).Summary by CodeRabbit