From 875d666691161cf0bd37dcab699fcce62e13ceef Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Thu, 27 Aug 2026 14:31:47 +1000 Subject: [PATCH 1/3] An append onto the same content is AlreadyApplied A multi-targeted project transitioning to inline snapshots fails the same call site under every framework, and each one queues an append. Accepting the first writes the literal the rest are still carrying, so the second accept found a Snapshot call in the way and refused - reporting a failure over a source file that was already right, and sending the reader off to re-run a test with nothing left to say. TryAppend now reads the chained call's argument before refusing. Same content is AlreadyApplied; different content is still NotFound, since that one genuinely cannot say what it wants until it has been re-run against the literal now in the source. Compared by value rather than by text, so a literal written in another shape still counts and F# answers as C# does despite the layout of its triple quoted literal. WalkChain reports the position of the call it found rather than the fact of it, because a caller deciding what to do about one has to read its argument. --- docs/inline.md | 2 + src/DiffEngine.Tests/InlinePatcherFsTests.cs | 24 +++++++++ src/DiffEngine.Tests/InlinePatcherTests.cs | 30 +++++++++++ src/DiffEngine/Inline/InlineApplier.cs | 10 ++-- src/DiffEngine/Inline/InlinePatcher.cs | 56 +++++++++++++++++--- 5 files changed, 110 insertions(+), 12 deletions(-) diff --git a/docs/inline.md b/docs/inline.md index 1fbcfac9..4575c23b 100644 --- a/docs/inline.md +++ b/docs/inline.md @@ -211,6 +211,8 @@ For the staging fallback, where no viewer could be resolved and the patch is a f `Apply` returns `Applied`, `AlreadyApplied` (the literal already matches), `NotFound` (the source changed since the test run — tell the user to re-run rather than retrying), or a failure with a message (locked file, unreadable source), which is retryable. +`AlreadyApplied` covers an `Append` onto a call that already has a `Snapshot` call holding this same content, which is what a multi-targeted project transitioning to inline meets: every framework fails the call site and queues an append, and whichever is accepted first writes the literal the rest are carrying. Only a chained call holding *different* content is `NotFound` — that one genuinely cannot say what it wants until it has been re-run against the literal now in the source. Accepting one framework's append before the others have run does mean the queue never sees them together, so a real disagreement between frameworks is reported as that `NotFound` rather than as a conflict to pick from. + `Remove` mode patches are configuration changes with nothing to review: apply them directly; `AddInlineAsync` refuses them. diff --git a/src/DiffEngine.Tests/InlinePatcherFsTests.cs b/src/DiffEngine.Tests/InlinePatcherFsTests.cs index 8bd5a0a5..d4eeba98 100644 --- a/src/DiffEngine.Tests/InlinePatcherFsTests.cs +++ b/src/DiffEngine.Tests/InlinePatcherFsTests.cs @@ -480,6 +480,30 @@ public async Task AppendIsRefusedWhenOneIsAlreadyChained() await Assert.That(reason).Contains("already has a Snapshot call"); } + // The second framework of a multi-targeted transition, appending onto the literal the first + // one's accept just wrote. Read by value, so F# gets the same answer C# does even though the + // layout of its triple quoted literal is nothing like the content it stands for + [Test] + public async Task AppendOntoTheSameContentIsAlreadyApplied() + { + var source = Test(" Verifier.Verify(15).Snapshot(\"same\").ToTask()"); + + var status = TryApply(source, 5, InlinePatchMode.Append, null, "same", out _, out _); + + await Assert.That(status).IsEqualTo(PatchStatus.AlreadyApplied); + } + + [Test] + public async Task AppendingTheSameContentTwiceIsAlreadyApplied() + { + var source = Test(" Verifier.Verify(15).ToTask()"); + + TryApply(source, 5, InlinePatchMode.Append, null, "a\nb", out var applied, out _); + var status = TryApply(applied, 5, InlinePatchMode.Append, null, "a\nb", out _, out _); + + await Assert.That(status).IsEqualTo(PatchStatus.AlreadyApplied); + } + [Test] public async Task AppendSkipsAVerifyOnAnotherReceiver() { diff --git a/src/DiffEngine.Tests/InlinePatcherTests.cs b/src/DiffEngine.Tests/InlinePatcherTests.cs index d7e55c2f..6f92ec5a 100644 --- a/src/DiffEngine.Tests/InlinePatcherTests.cs +++ b/src/DiffEngine.Tests/InlinePatcherTests.cs @@ -821,6 +821,36 @@ public async Task AppendIsRefusedWhenOneIsAlreadyChained() await Assert.That(reason).Contains("already has a Snapshot call"); } + /// + /// The multi-targeted transition, which is where an append lands on a chained call that is + /// not in its way at all. Two frameworks fail the same snapshot, each queues an append, and + /// accepting the first writes the literal the second is still carrying. Refused, that reads + /// as a failure over a source file that is already right, and sends the reader off to re-run + /// a test with nothing left to say. + /// + [Test] + public async Task AppendOntoTheSameContentIsAlreadyApplied() + { + var source = Method(" await Verify(value)\n .Snapshot(\"new\");"); + + var status = TryApply(source, 5, InlinePatchMode.Append, null, "new", out _, out _); + + await Assert.That(status).IsEqualTo(PatchStatus.AlreadyApplied); + } + + // The two halves of that have to agree, or the second framework's accept is refused over a + // literal the first one wrote from the very content being compared against it + [Test] + public async Task AppendingTheSameContentTwiceIsAlreadyApplied() + { + var source = Method(" await Verify(value);"); + + TryApply(source, 5, InlinePatchMode.Append, null, "a\nb", out var applied, out _); + var status = TryApply(applied, 5, InlinePatchMode.Append, null, "a\nb", out _, out _); + + await Assert.That(status).IsEqualTo(PatchStatus.AlreadyApplied); + } + [Test] public async Task AppendWithNoVerifyCall() { diff --git a/src/DiffEngine/Inline/InlineApplier.cs b/src/DiffEngine/Inline/InlineApplier.cs index ee6c01ea..0972b2a3 100644 --- a/src/DiffEngine/Inline/InlineApplier.cs +++ b/src/DiffEngine/Inline/InlineApplier.cs @@ -40,10 +40,12 @@ public static InlineApplyResult CanApply(InlinePatch patch) => /// over. Applied for yes, NotFound for no, and Failed where the source could not be read. /// /// Apart from CanApply in one way, and only for Append: a call that already has a Snapshot call - /// chained onto it answers yes here and is refused there. Both are right. An accept has nowhere - /// to put the literal it is carrying and says to re-run; a producer asking whether this call - /// site can host an inline snapshot has its answer, and taking the verification off inline - /// because another process got there first would be the wrong lesson to draw. + /// chained onto it holding other content answers yes here and is refused there. Both are right. + /// An accept has nowhere to put the literal it is carrying and says to re-run; a producer + /// asking whether this call site can host an inline snapshot has its answer, and taking the + /// verification off inline because another process got there first would be the wrong lesson + /// to draw. Where the chained call holds this same content there is nothing to tell apart and + /// both say yes, CanApply as AlreadyApplied. /// /// public static InlineApplyResult CanAnchor(InlinePatch patch) => diff --git a/src/DiffEngine/Inline/InlinePatcher.cs b/src/DiffEngine/Inline/InlinePatcher.cs index 39dc7b00..0ef27c84 100644 --- a/src/DiffEngine/Inline/InlinePatcher.cs +++ b/src/DiffEngine/Inline/InlinePatcher.cs @@ -433,10 +433,22 @@ static PatchStatus TryAppend( return PatchStatus.Applied; } - var insertAt = WalkChain(source, scan, closeParen + 1, methodName, out var alreadyChained); - // Another process may have appended one between the run and the accept - if (alreadyChained) + var insertAt = WalkChain(source, scan, closeParen + 1, methodName, out var chained); + // Another process may have appended one between the run and the accept, and two + // frameworks failing the same call site is the ordinary way that happens: each queues an + // append, and accepting the first leaves the second with nowhere to put a literal that is + // already there. Only the content tells the two apart. The same snapshot is done, and + // saying so matters - a refusal reads as a failure, and the reader who sent two identical + // snapshots and got one applied and one rejected has no way to see that their source is + // already right. A different one is a call site that cannot say what it wants until it has + // been re-run against the literal it now has. + if (chained >= 0) { + if (HoldsContent(source, scan, chained, newContent)) + { + return PatchStatus.AlreadyApplied; + } + failReason = $"The call near line {lineHint} already has a {methodName} call. Re-run the test."; return PatchStatus.NotFound; } @@ -454,6 +466,31 @@ static PatchStatus TryAppend( return PatchStatus.Applied; } + /// + /// Whether the call at already carries + /// as its expected argument. + /// + /// What the argument means rather than what it says, so a literal the append would have + /// written in another shape - a different delimiter, a different indent - still counts as the + /// same snapshot. Anything that is not a literal at all, or is hidden behind another named + /// argument, is not this content: no answer can be read out of it, and the caller's other + /// branch says to re-run, which is where a call site nobody can make sense of belongs. + /// + /// + static bool HoldsContent(string source, SourceScan scan, int openParen, string content) + { + if (!TryReadArguments(source, scan, openParen, out var expected) || + expected.IsAbsent || + expected.BlockedByName) + { + return false; + } + + var argument = source.Substring(expected.Start, expected.End - expected.Start); + return scan.Language.TryParse(argument, out var value) && + value == content; + } + /// /// Removes the Snapshot call, along with the whitespace and line break that preceded it so no /// blank line is left behind. @@ -533,11 +570,13 @@ static PatchStatus TryRemove( /// Walks the calls chained onto an invocation and returns where a call should be appended: /// the end of the chain, or the point in front of the language's /// when the chain ends in one. - /// is set when one of them is a call to . + /// is the open paren of the first call to + /// among them, or -1 where there is none. The position rather than the fact of it, because a + /// caller deciding what to do about one has to read its argument. /// - static int WalkChain(string source, SourceScan scan, int index, string name, out bool found) + static int WalkChain(string source, SourceScan scan, int index, string name, out int found) { - found = false; + found = -1; var terminator = scan.Language.ChainTerminator; // Where the chain was before the terminating call, which is where an appended one goes: // in front of the terminator, and behind the whitespace and line break that introduced it @@ -569,9 +608,10 @@ static int WalkChain(string source, SourceScan scan, int index, string name, out break; } - if (IsCall(source, nameStart, cursor, name)) + if (found < 0 && + IsCall(source, nameStart, cursor, name)) { - found = true; + found = paren; } if (terminator != null && From 5620e43ace9fb233455f245662319254838ecdc6 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Thu, 27 Aug 2026 14:37:42 +1000 Subject: [PATCH 2/3] Move the AlreadyApplied paragraph into inline.source.md docs/inline.md is generated, so the paragraph was written into the output and the next build stripped it back out. It belongs in the mdsource the generator reads; the generated file is unchanged either way. --- docs/mdsource/inline.source.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/mdsource/inline.source.md b/docs/mdsource/inline.source.md index 73cabf63..f7bd40e3 100644 --- a/docs/mdsource/inline.source.md +++ b/docs/mdsource/inline.source.md @@ -204,6 +204,8 @@ For the staging fallback, where no viewer could be resolved and the patch is a f `Apply` returns `Applied`, `AlreadyApplied` (the literal already matches), `NotFound` (the source changed since the test run — tell the user to re-run rather than retrying), or a failure with a message (locked file, unreadable source), which is retryable. +`AlreadyApplied` covers an `Append` onto a call that already has a `Snapshot` call holding this same content, which is what a multi-targeted project transitioning to inline meets: every framework fails the call site and queues an append, and whichever is accepted first writes the literal the rest are carrying. Only a chained call holding *different* content is `NotFound` — that one genuinely cannot say what it wants until it has been re-run against the literal now in the source. Accepting one framework's append before the others have run does mean the queue never sees them together, so a real disagreement between frameworks is reported as that `NotFound` rather than as a conflict to pick from. + `Remove` mode patches are configuration changes with nothing to review: apply them directly; `AddInlineAsync` refuses them. From 9eaa6bfb7eb969a0baed593fdf5f98ae6e57b9c7 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Thu, 27 Aug 2026 14:38:19 +1000 Subject: [PATCH 3/3] Update Directory.Build.props --- src/Directory.Build.props | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Directory.Build.props b/src/Directory.Build.props index 82ac0e5c..fef1ff49 100644 --- a/src/Directory.Build.props +++ b/src/Directory.Build.props @@ -1,7 +1,7 @@ - 20.1.1 + 20.1.2 1.0.0 Testing, Snapshot, Diff, Compare Launches diff tools based on file extensions. Designed to be consumed by snapshot testing libraries.