refactor: Add snapshot tests using Verify.SourceGenerators#48
Merged
DrBarnabus merged 1 commit intomainfrom Feb 22, 2026
Merged
refactor: Add snapshot tests using Verify.SourceGenerators#48DrBarnabus merged 1 commit intomainfrom
DrBarnabus merged 1 commit intomainfrom
Conversation
WalkthroughAdds Verify-based snapshot tests and baselines for the source generator, configures Git and Verify scrubbers, updates test project dependencies, and introduces a ModuleInitializer plus 32+ snapshot files covering 16 scenarios (SourceOutput + GenerationSpec). Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Runner
participant Verifier as Verify.SourceGenerators
participant SG as Source Generator
participant Scrubber as Verifier Scrubber
participant Git as Baseline Compare
Test->>SG: Create compilation & run generator
SG-->>Verifier: Emit generated sources + GenerationSpec
Verifier->>Scrubber: Apply registered scrubbers (normalize GeneratedCodeAttribute)
Scrubber-->>Verifier: Return scrubbed outputs
Verifier->>Git: Compare scrubbed outputs against `.verified.*` baselines
Git-->>Verifier: Report match or diff
Verifier-->>Test: Test pass/fail
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #48 +/- ##
=======================================
Coverage 87.66% 87.66%
=======================================
Files 35 35
Lines 2099 2099
Branches 344 344
=======================================
Hits 1840 1840
Misses 157 157
Partials 102 102 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
src/Directory.Packages.props (1)
29-30:Verify.Xunitis deprecated — plan migration toVerify.XunitV3.
Verify.Xunit31.12.5 is marked as legacy and no longer maintained on NuGet, withVerify.XunitV3as the suggested replacement. The package still resolves against the project's current dependency onxunit.extensibility.execution >= 2.9.3, so the tests function correctly today, but you'll be blocked from future Verify updates onceVerify.Xunitis fully abandoned.Migrating to
Verify.XunitV3would also require upgrading the test project from xunit v2 (2.9.3) to xunit v3 — reasonable to track as a follow-up issue rather than blocking this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Directory.Packages.props` around lines 29 - 30, The project is referencing the deprecated PackageVersion "Verify.Xunit" (31.12.5); update the package management to use "Verify.XunitV3" instead and plan a follow-up to upgrade tests to xUnit v3: replace the PackageVersion entry for Verify.Xunit with Verify.XunitV3 (keeping the same versioning strategy), and add a task/issue to upgrade test projects that rely on xunit.extensibility.execution >= 2.9.3 to xUnit v3 so the new Verify package is fully supported; ensure CI/test pipelines still restore and build after the change.src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.RepeatingPropertyCompositeKey_SourceOutput#UnitTests.UserTagKey.g.verified.cs (2)
165-201: RedundantpartitionKey.Lengthcheck on Line 178 — locked into the snapshot baseline.Line 167 already validates
partitionKey.Length != 36and throwsFormatException. The identical guard on Line 178 (partitionKey.Length != 36) is always false at that point. The same redundancy exists inTryParse(Line 218 vs Line 229). Since this snapshot locks in the generator's output, consider whether the generator should be updated to avoid this redundant check (in a follow-up), or accept it as harmless.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.RepeatingPropertyCompositeKey_SourceOutput`#UnitTests.UserTagKey.g.verified.cs around lines 165 - 201, The Parse method contains a redundant partitionKey.Length check duplicated before Guid.TryParseExact; remove the second unconditional "if (partitionKey.Length != 36)" guard in Parse (and the analogous duplicate in TryParse) so the code only validates length once, then proceeds to Guid.TryParseExact(partitionKey, "d", out var userId); update the generator or snapshot output to eliminate these duplicate checks and keep the subsequent Guid parsing/validation intact (refer to Parse and TryParse functions / methods in the generated output).
184-185: Hardcoded 128-elementstackalloccaps the number of parseable repeating tags.
Span<Range> tagsRanges = stackalloc Range[128]means any key with more than 128 tags will silently truncate duringSplit, potentially producing an incorrect parse result rather than a clear error. The same applies to theTryParsevariant on Line 235. This is a generator output characteristic being locked in; if unbounded collections are a valid use case, the generator may need adjustment in a future PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.RepeatingPropertyCompositeKey_SourceOutput`#UnitTests.UserTagKey.g.verified.cs around lines 184 - 185, The stackalloc buffer for tagsRanges (Span<Range> tagsRanges = stackalloc Range[128]) can silently truncate when more than 128 tags exist; update the parsing code that calls Split (the tagsCount assignment) and the corresponding TryParse variant to detect capacity exhaustion (i.e., tagsCount == 128) and handle it deterministically: either allocate a larger buffer (heap/ArrayPool) before splitting or return a clear failure/error (throw or return false) so inputs with >128 tags don't produce silent incorrect parses. Make the change at the locations referencing tagsRanges, the Split call, and the TryParse path so both normal parse and TryParse behave consistently.src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.ClashingKeyNames_SourceOutput#AnotherNamespace.BasicPrimaryKey.g.verified.cs (1)
154-163:[NotNullWhen(true)]on non-nullablestringparameter is redundant.
partitionKeyis declared asstring(non-nullable), yet it carries[NotNullWhen(true)]. The runtime null-guard at line 156 is fine as a defensive measure for callers without NRT, but the attribute has no effect on a non-nullable reference type. Compare with the single-key overload at line 88 which correctly usesstring?. This is a generator-level inconsistency — you may want to track it for a future generator fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.ClashingKeyNames_SourceOutput`#AnotherNamespace.BasicPrimaryKey.g.verified.cs around lines 154 - 163, The generated TryParse method has a redundant [NotNullWhen(true)] on the non-nullable parameter partitionKey; update the signature of public static bool TryParse(string partitionKey, string sortKey, ... out BasicPrimaryKey? result) to accept nullable refs (string? partitionKey, string? sortKey) or remove the attribute so it matches the nullability intent used by the single-key overload; specifically, adjust the TryParse method declaration and its annotations to either use string? for partitionKey and sortKey (keeping the NotNullWhen attribute) or drop the NotNullWhen attribute if you want to keep non-nullable strings, ensuring consistency with the other TryParse overloads and the ReadOnlySpan-based TryParse call.src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.InvariantCultureDisabled_SourceOutput#UnitTests.BasicPrimaryKey.g.verified.cs (1)
109-113: Redundant length guard in Parse —sortKey.Length < 1(line 112) makessortKey.Length == 0(line 126) dead code.The check at line 112 already ensures
sortKey.Length >= 1, so the subsequentsortKey.Length == 0check at line 126 can never be true. The same pattern appears inTryParse(lines 152 vs 166). This is a minor generator-level issue now locked into the snapshot.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.InvariantCultureDisabled_SourceOutput`#UnitTests.BasicPrimaryKey.g.verified.cs around lines 109 - 113, In both Parse and TryParse, remove the redundant early guard that checks "if (sortKey.Length < 1)" so the later explicit zero-length check "sortKey.Length == 0" remains meaningful; locate the checks inside the Parse and TryParse methods (the "sortKey" length validations) and delete the "< 1" guards to avoid dead code while preserving the later specific length==0 handling and associated error paths.CLAUDE.md (1)
69-71: Lines exceed the 120-character maximum.Lines 69 and 71 exceed the 120-character limit specified in the coding guidelines. Consider wrapping them.
As per coding guidelines:
**/*.{cs,csproj,props,xml,json,md}: Use 4-space indentation, UTF-8 encoding, LF line endings, and enforce 120-character maximum line length.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` around lines 69 - 71, The two Markdown bullet lines referencing Snapshot tests and the ModuleInitializer exceed the 120-character limit; edit the CLAUDE.md bullets (the lines mentioning "Snapshot tests (`Snapshots/`...`GenerationSpec`" and "The `ModuleInitializer` scrubs...") to wrap them into shorter lines under 120 characters while preserving the exact meaning and bullet structure (split long sentences at logical boundaries, e.g., after phrases like "use Verify.SourceGenerators" or "to keep snapshots stable across commits"), keep UTF-8/LF encoding and 4-space indentation conventions, and ensure no other content or punctuation is changed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.BasicCompositePrimaryKey_SourceOutput`#UnitTests.BasicPrimaryKey.g.verified.cs:
- Around line 266-274: The GetFormattedLength method in CustomEnumHelper uses
array indexing into Lengths without validating the enum value, which can produce
an IndexOutOfRangeException for invalid enums; update GetFormattedLength to
validate the value (e.g., cast to uint and check against Lengths.Length or
Names.Length) and throw a descriptive ArgumentOutOfRangeException for
out-of-range values so its semantics match the switch-based helper and the
existing TryFormat guard.
In
`@src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.cs`:
- Around line 232-262: The two repeating-property test pairs are inconsistent:
RepeatingPropertyKey_SourceOutput and RepeatingPropertyCompositeKey_SourceOutput
call RunDriver (which validates generator diagnostics) while their
_GenerationSpec counterparts call
CompilationHelper.RunSourceGenerator(disableDiagnosticValidation: true). Make
validation behavior consistent by disabling diagnostic validation in both
variants: modify RunDriver (or add an overload) to accept a
disableDiagnosticValidation flag and call it with disableDiagnosticValidation:
true from RepeatingPropertyKey_SourceOutput and
RepeatingPropertyCompositeKey_SourceOutput, or alternatively remove the
disableDiagnosticValidation: true from the GenerationSpec calls so both use the
same validation policy; update the tests for RepeatingPropertyKey_* and
RepeatingPropertyCompositeKey_* and the
RunDriver/CompilationHelper.RunSourceGenerator usage accordingly.
In
`@src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.RepeatingPropertyCompositeKey_SourceOutput`#UnitTests.UserTagKey.g.verified.cs:
- Around line 54-61: The ToPartitionKeyString method's switch in
ToPartitionKeyString(int throughPartIndex, bool includeTrailingDelimiter = true)
lacks a (0, true) arm so calling ToPartitionKeyString(0) with the default
includeTrailingDelimiter=true throws; add a (0, true) case (in the same switch
alongside the existing (0, false) case) that returns the partition component
with the PrimaryKeySeparator appended (e.g. the UserId formatted and
concatenated with PrimaryKeySeparator) so both includeTrailingDelimiter variants
are handled.
---
Nitpick comments:
In `@CLAUDE.md`:
- Around line 69-71: The two Markdown bullet lines referencing Snapshot tests
and the ModuleInitializer exceed the 120-character limit; edit the CLAUDE.md
bullets (the lines mentioning "Snapshot tests (`Snapshots/`...`GenerationSpec`"
and "The `ModuleInitializer` scrubs...") to wrap them into shorter lines under
120 characters while preserving the exact meaning and bullet structure (split
long sentences at logical boundaries, e.g., after phrases like "use
Verify.SourceGenerators" or "to keep snapshots stable across commits"), keep
UTF-8/LF encoding and 4-space indentation conventions, and ensure no other
content or punctuation is changed.
In
`@src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.ClashingKeyNames_SourceOutput`#AnotherNamespace.BasicPrimaryKey.g.verified.cs:
- Around line 154-163: The generated TryParse method has a redundant
[NotNullWhen(true)] on the non-nullable parameter partitionKey; update the
signature of public static bool TryParse(string partitionKey, string sortKey,
... out BasicPrimaryKey? result) to accept nullable refs (string? partitionKey,
string? sortKey) or remove the attribute so it matches the nullability intent
used by the single-key overload; specifically, adjust the TryParse method
declaration and its annotations to either use string? for partitionKey and
sortKey (keeping the NotNullWhen attribute) or drop the NotNullWhen attribute if
you want to keep non-nullable strings, ensuring consistency with the other
TryParse overloads and the ReadOnlySpan-based TryParse call.
In
`@src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.InvariantCultureDisabled_SourceOutput`#UnitTests.BasicPrimaryKey.g.verified.cs:
- Around line 109-113: In both Parse and TryParse, remove the redundant early
guard that checks "if (sortKey.Length < 1)" so the later explicit zero-length
check "sortKey.Length == 0" remains meaningful; locate the checks inside the
Parse and TryParse methods (the "sortKey" length validations) and delete the "<
1" guards to avoid dead code while preserving the later specific length==0
handling and associated error paths.
In
`@src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.RepeatingPropertyCompositeKey_SourceOutput`#UnitTests.UserTagKey.g.verified.cs:
- Around line 165-201: The Parse method contains a redundant partitionKey.Length
check duplicated before Guid.TryParseExact; remove the second unconditional "if
(partitionKey.Length != 36)" guard in Parse (and the analogous duplicate in
TryParse) so the code only validates length once, then proceeds to
Guid.TryParseExact(partitionKey, "d", out var userId); update the generator or
snapshot output to eliminate these duplicate checks and keep the subsequent Guid
parsing/validation intact (refer to Parse and TryParse functions / methods in
the generated output).
- Around line 184-185: The stackalloc buffer for tagsRanges (Span<Range>
tagsRanges = stackalloc Range[128]) can silently truncate when more than 128
tags exist; update the parsing code that calls Split (the tagsCount assignment)
and the corresponding TryParse variant to detect capacity exhaustion (i.e.,
tagsCount == 128) and handle it deterministically: either allocate a larger
buffer (heap/ArrayPool) before splitting or return a clear failure/error (throw
or return false) so inputs with >128 tags don't produce silent incorrect parses.
Make the change at the locations referencing tagsRanges, the Split call, and the
TryParse path so both normal parse and TryParse behave consistently.
In `@src/Directory.Packages.props`:
- Around line 29-30: The project is referencing the deprecated PackageVersion
"Verify.Xunit" (31.12.5); update the package management to use "Verify.XunitV3"
instead and plan a follow-up to upgrade tests to xUnit v3: replace the
PackageVersion entry for Verify.Xunit with Verify.XunitV3 (keeping the same
versioning strategy), and add a task/issue to upgrade test projects that rely on
xunit.extensibility.execution >= 2.9.3 to xUnit v3 so the new Verify package is
fully supported; ensure CI/test pipelines still restore and build after the
change.
...rSnapshotTests.BasicCompositePrimaryKey_SourceOutput#UnitTests.BasicPrimaryKey.g.verified.cs
Show resolved
Hide resolved
src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.cs
Show resolved
Hide resolved
...rSnapshotTests.RepeatingPropertyCompositeKey_SourceOutput#UnitTests.UserTagKey.g.verified.cs
Show resolved
Hide resolved
Add Verify.SourceGenerators snapshot tests to capture generated source output and GenerationSpec models as baselines for all 16 CompilationHelper scenarios. These snapshots act as the safety net for subsequent refactoring work, catching any unintended changes to emitted code or parser output. - Add Verify.SourceGenerators and Verify.Xunit to central package management - Add ModuleInitializer with scrubber for non-deterministic GeneratedCodeAttribute version - Add 32 snapshot tests (16 source output + 16 GenerationSpec) in Snapshots/ subfolder - Add .gitignore entry for *.received.* and .gitattributes for *.verified.* files - Update CLAUDE.md with snapshot testing conventions
DrBarnabus
force-pushed
the
refactor/add-snapshot-tests
branch
from
905a987 to
6b546dc
Compare
This was referenced Feb 22, 2026
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.NestedTypeDeclarations_SourceOutput#UnitTests.OutermostClass.BasicPrimaryKey.g.verified.cs (2)
55-88:ToPartitionKeyString()(no-arg overload) is a full copy ofToString().Because this type has no separator, both methods must return the same full-key string. The emitter could instead emit a one-liner delegation:
♻️ Suggested emitter output simplification
- public string ToPartitionKeyString() - { - return string.Create(110, this, static (destination, state) => - { - // ... identical body to ToString() ... - }); - } + public string ToPartitionKeyString() => ToString();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.NestedTypeDeclarations_SourceOutput`#UnitTests.OutermostClass.BasicPrimaryKey.g.verified.cs around lines 55 - 88, The ToPartitionKeyString() no-arg overload is duplicating ToString() output by re-emitting the full formatting logic; since this type has no separator the two should be identical, so replace the duplicated body in ToPartitionKeyString() with a simple delegation to ToString() (e.g. return ToString();) to avoid redundant code and keep behavior consistent between ToPartitionKeyString() and ToString().
27-27: Numerous lines in the emitted output exceed the 120-character limit.Examples include the
ISpanFormattable.TryFormat(...)call sites (lines 27, 37, 47, 62, 72, 82), theswitch-expression arms (lines 94–99), theGuid.TryParseExact(...)chains (lines 120, 123, 126, 155, 158, 161), and the explicit interface delegate lines (175, 181). All of these originate in the emitter; the snapshot simply reflects what the generator produces.As per coding guidelines,
**/*.{cs,...}files enforce a 120-character maximum line length.Also applies to: 37-37, 47-47, 62-62, 72-72, 82-82, 94-99, 120-127, 155-162, 175-175, 181-181
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.NestedTypeDeclarations_SourceOutput`#UnitTests.OutermostClass.BasicPrimaryKey.g.verified.cs at line 27, The generated source contains many lines exceeding the 120-char limit (notably the ISpanFormattable.TryFormat(...) call sites, the switch-expression arms, the Guid.TryParseExact(...) chains, and the explicit-interface delegate expressions); update the emitter so it breaks long expressions into shorter pieces: assign complex expressions or slices to clearly named local variables before calling TryFormat (e.g., capture destination[position..] into a var), break argument lists and chained calls across multiple lines, emit multi-line switch arms instead of long single-line arms, and replace inline long lambda/delegate expressions for explicit interface implementations with private helper methods or local funcs; apply these changes around the symbols seen in the snapshot (ISpanFormattable.TryFormat, the switch-expression arms, Guid.TryParseExact chains, and the explicit interface delegate emit sites) so emitted lines stay <=120 chars.src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.ExplicitlyMarkedConstructor_SourceOutput#UnitTests.BasicPrimaryKey.g.verified.cs (1)
117-123: Lines 117 and 123 exceed the 120-character limit prescribed by the coding guidelines.Both explicit interface implementations carry two fully-qualified
[global::System.Diagnostics.CodeAnalysis.*]attributes inline, pushing the lines well past 120 characters. As this snapshot faithfully captures generator output, the fix is in the emitter — the attribute arguments could be placed on separate lines (or the fully-qualified form used only where necessary).As per coding guidelines,
**/*.{cs,csproj,props,xml,json,md}enforces a 120-character maximum line length.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.ExplicitlyMarkedConstructor_SourceOutput`#UnitTests.BasicPrimaryKey.g.verified.cs around lines 117 - 123, The snapshot generator emits explicit interface implementations for IParsable<BasicPrimaryKey>.TryParse and ISpanParsable<BasicPrimaryKey>.TryParse with long fully-qualified attributes inline, exceeding the 120-char limit; update the emitter (the code that builds these members) to break attribute arguments or place attributes on their own line(s) (or use shorter attribute names where safe) for the methods IParsable<BasicPrimaryKey>.TryParse, ISpanParsable<BasicPrimaryKey>.Parse, and ISpanParsable<BasicPrimaryKey>.TryParse so the generated lines stay within 120 characters.src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.BasicNonSequentialEnumPrimaryKey_GenerationSpec.verified.txt (1)
157-194:LengthRequired: 1forThirdPartis lower than the actual minimum member name length.The
CustomEnumhas three members whose serialised names are"One"(3),"Two"(3), and"Three"(5). The shortest possible sort-key value is thereforeConstantValue#One= 17 characters, yet the generator assignsLengthRequired: 1to the enum part, producing a sort-key pre-check ofsortKey.Length < 15(instead of< 17) in the generated code.Inputs of length 15 or 16 pass the pre-check but are correctly rejected by the subsequent split/parse step, so correctness is maintained. This is solely a minor optimisation gap in the generator's length-inference logic for enum key parts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.BasicNonSequentialEnumPrimaryKey_GenerationSpec.verified.txt` around lines 157 - 194, The generator is underestimating enum part minimum length (LengthRequired = 1 for ThirdPart/EnumSpec CustomEnum); update the enum-length inference in the source generator so that for FormatType/ParseType Enum you compute the minimum serialized member-name length (min length of EnumSpec.Members' Names) and add any fixed prefix/suffix length used in the key format (e.g., the "ConstantValue#" or other constant characters) to produce the correct LengthRequired, then use that value instead of 1 when emitting LengthRequired for properties like ThirdPart.src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.BasicNonSequentialEnumPrimaryKey_SourceOutput#UnitTests.BasicNonSequentialEnum.g.verified.cs (1)
279-300:TryFormatdefault branch is dead code.
GetFormattedLength(value)(line 281) throwsArgumentOutOfRangeExceptionfor any enum value outsideOne/Two/Three, so execution never reaches theswitchdefaultcase at lines 296–298. ThecharsWritten = 0; return false;there is unreachable.A secondary (even lower-severity) observation: the buffer-too-small path leaves
charsWrittenset to the required length rather than 0. TheISpanFormattableconvention ischarsWritten = 0on anyfalsereturn, though the value is never observed here because all call sites supply a pre-sized buffer fromstring.Create.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.BasicNonSequentialEnumPrimaryKey_SourceOutput`#UnitTests.BasicNonSequentialEnum.g.verified.cs around lines 279 - 300, In TryFormat, ensure false returns set charsWritten=0 and make the unreachable default explicit: instead of leaving charsWritten as the required length when destination is too small, set charsWritten = 0 before returning false; and replace the switch default branch (currently setting charsWritten = 0 and returning false) with throwing an ArgumentOutOfRangeException (to match GetFormattedLength behavior) or remove the default entirely so callers see the same exception semantics as GetFormattedLength; reference TryFormat and GetFormattedLength when making these changes.src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.RepeatingPropertyCompositeKey_SourceOutput#UnitTests.UserTagKey.g.verified.cs (1)
184-184: Hard-codedstackalloc Range[128]caps repeating tags at 128.The generated parsing code uses
Span<Range> tagsRanges = stackalloc Range[128], which silently truncates if there are more than 128 tags. If the split produces more than 128 segments,ReadOnlySpan<char>.Splitwill merge the remaining text into the last range, causing incorrect tag values rather than an explicit error. This is a generator-level concern, but worth documenting as a known limitation or adding an overflow check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.RepeatingPropertyCompositeKey_SourceOutput`#UnitTests.UserTagKey.g.verified.cs at line 184, The generated parser currently uses a fixed buffer "Span<Range> tagsRanges = stackalloc Range[128]" which silently truncates when ReadOnlySpan<char>.Split produces more than 128 segments; change the generator to detect overflow by checking the split/segment count against the stackalloc length (the tagsRanges buffer) and either (a) throw a clear exception when the number of tags exceeds the capacity, or (b) switch to an heap-allocated array (e.g., new Range[count]) when more than 128 segments are needed; update the code paths that call ReadOnlySpan<char>.Split and the logic that writes into tagsRanges to perform this capacity check before writing into tagsRanges to avoid merging of trailing text into the last range.src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.cs (1)
268-281: Minor:RunDrivervalidates only generator diagnostics, not compilation diagnostics.
CompilationHelper.RunSourceGenerator(used in the_GenerationSpectests) validates both compilation-level and generator-level diagnostics whendisableDiagnosticValidationis false, whereasRunDriveronly checksdriver.GetRunResult().Diagnostics(generator-level). This means a scenario that emits a compilation error (e.g. Warning or Error severity from the output compilation) would be caught in the_GenerationSpectest but silently pass in the_SourceOutputtest.For snapshot tests this is low risk — the verified baselines would still catch unexpected output changes — but worth being aware of if this is ever used as a correctness gate beyond snapshot diffing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.cs` around lines 268 - 281, RunDriver currently only checks generator diagnostics via driver.GetRunResult().Diagnostics and ignores compilation-level diagnostics; update RunDriver (and respect the disableDiagnosticValidation flag) to also validate compilation.GetDiagnostics() (filtering Severity > DiagnosticSeverity.Info) and assert it's empty when validation is enabled, in addition to the existing check of driver.GetRunResult().Diagnostics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.BasicNonSequentialEnumPrimaryKey_SourceOutput`#UnitTests.BasicNonSequentialEnum.g.verified.cs:
- Around line 202-211: Update the two-string TryParse overload so its parameters
are nullable to match the NotNullWhen attribute and the single-key overload:
change the parameter types for partitionKey and sortKey from string to string?
in the TryParse method signature (the overload that calls
TryParse(ReadOnlySpan<char>, ReadOnlySpan<char>, out BasicNonSequentialEnum?))
so the [NotNullWhen(true)] attribute is meaningful and consistent with the
single-key TryParse(primaryKey) overload; ensure the null-check/guard remains
and that the source generator emits the nullable signatures (adjust generator
logic that builds the partitionKey/sortKey parameter types).
In
`@src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.ClashingKeyNames_GenerationSpec.verified.txt`:
- Around line 149-153: PropertyValidation.GetFormattedLength currently maps the
Guid "x" format incorrectly to (32,false); update the mapping for the "x" (and
"X" if present) specifier to return (68,true) so the method returns the fixed
68-character exact-length result used by the "x" Guid format; modify the entry
in PropertyValidation.GetFormattedLength (the switch/lookup that includes the
line currently returning "x" => (32, false)) to "x" => (68, true) so emitter and
parser can use the fixed-size fast-path.
In
`@src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.NestedPrivateTypeDeclarations_SourceOutput`#UnitTests.OutermostClass.BasicPrimaryKey.g.verified.cs:
- Around line 27-31: The emitter is writing a hardcoded tab into generated throw
statements (e.g., writer.WriteLine("\tthrow new FormatException();\n") in
SourceGenerator.Emitter.cs); remove the literal "\t" and the explicit "\n" and
use the writer's indentation API/context so the line is emitted with the current
indentation (e.g., replace the call with writer.WriteLine("throw new
FormatException();") or the equivalent writer call your emitter uses for
indented lines) so generated code uses the writer's 4-space indentation rather
than a tab.
---
Duplicate comments:
In
`@src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.BasicCompositePrimaryKey_SourceOutput`#UnitTests.BasicPrimaryKey.g.verified.cs:
- Around line 266-274: GetFormattedLength in class CustomEnumHelper currently
indexes Lengths[(uint)value] without validating value; add the same guard used
in TryFormat (check (uint)value >= Names.Length or Lengths.Length) and if out of
range throw a descriptive ArgumentOutOfRangeException (include the parameter
name and the invalid value) rather than letting an IndexOutOfRangeException
propagate, so look for GetFormattedLength, Lengths, Names and TryFormat and
implement the bounds check and explicit exception there.
In
`@src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.RepeatingPropertyCompositeKey_SourceOutput`#UnitTests.UserTagKey.g.verified.cs:
- Around line 54-61: The switch in ToPartitionKeyString is missing the (0, true)
arm so calling ToPartitionKeyString(0) (default includeTrailingDelimiter = true)
falls to the throw; add a new case for (0, true) in the ToPartitionKeyString
method that returns the UserId formatted as in the existing (0, false) arm but
with the PrimaryKeySeparator appended (e.g., $"{UserId:d}{PrimaryKeySeparator}")
so the generator emits a `(0, true)` branch alongside the existing `(0, false)`
branch.
---
Nitpick comments:
In
`@src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.BasicNonSequentialEnumPrimaryKey_GenerationSpec.verified.txt`:
- Around line 157-194: The generator is underestimating enum part minimum length
(LengthRequired = 1 for ThirdPart/EnumSpec CustomEnum); update the enum-length
inference in the source generator so that for FormatType/ParseType Enum you
compute the minimum serialized member-name length (min length of
EnumSpec.Members' Names) and add any fixed prefix/suffix length used in the key
format (e.g., the "ConstantValue#" or other constant characters) to produce the
correct LengthRequired, then use that value instead of 1 when emitting
LengthRequired for properties like ThirdPart.
In
`@src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.BasicNonSequentialEnumPrimaryKey_SourceOutput`#UnitTests.BasicNonSequentialEnum.g.verified.cs:
- Around line 279-300: In TryFormat, ensure false returns set charsWritten=0 and
make the unreachable default explicit: instead of leaving charsWritten as the
required length when destination is too small, set charsWritten = 0 before
returning false; and replace the switch default branch (currently setting
charsWritten = 0 and returning false) with throwing an
ArgumentOutOfRangeException (to match GetFormattedLength behavior) or remove the
default entirely so callers see the same exception semantics as
GetFormattedLength; reference TryFormat and GetFormattedLength when making these
changes.
In
`@src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.cs`:
- Around line 268-281: RunDriver currently only checks generator diagnostics via
driver.GetRunResult().Diagnostics and ignores compilation-level diagnostics;
update RunDriver (and respect the disableDiagnosticValidation flag) to also
validate compilation.GetDiagnostics() (filtering Severity >
DiagnosticSeverity.Info) and assert it's empty when validation is enabled, in
addition to the existing check of driver.GetRunResult().Diagnostics.
In
`@src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.ExplicitlyMarkedConstructor_SourceOutput`#UnitTests.BasicPrimaryKey.g.verified.cs:
- Around line 117-123: The snapshot generator emits explicit interface
implementations for IParsable<BasicPrimaryKey>.TryParse and
ISpanParsable<BasicPrimaryKey>.TryParse with long fully-qualified attributes
inline, exceeding the 120-char limit; update the emitter (the code that builds
these members) to break attribute arguments or place attributes on their own
line(s) (or use shorter attribute names where safe) for the methods
IParsable<BasicPrimaryKey>.TryParse, ISpanParsable<BasicPrimaryKey>.Parse, and
ISpanParsable<BasicPrimaryKey>.TryParse so the generated lines stay within 120
characters.
In
`@src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.NestedTypeDeclarations_SourceOutput`#UnitTests.OutermostClass.BasicPrimaryKey.g.verified.cs:
- Around line 55-88: The ToPartitionKeyString() no-arg overload is duplicating
ToString() output by re-emitting the full formatting logic; since this type has
no separator the two should be identical, so replace the duplicated body in
ToPartitionKeyString() with a simple delegation to ToString() (e.g. return
ToString();) to avoid redundant code and keep behavior consistent between
ToPartitionKeyString() and ToString().
- Line 27: The generated source contains many lines exceeding the 120-char limit
(notably the ISpanFormattable.TryFormat(...) call sites, the switch-expression
arms, the Guid.TryParseExact(...) chains, and the explicit-interface delegate
expressions); update the emitter so it breaks long expressions into shorter
pieces: assign complex expressions or slices to clearly named local variables
before calling TryFormat (e.g., capture destination[position..] into a var),
break argument lists and chained calls across multiple lines, emit multi-line
switch arms instead of long single-line arms, and replace inline long
lambda/delegate expressions for explicit interface implementations with private
helper methods or local funcs; apply these changes around the symbols seen in
the snapshot (ISpanFormattable.TryFormat, the switch-expression arms,
Guid.TryParseExact chains, and the explicit interface delegate emit sites) so
emitted lines stay <=120 chars.
In
`@src/CompositeKey.SourceGeneration.UnitTests/Snapshots/SourceGeneratorSnapshotTests.RepeatingPropertyCompositeKey_SourceOutput`#UnitTests.UserTagKey.g.verified.cs:
- Line 184: The generated parser currently uses a fixed buffer "Span<Range>
tagsRanges = stackalloc Range[128]" which silently truncates when
ReadOnlySpan<char>.Split produces more than 128 segments; change the generator
to detect overflow by checking the split/segment count against the stackalloc
length (the tagsRanges buffer) and either (a) throw a clear exception when the
number of tags exceeds the capacity, or (b) switch to an heap-allocated array
(e.g., new Range[count]) when more than 128 segments are needed; update the code
paths that call ReadOnlySpan<char>.Split and the logic that writes into
tagsRanges to perform this capacity check before writing into tagsRanges to
avoid merging of trailing text into the last range.
...BasicNonSequentialEnumPrimaryKey_SourceOutput#UnitTests.BasicNonSequentialEnum.g.verified.cs
Show resolved
Hide resolved
...nitTests/Snapshots/SourceGeneratorSnapshotTests.ClashingKeyNames_GenerationSpec.verified.txt
Show resolved
Hide resolved
...dPrivateTypeDeclarations_SourceOutput#UnitTests.OutermostClass.BasicPrimaryKey.g.verified.cs
Show resolved
Hide resolved
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
1 participant
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.
Summary
Closes #39
Verify.SourceGeneratorsandVerify.Xunitto central package management andSourceGeneration.UnitTestsproject referencesModuleInitializerwith targeted scrubber that normalises the non-deterministicGeneratedCodeAttributeversion string to"VERSION"GenerationSpecmodel) covering allCompilationHelperfactory scenarios, organised in aSnapshots/subfolder*.received.*to.gitignoreand*.verified.*entries to.gitattributesper Verify conventionsCLAUDE.mdwith snapshot testing guidanceThese snapshots act as the safety net for all subsequent refactoring PRs — any change to emitted
.g.cscode or parser-extractedGenerationSpecmodels will cause a snapshot diff failure.Test plan
GeneratedCodeAttributeversion with"VERSION"— no other content is scrubbed*.received.*files are gitignored; only*.verified.*baselines are committedSummary by CodeRabbit
Tests
Chores