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/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. 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 && 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.