From 86f1992e6dcdd523059e328764c7e2e2970b47ae Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Tue, 22 Sep 2026 16:14:07 +1000 Subject: [PATCH 1/3] Retire what the inline switch queued once it is turned off A snapshot the global switch inlines has no Snapshot call yet, so the patch it queues appends one. Turn the switch off and nothing in the source says that call site was ever inline, and RetireInline only runs where inline is in play, which is what spares a codebase that never used inline a round trip to the queue owner per verification. So the entry stayed pending for good, in the tray, the viewer or staged under obj, and review tooling went on offering it. Accepting it appended a Snapshot call that turned inline back on for that test, since an explicit Snapshot wins over the switch. The run that hands an appended patch over now records its call site under the intermediate directory, before the hand over, so a snapshot is never pending without a record. The first verification of a run with the switch off retires every recorded call site and deletes the records. By the line the entry was queued under, which is the key the owner holds it by, so there is no member fallback to reach a sibling call site's entry instead. Staged copies go with it, wherever an exiting owner or a run with no owner wrote them. Asking the owner what it holds would have found these too, but every run of every codebase would pay for the asking: on Windows a connect to a port nothing listens on waits out its timeout. With nothing recorded this costs one directory check per process. Per intermediate directory, so per configuration and target framework: a switch that is on for one framework and off for another does not have the second retiring what the first just queued. A retire answers to the switches a queue does, and a verification that cannot reach the owner leaves the records for one that can. Entries queued before this change have no record, so they are not retired. --- docs/inline-snapshots.md | 2 + docs/mdsource/inline-snapshots.source.md | 2 + src/StaticSettingsTests/BaseTest.cs | 12 ++ .../InlineSwitchOffTests.cs | 201 ++++++++++++++++++ src/Verify/Serialization/VerifierSettings.cs | 1 + src/Verify/Verifier/InlineEngine.cs | 35 ++- src/Verify/Verifier/InlineSwitchRecords.cs | 173 +++++++++++++++ src/Verify/Verifier/InnerVerifier_Inner.cs | 8 + 8 files changed, 432 insertions(+), 2 deletions(-) create mode 100644 src/StaticSettingsTests/InlineSwitchOffTests.cs create mode 100644 src/Verify/Verifier/InlineSwitchRecords.cs diff --git a/docs/inline-snapshots.md b/docs/inline-snapshots.md index f8fe057a5..47698569f 100644 --- a/docs/inline-snapshots.md +++ b/docs/inline-snapshots.md @@ -107,6 +107,8 @@ public static class ModuleInitializer Every `Verify*` then uses an inline snapshot, unless it is declined by one of the rules below, and accepting one appends the `.Snapshot(...)` call to the verify invocation. +Turning the switch off again withdraws what it left pending review. Snapshots the switch handed to DiffEngineTray, DiffEngineViewer or the staging directory, and that were never accepted, are dropped by the next test run, so review tooling does not go on offering to add a `.Snapshot(...)` call to a test that is back on `.verified.` files. + To decide per verification, pass a delegate: diff --git a/docs/mdsource/inline-snapshots.source.md b/docs/mdsource/inline-snapshots.source.md index a973a28f9..96b73e0c4 100644 --- a/docs/mdsource/inline-snapshots.source.md +++ b/docs/mdsource/inline-snapshots.source.md @@ -49,6 +49,8 @@ snippet: StaticInline Every `Verify*` then uses an inline snapshot, unless it is declined by one of the rules below, and accepting one appends the `.Snapshot(...)` call to the verify invocation. +Turning the switch off again withdraws what it left pending review. Snapshots the switch handed to DiffEngineTray, DiffEngineViewer or the staging directory, and that were never accepted, are dropped by the next test run, so review tooling does not go on offering to add a `.Snapshot(...)` call to a test that is back on `.verified.` files. + To decide per verification, pass a delegate: snippet: StaticInlineDelegate diff --git a/src/StaticSettingsTests/BaseTest.cs b/src/StaticSettingsTests/BaseTest.cs index 738479092..3034bb9aa 100644 --- a/src/StaticSettingsTests/BaseTest.cs +++ b/src/StaticSettingsTests/BaseTest.cs @@ -18,5 +18,17 @@ protected BaseTest() VerifierSettings.Reset(); CombinationSettings.Reset(); DerivePathInfo(PathInfo.DeriveDefault); + + // A test that queued with the switch on leaves a record in obj, and the next verification + // with the switch off would retire it through whatever owns the queue on this machine + if (Directory.Exists(InlineSwitchRecordsDirectory)) + { + Directory.Delete(InlineSwitchRecordsDirectory, true); + } } + + protected static string InlineSwitchRecordsDirectory => + Path.Combine( + AttributeReader.GetIntermediateDirectory(typeof(BaseTest).Assembly), + InlineSwitchRecords.DirectoryName); } \ No newline at end of file diff --git a/src/StaticSettingsTests/InlineSwitchOffTests.cs b/src/StaticSettingsTests/InlineSwitchOffTests.cs new file mode 100644 index 000000000..ac627fc56 --- /dev/null +++ b/src/StaticSettingsTests/InlineSwitchOffTests.cs @@ -0,0 +1,201 @@ +using DiffEngine; + +// Lives here, rather than in Verify.Tests, since the inline switch is a static setting. This +// project runs serially with BaseTest resetting it between tests, and resetting it part way through +// a test is what stands in for the next run, with the switch taken out of the module initializer. +// +// A snapshot the switch inlines has no Snapshot call yet, so the patch it queues appends one. Once +// the switch is off no verification knows that call site was ever inline, so without a record of it +// the entry stayed pending for good, and accepting it turned inline back on for that test. +public class InlineSwitchOffTests : + BaseTest, + IDisposable +{ + List queued = []; + List retired = []; + Func> originalAddInline = InlineEngine.AddInline; + Action originalSendRetire = InlineEngine.SendRetire; + bool originalDisabled = DiffRunner.Disabled; + TempDirectory temp = new(); + + public InlineSwitchOffTests() + { + // Both stood in for: the call sites are real ones in this file, and whatever owns the queue + // on this machine would otherwise be sent them + InlineEngine.AddInline = _ => + { + queued.Add(_); + return Task.FromResult(InlineResult.Queued); + }; + InlineEngine.SendRetire = Retire; + + // The ambient values feed diffEnabled, and a retire answers to the same switches, so pin + // both rather than pass or fail by whether the run is on a build server or under an AI CLI + BuildServerDetector.Detected = false; + DiffRunner.Disabled = false; + } + + void Retire(string sourceFile, int line, string? memberName) => + retired.Add(new(sourceFile, line, memberName)); + + // BaseTest restores BuildServerDetector.Detected and deletes the records for every test, so only + // the seams and DiffRunner need undoing + public void Dispose() + { + InlineEngine.AddInline = originalAddInline; + InlineEngine.SendRetire = originalSendRetire; + DiffRunner.Disabled = originalDisabled; + temp.Dispose(); + } + + [Fact] + public async Task SwitchingOffRetiresWhatTheSwitchQueued() + { + VerifierSettings.Inline(); + await Assert.ThrowsAsync(() => Verify("value")); + var patch = Assert.Single(queued); + Assert.Equal(InlinePatchMode.Append, patch.Mode); + Assert.Single(Records()); + + // The next run, with the switch gone from the module initializer + VerifierSettings.Reset(); + await Verify("value", PassingSettings()); + + var retire = Assert.Single(retired); + Assert.Equal(patch.SourceFile, retire.SourceFile); + Assert.Equal(patch.LineHint, retire.Line); + // The record holds the line the entry was queued under, which is the key the owner holds it + // by. So no member, which is what could reach a sibling call site's entry instead + Assert.Null(retire.MemberName); + Assert.Empty(Records()); + } + + /// + /// A queued snapshot does not always stay in the queue: an owner on its way out writes what it + /// holds under the source project's obj, which accept tooling reads just as it reads the queue. + /// So the retire has to reach those files as well. + /// + [Fact] + public async Task SwitchingOffClearsWhatTheSwitchLeftOnDisk() + { + VerifierSettings.Inline(); + await Assert.ThrowsAsync(() => Verify("value")); + var patch = Assert.Single(queued); + + var staging = Path.Combine( + AttributeReader.GetProjectDirectory(typeof(InlineSwitchOffTests).Assembly), + "obj", + InlineStaging.DirectoryName); + Directory.CreateDirectory(staging); + var stem = nameof(SwitchingOffClearsWhatTheSwitchLeftOnDisk); + var patchFile = Path.Combine(staging, $"{stem}.inlinepatch"); + var receivedFile = Path.Combine(staging, $"{stem}.received.txt"); + var expectedFile = Path.Combine(staging, $"{stem}.expected.txt"); + InlinePatchFile.Write(patchFile, patch); + await File.WriteAllTextAsync(receivedFile, patch.NewContent); + await File.WriteAllTextAsync(expectedFile, ""); + + VerifierSettings.Reset(); + await Verify("value", PassingSettings()); + + Assert.False(File.Exists(patchFile)); + Assert.False(File.Exists(receivedFile)); + Assert.False(File.Exists(expectedFile)); + } + + /// + /// While the switch is on, what it queued is pending for a reason: the call site is still an + /// inline snapshot, waiting to be accepted. + /// + [Fact] + public async Task SwitchStillOnKeepsTheRecord() + { + VerifierSettings.Inline(); + await Assert.ThrowsAsync(() => Verify("value")); + var patch = Assert.Single(queued); + + // The next run, with the switch still on. NotInline so this verification passes as a file, + // which retires its own call site and nothing else + VerifierSettings.Reset(); + VerifierSettings.Inline(); + var settings = PassingSettings(); + settings.NotInline(); + await Verify("value", settings); + + Assert.DoesNotContain(retired, _ => _.Line == patch.LineHint); + Assert.Single(Records()); + } + + /// + /// Every verification of a codebase that never turned the switch on comes through here, so with + /// nothing recorded nothing may reach the queue owner. + /// + [Fact] + public async Task NothingRecordedRetiresNothing() + { + await Verify("value", PassingSettings()); + + Assert.Empty(retired); + } + + /// + /// A verification that may not reach the queue owner leaves the records for one that can, + /// rather than dropping them unretired. + /// + [Fact] + public async Task DiffDisabledLeavesTheRecordsForALaterVerification() + { + VerifierSettings.Inline(); + await Assert.ThrowsAsync(() => Verify("value")); + var patch = Assert.Single(queued); + + VerifierSettings.Reset(); + var disabled = PassingSettings(); + disabled.DisableDiff(); + await Verify("value", disabled); + + Assert.Empty(retired); + Assert.Single(Records()); + + await Verify("value", PassingSettings()); + + Assert.Equal(patch.LineHint, Assert.Single(retired).Line); + Assert.Empty(Records()); + } + + /// + /// Only the switch appends. A patch that sets an argument belongs to a Snapshot call the source + /// still holds, and that call site settles or retires itself. + /// + [Fact] + public async Task AnExplicitSnapshotIsNotRecorded() + { + var settings = new VerifySettings(); + settings.UseDirectory(temp); + // Deliberately not this file, as in InlineQueueTests + settings.Snapshot("wrong", temp.BuildPath("Fake.cs"), 1, "\"wrong\""); + + await Assert.ThrowsAsync(() => Verify("value", settings)); + + Assert.Equal(InlinePatchMode.Set, Assert.Single(queued).Mode); + Assert.Empty(Records()); + } + + // A file verification that passes. A failing one hands a pending move to whatever owns the queue + // on this machine, so AutoVerify accepts it into the temp directory instead + VerifySettings PassingSettings() + { + var settings = new VerifySettings(); + settings.UseDirectory(temp); + settings.AutoVerify(); + settings.DisableRequireUniquePrefix(); + return settings; + } + + static List Records() => + Directory.Exists(InlineSwitchRecordsDirectory) + ? Directory.EnumerateFiles(InlineSwitchRecordsDirectory).ToList() + : []; + + sealed record Retired(string SourceFile, int Line, string? MemberName); +} diff --git a/src/Verify/Serialization/VerifierSettings.cs b/src/Verify/Serialization/VerifierSettings.cs index 7e11184d4..2df4c92c6 100644 --- a/src/Verify/Serialization/VerifierSettings.cs +++ b/src/Verify/Serialization/VerifierSettings.cs @@ -164,6 +164,7 @@ internal static void Reset() inlineMaxLines = null; inlineApplyMaxLinesToExisting = false; inlineEntryPoints = null; + InlineSwitchRecords.Reset(); UniquePrefixDisabled = false; UseUniqueDirectorySplitMode = false; omitContentFromException = false; diff --git a/src/Verify/Verifier/InlineEngine.cs b/src/Verify/Verifier/InlineEngine.cs index 35f77484a..71f0acbd4 100644 --- a/src/Verify/Verifier/InlineEngine.cs +++ b/src/Verify/Verifier/InlineEngine.cs @@ -18,6 +18,12 @@ class InlineEngine( /// internal static Func> AddInline = _ => DiffRunner.AddInlineAsync(_); + /// + /// Swapped in tests, as is: what a retire told the queue owner is + /// otherwise only observable from the owner. + /// + internal static Action SendRetire = DiffRunner.RetireInline; + /// /// Swapped in tests. Every source rewrite below is a no-op on a build server, so the tests that /// cover those rewrites need the check off. Scoped to inline rather than moving @@ -140,7 +146,9 @@ public static void RecordInline(string mappedSourceFile, string? memberName) /// Drops whatever a previous run queued for a call site that is no longer an inline snapshot: /// NotInline, a global switch that declined it, or a literal that outgrew the size /// limit. Nothing here compares anything, so this is static and needs no engine — there is no - /// inline verification to build one for, which is the whole point. + /// inline verification to build one for, which is the whole point. A switch turned off + /// altogether leaves no verification knowing it was ever on, so those call sites go through + /// instead. /// /// /// Without this the entry outlives the decision that made it meaningless. Settling only ever @@ -177,10 +185,25 @@ public static void Retire(VerifySettings settings, string? sourceFile, int line, inlinedMembers.ContainsKey($"{mapped}|{memberName}") ? null : memberName; - DiffRunner.RetireInline(mapped, line, member); + SendRetire(mapped, line, member); ClearStaged(mapped, line, member); } + /// + /// Drops a call site the global switch queued while it was on, now that it is off. See + /// . + /// + /// + /// No member. The record holds the line the entry was queued under, which is the key the owner + /// holds it by, so there is no drift for the member to recover from — and the member is what + /// can reach a sibling call site's entry instead. + /// + public static void RetireRecorded(string mappedSourceFile, int line) + { + SendRetire(mappedSourceFile, line, null); + ClearStaged(mappedSourceFile, line, null); + } + /// /// Rewrites the source in place. Only used by auto verify, and by the migration away from /// inline, both of which are decided rather than reviewed. @@ -212,6 +235,14 @@ public bool TryApply() return (null, null); } + // An appended snapshot is the global switch's, and once the switch is off nothing in the + // source says this call site was ever inline. Recorded before it is handed over, so it is + // never pending without a record + if (inline.Mode == InlinePatchMode.Append) + { + InlineSwitchRecords.Write(MappedSourceFile, inline.Line); + } + var result = await AddInline(BuildPatch()); if (result != InlineResult.NoViewerFound) { diff --git a/src/Verify/Verifier/InlineSwitchRecords.cs b/src/Verify/Verifier/InlineSwitchRecords.cs new file mode 100644 index 000000000..74cfb47b6 --- /dev/null +++ b/src/Verify/Verifier/InlineSwitchRecords.cs @@ -0,0 +1,173 @@ +/// +/// The call sites the global inline switch handed over for review, recorded so a run with the +/// switch turned off can retire them. +/// +/// +/// A snapshot the switch inlines has no Snapshot call yet, so the patch it hands over appends one. +/// Once the switch is off, nothing in the source says that call site was ever inline, and a +/// verification only retires where inline is in play - which is what spares a codebase that never +/// turned the switch on a round trip to the queue owner per verification. So the entry stayed +/// pending for good, in the queue or staged on disk, and accepting it appended a Snapshot call that +/// turned inline back on for that test, since an explicit Snapshot wins over the switch. +/// +/// Asking the owner what it holds would find these too, but every run of every codebase would pay +/// for the asking: nothing owning the queue is the ordinary state of a machine with no tray, and on +/// Windows a connect to a port nothing listens on waits out its timeout rather than being refused. +/// So the run that hands a patch over records the call site, and the first verification of a run +/// with the switch off retires whatever was recorded. Where nothing was, that costs one directory +/// check per process. +/// +/// +/// Kept in the intermediate directory, which is per configuration and target framework, so a +/// switch that is on for one framework and off for another does not have the second retiring what +/// the first has just queued. Removed whenever obj is cleaned. +/// +/// +static class InlineSwitchRecords +{ + internal const string DirectoryName = "VerifyInlineSwitch"; + + static Lock locker = new(); + static volatile bool retired; + + /// + /// Records a call site whose snapshot is being handed over with a patch that appends a Snapshot + /// call. Only the switch produces those, so only its call sites are recorded. + /// + public static void Write(string sourceFile, int line) + { + var intermediate = VerifierSettings.IntermediateDir; + if (intermediate is null) + { + // The project does not consume Verify's build props, so the obj directory is unknown. + return; + } + + try + { + var directory = Path.Combine(intermediate, DirectoryName); + Directory.CreateDirectory(directory); + // Named by call site, so a re run overwrites the same record instead of accumulating + var path = Path.Combine(directory, $"{Fnv1a.Hash($"{sourceFile}:{line}")}.txt"); + File.WriteAllText(path, $"{sourceFile}{Environment.NewLine}{line}"); + } + catch + { + // Only ever used to clean up after the switch, so failing to write one must not change + // the test outcome. + } + } + + /// + /// Retires every recorded call site, once per process, when the switch is off. Called by every + /// verification before it can queue, settle or retire anything of its own, so a record cannot + /// retire an entry this run has just queued. + /// + public static void RetireIfSwitchedOff(VerifySettings settings) + { + if (retired || + // On, so whatever is recorded is still pending for a reason + VerifierSettings.inline is not null || + // A retire tells the queue owner something, so it answers to the same switches a queue + // does. Not marked done, so a later verification that can reach the owner still retires + DiffRunner.Disabled || + !settings.diffEnabled || + InlineEngine.IsBuildServer()) + { + return; + } + + using (locker.EnterScope()) + { + if (retired) + { + return; + } + + RetireRecorded(); + retired = true; + } + } + + static void RetireRecorded() + { + var intermediate = VerifierSettings.IntermediateDir; + if (intermediate is null) + { + return; + } + + var directory = Path.Combine(intermediate, DirectoryName); + if (!Directory.Exists(directory)) + { + return; + } + + foreach (var record in Records(directory)) + { + if (TryRead(record, out var sourceFile, out var line)) + { + InlineEngine.RetireRecorded(sourceFile, line); + } + + TryDelete(record); + } + } + + static string[] Records(string directory) + { + try + { + return Directory.GetFiles(directory, "*.txt"); + } + catch (Exception exception) + when (exception is IOException or UnauthorizedAccessException) + { + return []; + } + } + + static bool TryRead(string record, [NotNullWhen(true)] out string? sourceFile, out int line) + { + sourceFile = null; + line = 0; + + string[] lines; + try + { + lines = File.ReadAllLines(record); + } + catch (Exception exception) + when (exception is IOException or UnauthorizedAccessException) + { + return false; + } + + if (lines.Length < 2 || + lines[0].Length == 0 || + !int.TryParse(lines[1], out line) || + line < 1) + { + return false; + } + + sourceFile = lines[0]; + return true; + } + + static void TryDelete(string record) + { + try + { + File.Delete(record); + } + catch (Exception exception) + when (exception is IOException or UnauthorizedAccessException) + { + // A record left behind is retired again by a later run, which finds nothing to retire. + } + } + + internal static void Reset() => + retired = false; +} diff --git a/src/Verify/Verifier/InnerVerifier_Inner.cs b/src/Verify/Verifier/InnerVerifier_Inner.cs index 711dabffe..efe62cb69 100644 --- a/src/Verify/Verifier/InnerVerifier_Inner.cs +++ b/src/Verify/Verifier/InnerVerifier_Inner.cs @@ -26,6 +26,10 @@ async Task VerifyInner(object? root, Func? cleanup, IEnumera throw new("All targets have been excluded by ExcludeTargets. A verification requires at least one target."); } + // Before this verification can queue, settle or retire anything of its own, so what a run + // with the switch on left pending is dealt with first + InlineSwitchRecords.RetireIfSwitchedOff(settings); + var inline = ResolveInline(resultTargets); InlineEngine? inlineEngine = null; string? migratedExpected = null; @@ -116,6 +120,10 @@ async Task VerifyInner(object? root, Func? cleanup, IEnumera /// Gated on inline being in play at all, so a codebase that never turned it on never pays a /// loopback round trip per verification. Where a Snapshot call is still there but declined, /// its own call site is the one to retire; otherwise it is the verify call's. + /// + /// A global switch that has been turned off is not in play, so what it queued while it was on + /// is never reached from here. retires those. + /// /// void RetireInline() { From 50e9e10e12ae8d1d4fec667b4dfca2c8bd830c4e Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Tue, 22 Sep 2026 18:16:06 +1000 Subject: [PATCH 2/3] Keep switch records honest about what they can retire A record that could not be read was deleted as if malformed, stranding the entry it named; an unreadable one is now left for a later run. A record whose retire threw took every later verification of the process down with it, since neither the once-per-process flag nor the delete was reached; the retire is now guarded per record and the flag set regardless. The flag also latched when the intermediate directory was not yet known, which a verification through the raw api can reach; it no longer does. A record outlived its call site: nothing deleted it once an explicit Snapshot call was accepted there, and the switch-off retire, keyed by line alone, then dropped that call's own pending snapshot. A switched-on verification that finds the call site is no longer one the switch appends to now forgets the record. The remarks and docs claimed more than the mechanism gives: per-framework isolation only for a framework whose switch was never on, records surviving a Clean target but not an obj wipe, nothing recorded for a project without an intermediate directory, and a running owner that does not answer being the one send whose record is lost. --- docs/inline-snapshots.md | 2 +- docs/mdsource/inline-snapshots.source.md | 2 +- .../InlineSwitchOffTests.cs | 57 ++++++++ src/Verify/Verifier/InlineEngine.cs | 5 +- src/Verify/Verifier/InlineSwitchRecords.cs | 136 ++++++++++++++---- src/Verify/Verifier/InnerVerifier_Inner.cs | 11 ++ 6 files changed, 183 insertions(+), 30 deletions(-) diff --git a/docs/inline-snapshots.md b/docs/inline-snapshots.md index 47698569f..f57c0917b 100644 --- a/docs/inline-snapshots.md +++ b/docs/inline-snapshots.md @@ -107,7 +107,7 @@ public static class ModuleInitializer Every `Verify*` then uses an inline snapshot, unless it is declined by one of the rules below, and accepting one appends the `.Snapshot(...)` call to the verify invocation. -Turning the switch off again withdraws what it left pending review. Snapshots the switch handed to DiffEngineTray, DiffEngineViewer or the staging directory, and that were never accepted, are dropped by the next test run, so review tooling does not go on offering to add a `.Snapshot(...)` call to a test that is back on `.verified.` files. +Turning the switch off again withdraws what it left pending review. Snapshots the switch handed to DiffEngineTray, DiffEngineViewer or the staging directory, and that were never accepted, are dropped by the next test run, so review tooling does not go on offering to add a `.Snapshot(...)` call to a test that is back on `.verified.` files. The run that drops them has to have diff tooling enabled (so not a build server, and not `DiffEngine_Disabled`), and to build the same configuration and target framework as the run that queued them: the call sites are recorded under the project's intermediate directory, which a project that does not consume Verify's build props does not have, and which deleting `obj` removes. Snapshots queued by a version of Verify before this recording stay pending. To decide per verification, pass a delegate: diff --git a/docs/mdsource/inline-snapshots.source.md b/docs/mdsource/inline-snapshots.source.md index 96b73e0c4..ffc79fd62 100644 --- a/docs/mdsource/inline-snapshots.source.md +++ b/docs/mdsource/inline-snapshots.source.md @@ -49,7 +49,7 @@ snippet: StaticInline Every `Verify*` then uses an inline snapshot, unless it is declined by one of the rules below, and accepting one appends the `.Snapshot(...)` call to the verify invocation. -Turning the switch off again withdraws what it left pending review. Snapshots the switch handed to DiffEngineTray, DiffEngineViewer or the staging directory, and that were never accepted, are dropped by the next test run, so review tooling does not go on offering to add a `.Snapshot(...)` call to a test that is back on `.verified.` files. +Turning the switch off again withdraws what it left pending review. Snapshots the switch handed to DiffEngineTray, DiffEngineViewer or the staging directory, and that were never accepted, are dropped by the next test run, so review tooling does not go on offering to add a `.Snapshot(...)` call to a test that is back on `.verified.` files. The run that drops them has to have diff tooling enabled (so not a build server, and not `DiffEngine_Disabled`), and to build the same configuration and target framework as the run that queued them: the call sites are recorded under the project's intermediate directory, which a project that does not consume Verify's build props does not have, and which deleting `obj` removes. Snapshots queued by a version of Verify before this recording stay pending. To decide per verification, pass a delegate: diff --git a/src/StaticSettingsTests/InlineSwitchOffTests.cs b/src/StaticSettingsTests/InlineSwitchOffTests.cs index ac627fc56..7ac3dddc4 100644 --- a/src/StaticSettingsTests/InlineSwitchOffTests.cs +++ b/src/StaticSettingsTests/InlineSwitchOffTests.cs @@ -181,6 +181,63 @@ public async Task AnExplicitSnapshotIsNotRecorded() Assert.Empty(Records()); } + /// + /// A record lives only while its call site is one the switch appends to. Once a switched-on + /// verification at that call site is not appended to any more, the record would only ever + /// retire whatever sits on that line at switch off, which can be an explicit Snapshot call's + /// own pending snapshot. + /// + [Fact] + public async Task ACallSiteTheSwitchNoLongerAppendsToForgetsItsRecord() + { + // One call site for both runs, since the record is keyed by the line + for (var run = 0; run < 2; run++) + { + VerifierSettings.Reset(); + VerifierSettings.Inline(); + VerifySettings? settings = null; + if (run == 1) + { + // The next run, still on, with the call site now declined + settings = PassingSettings(); + settings.NotInline(); + } + + var verification = Verify("value", settings ?? new()); + if (run == 0) + { + await Assert.ThrowsAsync(() => verification); + Assert.Single(Records()); + continue; + } + + await verification; + } + + Assert.Empty(Records()); + } + + /// + /// A record that cannot be read is not a malformed one: deleting it would strand the entry it + /// names for good, so it is left for a run that can read it. + /// + [Fact] + public async Task AnUnreadableRecordIsLeftForALaterRun() + { + VerifierSettings.Inline(); + await Assert.ThrowsAsync(() => Verify("value")); + var record = Assert.Single(Records()); + + VerifierSettings.Reset(); + using (new FileStream(record, FileMode.Open, FileAccess.Read, FileShare.Delete)) + { + await Verify("value", PassingSettings()); + } + + Assert.Empty(retired); + Assert.Single(Records()); + } + // A file verification that passes. A failing one hands a pending move to whatever owns the queue // on this machine, so AutoVerify accepts it into the temp directory instead VerifySettings PassingSettings() diff --git a/src/Verify/Verifier/InlineEngine.cs b/src/Verify/Verifier/InlineEngine.cs index 71f0acbd4..04cf5e70f 100644 --- a/src/Verify/Verifier/InlineEngine.cs +++ b/src/Verify/Verifier/InlineEngine.cs @@ -236,8 +236,9 @@ public bool TryApply() } // An appended snapshot is the global switch's, and once the switch is off nothing in the - // source says this call site was ever inline. Recorded before it is handed over, so it is - // never pending without a record + // source says this call site was ever inline. Recorded before it is handed over, so a + // hand over that succeeds is never ahead of its record. The write is best effort, and a + // project with no intermediate directory has nowhere to write: those entries stay pending if (inline.Mode == InlinePatchMode.Append) { InlineSwitchRecords.Write(MappedSourceFile, inline.Line); diff --git a/src/Verify/Verifier/InlineSwitchRecords.cs b/src/Verify/Verifier/InlineSwitchRecords.cs index 74cfb47b6..e3560bd1f 100644 --- a/src/Verify/Verifier/InlineSwitchRecords.cs +++ b/src/Verify/Verifier/InlineSwitchRecords.cs @@ -18,9 +18,25 @@ /// check per process. /// /// +/// A record lives only while its call site is still one the switch would append to. A later +/// switched-on verification that finds an explicit Snapshot there, or declines the call site, +/// forgets the record: an accepted Snapshot call is keyed by its own line, and a stale record on +/// that line would otherwise retire the explicit call's next pending snapshot. +/// +/// /// Kept in the intermediate directory, which is per configuration and target framework, so a -/// switch that is on for one framework and off for another does not have the second retiring what -/// the first has just queued. Removed whenever obj is cleaned. +/// framework whose switch was never on retires nothing another framework queued. The retire itself +/// names no framework, since the statement is that the call site is not inline any more: a +/// framework whose switch was on and is then turned off retires the whole entry, and a framework +/// still on re-queues on its next run. The directory survives a Clean target, which removes only +/// build outputs, but not an obj wipe: entries queued before that have no record and stay pending. +/// +/// +/// A project that does not consume Verify's build props has no intermediate directory, so nothing +/// is recorded and nothing is retired there; the same goes for a record whose write failed. And a +/// retire that reached nobody deletes its record all the same: an owner that has exited persisted +/// its queue to disk, where the staging clear finds it, and an owner that is running but did not +/// answer is the one case a record is lost for, since the send reports no outcome. /// /// static class InlineSwitchRecords @@ -36,19 +52,14 @@ static class InlineSwitchRecords /// public static void Write(string sourceFile, int line) { - var intermediate = VerifierSettings.IntermediateDir; - if (intermediate is null) + if (RecordPath(sourceFile, line) is not { } path) { - // The project does not consume Verify's build props, so the obj directory is unknown. return; } try { - var directory = Path.Combine(intermediate, DirectoryName); - Directory.CreateDirectory(directory); - // Named by call site, so a re run overwrites the same record instead of accumulating - var path = Path.Combine(directory, $"{Fnv1a.Hash($"{sourceFile}:{line}")}.txt"); + Directory.CreateDirectory(Path.GetDirectoryName(path)!); File.WriteAllText(path, $"{sourceFile}{Environment.NewLine}{line}"); } catch @@ -58,6 +69,47 @@ public static void Write(string sourceFile, int line) } } + /// + /// Drops the record for a call site the switch no longer appends to: an explicit Snapshot call + /// is there now, or the switch declined it. Only called while the switch is on, so a codebase + /// that never turned it on never pays the file check. + /// + public static void Forget(string sourceFile, int line) + { + if (RecordPath(sourceFile, line) is not { } path) + { + return; + } + + try + { + if (File.Exists(path)) + { + File.Delete(path); + } + } + catch (Exception exception) + when (exception is IOException or UnauthorizedAccessException) + { + // Left for the switched-off run, which retires a call site the owner no longer holds. + } + } + + /// + /// Named by call site, so a re run overwrites the same record instead of accumulating. Null + /// when the project does not consume Verify's build props, so the obj directory is unknown. + /// + static string? RecordPath(string sourceFile, int line) + { + var intermediate = VerifierSettings.IntermediateDir; + if (intermediate is null) + { + return null; + } + + return Path.Combine(intermediate, DirectoryName, $"{Fnv1a.Hash($"{sourceFile}:{line}")}.txt"); + } + /// /// Retires every recorded call site, once per process, when the switch is off. Called by every /// verification before it can queue, settle or retire anything of its own, so a record cannot @@ -84,20 +136,28 @@ VerifierSettings.inline is not null || return; } - RetireRecorded(); - retired = true; + // The intermediate directory is assigned by the adapters, so a verification through the + // raw InnerVerifier api can get here before it is known. Not marked done then either + if (VerifierSettings.IntermediateDir is not { } intermediate) + { + return; + } + + try + { + RetireRecorded(Path.Combine(intermediate, DirectoryName)); + } + finally + { + // Whatever went wrong is paid once. Cleaning up after the switch must not change a + // test outcome, and certainly not every test outcome in the process + retired = true; + } } } - static void RetireRecorded() + static void RetireRecorded(string directory) { - var intermediate = VerifierSettings.IntermediateDir; - if (intermediate is null) - { - return; - } - - var directory = Path.Combine(intermediate, DirectoryName); if (!Directory.Exists(directory)) { return; @@ -105,9 +165,26 @@ static void RetireRecorded() foreach (var record in Records(directory)) { - if (TryRead(record, out var sourceFile, out var line)) + var read = TryRead(record, out var sourceFile, out var line); + if (read == RecordRead.Unreadable) + { + // A transient failure to read is not a malformed record. Left for a later run, + // since deleting it would strand the entry it names for good + continue; + } + + try + { + if (read == RecordRead.Read) + { + InlineEngine.RetireRecorded(sourceFile!, line); + } + } + catch (Exception exception) + when (exception is not OutOfMemoryException) { - InlineEngine.RetireRecorded(sourceFile, line); + // A record that cannot be retired never becomes retirable, so it is deleted below + // rather than throwing into every verification of the process } TryDelete(record); @@ -127,7 +204,14 @@ static string[] Records(string directory) } } - static bool TryRead(string record, [NotNullWhen(true)] out string? sourceFile, out int line) + enum RecordRead + { + Read, + Malformed, + Unreadable + } + + static RecordRead TryRead(string record, out string? sourceFile, out int line) { sourceFile = null; line = 0; @@ -140,19 +224,19 @@ static bool TryRead(string record, [NotNullWhen(true)] out string? sourceFile, o catch (Exception exception) when (exception is IOException or UnauthorizedAccessException) { - return false; + return RecordRead.Unreadable; } if (lines.Length < 2 || - lines[0].Length == 0 || + lines[0].Trim().Length == 0 || !int.TryParse(lines[1], out line) || line < 1) { - return false; + return RecordRead.Malformed; } sourceFile = lines[0]; - return true; + return RecordRead.Read; } static void TryDelete(string record) diff --git a/src/Verify/Verifier/InnerVerifier_Inner.cs b/src/Verify/Verifier/InnerVerifier_Inner.cs index efe62cb69..9c8171b48 100644 --- a/src/Verify/Verifier/InnerVerifier_Inner.cs +++ b/src/Verify/Verifier/InnerVerifier_Inner.cs @@ -66,6 +66,17 @@ async Task VerifyInner(object? root, Func? cleanup, IEnumera ValidatePrefix(settings, pathPrefixReceived!); } + // The switch is on but this call site is not one it appends to any more: an explicit + // Snapshot call is there now, or the switch declined it. Its record would otherwise outlive + // the entry it was written for and retire whatever sits on that line at switch off + if (VerifierSettings.inline is not null && + inline?.Mode != InlinePatchMode.Append && + inlineSourceFile is not null && + lineNumber != 0) + { + InlineSwitchRecords.Forget(MapSourceFile(inlineSourceFile), lineNumber); + } + if (inline is null) { RetireInline(); From bd2cdcbffc705fca0f53c8e5d4796d5d08823b0b Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Tue, 22 Sep 2026 20:03:00 +1000 Subject: [PATCH 3/3] Pin what moving off inline retires Breaking the switch records and retire paths on purpose showed most of what they promise was held by no test. A record outliving the Snapshot call its snapshot was accepted into, a call site the switch declines keeping its pending entry, a declined Snapshot call retiring the verify call's line rather than its own, a retire that throws failing the verification, and the retire running after the run's own queue all went unnoticed. So did retiring on a build server or with DiffRunner disabled, keeping malformed records, and skipping a record whose test has since been deleted. Each now has a test that fails when it breaks. The declined Snapshot call is asserted on the wire in InlineRetireTests, since it does not involve the switch. --- .../InlineSwitchOffTests.cs | 244 ++++++++++++++++++ src/Verify.Tests/InlineRetireTests.cs | 27 ++ 2 files changed, 271 insertions(+) diff --git a/src/StaticSettingsTests/InlineSwitchOffTests.cs b/src/StaticSettingsTests/InlineSwitchOffTests.cs index 7ac3dddc4..3245ad40b 100644 --- a/src/StaticSettingsTests/InlineSwitchOffTests.cs +++ b/src/StaticSettingsTests/InlineSwitchOffTests.cs @@ -103,6 +103,77 @@ public async Task SwitchingOffClearsWhatTheSwitchLeftOnDisk() Assert.False(File.Exists(expectedFile)); } + /// + /// A test deleted while its snapshot was pending leaves nothing to verify at its call site, so + /// the record is all that still names the entry. The retire does not consult the source, and + /// must not start to: a verify call at the recorded line is exactly what a deleted test lacks. + /// + [Fact] + public async Task ARecordWhoseTestWasDeletedIsStillRetired() + { + // The file after the delete, with no verify call left at the recorded line + var source = temp.BuildPath("Tests.cs"); + await File.WriteAllTextAsync( + source, + """ + class Tests + { + [Fact] + public Task Remaining() => + Task.CompletedTask; + } + """); + InlineSwitchRecords.Write(source, 5); + + await Verify("value", PassingSettings()); + + var retire = Assert.Single(retired); + Assert.Equal(source, retire.SourceFile); + Assert.Equal(5, retire.Line); + Assert.Empty(Records()); + } + + /// + /// A switched-off run retires before its first verification can queue anything, so a record + /// cannot drop an entry that run has just queued under the same key: here an explicit Snapshot + /// call that now sits on the recorded line, failing against its literal. + /// + [Fact] + public async Task ARecordCannotRetireWhatTheSwitchedOffRunQueues() + { + var events = new List(); + InlinePatch? appended = null; + InlineEngine.AddInline = patch => + { + appended ??= patch; + events.Add($"queue {patch.LineHint}"); + return Task.FromResult(InlineResult.Queued); + }; + InlineEngine.SendRetire = (_, line, _) => events.Add($"retire {line}"); + + // One call site for both runs, since the record is keyed by the line + for (var run = 0; run < 2; run++) + { + VerifierSettings.Reset(); + var settings = new VerifySettings(); + if (run == 0) + { + VerifierSettings.Inline(); + } + else + { + // The next run, switched off. This file and line, so the patch is queued under the + // key the record names, which the seams keep from whatever owns the queue here + settings.Snapshot("old", appended!.SourceFile, appended.LineHint, "\"old\""); + } + + await Assert.ThrowsAsync(() => Verify("value", settings)); + } + + var line = appended!.LineHint; + Assert.Equal([$"queue {line}", $"retire {line}", $"queue {line}"], events); + } + /// /// While the switch is on, what it queued is pending for a reason: the call site is still an /// inline snapshot, waiting to be accepted. @@ -163,6 +234,58 @@ public async Task DiffDisabledLeavesTheRecordsForALaterVerification() Assert.Empty(Records()); } + /// + /// As , with diff off for the + /// whole process, which DiffEngine does itself under continuous testing and an AI CLI. The + /// settings are built first, since they read the same switch, and this is about the check that + /// reads it directly. + /// + [Fact] + public async Task DiffRunnerDisabledLeavesTheRecordsForALaterVerification() + { + VerifierSettings.Inline(); + await Assert.ThrowsAsync(() => Verify("value")); + var patch = Assert.Single(queued); + + VerifierSettings.Reset(); + var settings = PassingSettings(); + DiffRunner.Disabled = true; + await Verify("value", settings); + + Assert.Empty(retired); + Assert.Single(Records()); + + DiffRunner.Disabled = false; + await Verify("value", PassingSettings()); + + Assert.Equal(patch.LineHint, Assert.Single(retired).Line); + Assert.Empty(Records()); + } + + /// + /// As , on a build server. + /// + [Fact] + public async Task ABuildServerLeavesTheRecordsForALaterVerification() + { + VerifierSettings.Inline(); + await Assert.ThrowsAsync(() => Verify("value")); + var patch = Assert.Single(queued); + + VerifierSettings.Reset(); + BuildServerDetector.Detected = true; + await Verify("value", PassingSettings()); + + Assert.Empty(retired); + Assert.Single(Records()); + + BuildServerDetector.Detected = false; + await Verify("value", PassingSettings()); + + Assert.Equal(patch.LineHint, Assert.Single(retired).Line); + Assert.Empty(Records()); + } + /// /// Only the switch appends. A patch that sets an argument belongs to a Snapshot call the source /// still holds, and that call site settles or retires itself. @@ -217,6 +340,84 @@ public async Task ACallSiteTheSwitchNoLongerAppendsToForgetsItsRecord() Assert.Empty(Records()); } + /// + /// The case the forget exists for: the snapshot was accepted, so an explicit Snapshot call is + /// at the call site now, and with the switch still on it is compared rather than appended to. + /// + [Fact] + public async Task AnAcceptedSnapshotAtTheCallSiteForgetsItsRecord() + { + // One call site for both runs, since the record is keyed by the line + for (var run = 0; run < 2; run++) + { + VerifierSettings.Reset(); + VerifierSettings.Inline(); + var settings = new VerifySettings(); + if (run == 1) + { + // The next run, still on, with the snapshot accepted. Deliberately not this file, as + // in AnExplicitSnapshotIsNotRecorded, and diff off so the passing comparison settles + // nothing with whatever owns the queue on this machine + settings.Snapshot("value", temp.BuildPath("Fake.cs"), 1, "\"value\""); + settings.DisableDiff(); + } + + var verification = Verify("value", settings); + if (run == 0) + { + await Assert.ThrowsAsync(() => verification); + Assert.Single(Records()); + continue; + } + + await verification; + } + + Assert.Empty(Records()); + } + + /// + /// A call site the switch stops inlining without NotInline: here its delegate declines it, and + /// the same goes for a parameter, the size limit or a binary first target. What it queued while + /// it was inline is stale, so it is retired. + /// + [Fact] + public async Task ACallSiteTheSwitchDeclinesRetiresWhatItQueued() + { + InlinePatch? patch = null; + // One call site for both runs, since the entry is keyed by the line + for (var run = 0; run < 2; run++) + { + VerifierSettings.Reset(); + VerifySettings? settings = null; + if (run == 0) + { + VerifierSettings.Inline(); + } + else + { + // The next run, still on, with the delegate now declining the call site + VerifierSettings.Inline((_, _, _, _) => false); + settings = PassingSettings(); + } + + var verification = Verify("value", settings ?? new()); + if (run == 0) + { + await Assert.ThrowsAsync(() => verification); + patch = Assert.Single(queued); + continue; + } + + await verification; + } + + var retire = Assert.Single(retired); + Assert.Equal(patch!.SourceFile, retire.SourceFile); + Assert.Equal(patch.LineHint, retire.Line); + Assert.Empty(Records()); + } + /// /// A record that cannot be read is not a malformed one: deleting it would strand the entry it /// names for good, so it is left for a run that can read it. @@ -238,6 +439,49 @@ public async Task AnUnreadableRecordIsLeftForALaterRun() Assert.Single(Records()); } + /// + /// A record that reads but names no call site can never be retired, so it is deleted rather + /// than read again by every later run. + /// + [Fact] + public async Task AMalformedRecordIsDeleted() + { + Directory.CreateDirectory(InlineSwitchRecordsDirectory); + await File.WriteAllTextAsync(Path.Combine(InlineSwitchRecordsDirectory, "malformed.txt"), "not a record"); + + await Verify("value", PassingSettings()); + + Assert.Empty(retired); + Assert.Empty(Records()); + } + + /// + /// Cleaning up after the switch must not change a test outcome. A retire that throws costs only + /// its own record, and the rest are still retired. + /// + [Fact] + public async Task ARetireThatThrowsDoesNotFailTheVerification() + { + InlineSwitchRecords.Write(temp.BuildPath("First.cs"), 1); + InlineSwitchRecords.Write(temp.BuildPath("Second.cs"), 1); + var thrown = false; + InlineEngine.SendRetire = (sourceFile, line, memberName) => + { + if (!thrown) + { + thrown = true; + throw new("The queue owner fell over"); + } + + Retire(sourceFile, line, memberName); + }; + + await Verify("value", PassingSettings()); + + Assert.Single(retired); + Assert.Empty(Records()); + } + // A file verification that passes. A failing one hands a pending move to whatever owns the queue // on this machine, so AutoVerify accepts it into the temp directory instead VerifySettings PassingSettings() diff --git a/src/Verify.Tests/InlineRetireTests.cs b/src/Verify.Tests/InlineRetireTests.cs index 98b2db126..3d4741d60 100644 --- a/src/Verify.Tests/InlineRetireTests.cs +++ b/src/Verify.Tests/InlineRetireTests.cs @@ -76,6 +76,33 @@ public async Task NotInlineRetiresTheCallSite() Assert.Null(settle.Origin); } + /// + /// A declined Snapshot call is a call site of its own, and it is the one whose entry is pending: + /// a patch for a literal already in the source is keyed by the Snapshot call's line, not the + /// verify call's. The same goes for a literal that outgrew the size limit. + /// + [Fact] + public async Task ADeclinedSnapshotRetiresItsOwnCallSite() + { + // Nothing is at this path. Declining a literal strips it from the file it names, and this is + // about the retire alone + var source = Path.Combine(listener.Directory, "Snapshot.cs"); + + var settings = new VerifySettings(); + settings.UseDirectory(listener.Directory); + settings.Snapshot("value", source, 7, "\"value\""); + settings.NotInline(); + + // Accepted rather than left failing, for the reason given in NotInlineRetiresTheCallSite + settings.AutoVerify(); + + await Verify("value", settings); + + var settle = listener.AwaitSettle(nameof(ADeclinedSnapshotRetiresItsOwnCallSite)); + Assert.NotNull(settle); + Assert.Equal(InlineKey.For(InnerVerifier.MapSourceFile(source), 7), settle.Key); + } + /// /// A settle only ever reached the queue owner, which says nothing to a snapshot that is on /// disk instead — staged by a run that found no owner, or written out by one on its way out.