diff --git a/docs/inline-snapshots.md b/docs/inline-snapshots.md index f57c0917b..fba3b2d4c 100644 --- a/docs/inline-snapshots.md +++ b/docs/inline-snapshots.md @@ -390,7 +390,7 @@ Two further differences need nothing from the reader. F# does not implement `Cal Both directions are handled without any manual file editing. -**File to inline.** The existing `.verified.` file for the inlined target is detected as stale and flows through the standard [Delete handling](exception-message-format.md): deleted automatically under AutoVerify, otherwise listed in the `Delete:` section and pended for review. A pending delete goes to the tray when one is running, and to the viewer otherwise, launching one if none is up, so it is reviewable with no tray installed. Files belonging to the other targets keep their names and are left alone. +**File to inline.** The existing `.verified.` file for the inlined target is detected as stale and flows through the standard [Delete handling](exception-message-format.md): deleted automatically under AutoVerify, otherwise listed in the `Delete:` section and pended for review. A pending delete goes to the tray when one is running, and to the viewer otherwise, launching one if none is up, so it is reviewable with no tray installed. If the snapshot moves back to that file before the delete is accepted, the next run verifies against the file and withdraws the delete, so accepting it cannot remove a file that is in use again. Files belonging to the other targets keep their names and are left alone. This direction has no opt-in of its own, so a snapshot that shrinks back under a [`maxLines`](#limiting-the-size-of-an-inline-snapshot) limit returns to inline as soon as it does, leaving its file behind as a stale delete. A snapshot whose size hovers around the limit therefore moves each time it crosses. diff --git a/docs/mdsource/inline-snapshots.source.md b/docs/mdsource/inline-snapshots.source.md index ffc79fd62..acb0d5b6e 100644 --- a/docs/mdsource/inline-snapshots.source.md +++ b/docs/mdsource/inline-snapshots.source.md @@ -273,7 +273,7 @@ Two further differences need nothing from the reader. F# does not implement `Cal Both directions are handled without any manual file editing. -**File to inline.** The existing `.verified.` file for the inlined target is detected as stale and flows through the standard [Delete handling](exception-message-format.md): deleted automatically under AutoVerify, otherwise listed in the `Delete:` section and pended for review. A pending delete goes to the tray when one is running, and to the viewer otherwise, launching one if none is up, so it is reviewable with no tray installed. Files belonging to the other targets keep their names and are left alone. +**File to inline.** The existing `.verified.` file for the inlined target is detected as stale and flows through the standard [Delete handling](exception-message-format.md): deleted automatically under AutoVerify, otherwise listed in the `Delete:` section and pended for review. A pending delete goes to the tray when one is running, and to the viewer otherwise, launching one if none is up, so it is reviewable with no tray installed. If the snapshot moves back to that file before the delete is accepted, the next run verifies against the file and withdraws the delete, so accepting it cannot remove a file that is in use again. Files belonging to the other targets keep their names and are left alone. This direction has no opt-in of its own, so a snapshot that shrinks back under a [`maxLines`](#limiting-the-size-of-an-inline-snapshot) limit returns to inline as soon as it does, leaving its file behind as a stale delete. A snapshot whose size hovers around the limit therefore moves each time it crosses. diff --git a/src/StaticSettingsTests/RaisedDeleteTests.cs b/src/StaticSettingsTests/RaisedDeleteTests.cs new file mode 100644 index 000000000..b83600bc7 --- /dev/null +++ b/src/StaticSettingsTests/RaisedDeleteTests.cs @@ -0,0 +1,202 @@ +using DiffEngine; + +// Lives here, rather than in Verify.Tests, since what a run raised is read back once per process, +// and resetting that part way through a test is what stands in for the next run. This project +// runs serially with BaseTest resetting between tests. +// +// A delete is raised for a verified file no target produced, and waits in the tray or the viewer +// for someone to accept it. Once a later run verifies against that file again, accepting the delete +// would remove a file a passing test depends on, so the run that finds it in use withdraws it. +public class RaisedDeleteTests : + BaseTest, + IDisposable +{ + List added = []; + List settled = []; + Func originalAddDelete = RaisedDeletes.AddDelete; + Action originalSettleDelete = RaisedDeletes.SettleDelete; + bool originalDisabled = DiffRunner.Disabled; + TempDirectory temp = new(); + + public RaisedDeleteTests() + { + // Both stood in for, so nothing reaches whatever tray or viewer is running on this machine + RaisedDeletes.AddDelete = _ => + { + added.Add(_); + return Task.CompletedTask; + }; + RaisedDeletes.SettleDelete = _ => settled.Add(_); + + // DiffEngine switches itself off on a build server, under continuous testing and under an + // AI CLI, and both a delete and its settle answer to that switch + DiffRunner.Disabled = false; + } + + public void Dispose() + { + RaisedDeletes.AddDelete = originalAddDelete; + RaisedDeletes.SettleDelete = originalSettleDelete; + DiffRunner.Disabled = originalDisabled; + temp.Dispose(); + } + + [Fact] + public async Task ADeleteIsWithdrawnOnceItsFileIsInUseAgain() + { + var (first, second) = await SeedTwoTargets(); + + // One target, so both indexed files are left over from a run that produced two + await Assert.ThrowsAsync(() => Verify("a", Settings())); + Assert.Equal([first, second], added.Order(StringComparer.Ordinal)); + Assert.Equal(2, Records().Count); + Assert.Empty(settled); + + // The next run, with the second target back + VerifierSettings.Reset(); + await Verify(TwoTargets("a", "b"), Settings()); + + Assert.Equal([first, second], settled.Order(StringComparer.Ordinal)); + Assert.Empty(Records()); + Assert.True(File.Exists(first)); + Assert.True(File.Exists(second)); + } + + /// + /// A file that no longer matches is still in use: the mismatch is a pending move onto it, and + /// accepting a delete of it as well would leave that move with nothing to replace. + /// + [Fact] + public async Task ADeleteIsWithdrawnWhenItsFileNoLongerMatches() + { + var (first, second) = await SeedTwoTargets(); + await Assert.ThrowsAsync(() => Verify("a", Settings())); + + VerifierSettings.Reset(); + await Assert.ThrowsAsync(() => Verify(TwoTargets("a", "changed"), Settings())); + + Assert.Equal([first, second], settled.Order(StringComparer.Ordinal)); + Assert.Empty(Records()); + } + + /// + /// A run that cannot reach the owner leaves the records for one that can, rather than dropping + /// them with their deletes still pending. + /// + [Fact] + public async Task WithDiffEngineOffTheRecordsWaitForALaterRun() + { + var (first, second) = await SeedTwoTargets(); + await Assert.ThrowsAsync(() => Verify("a", Settings())); + + VerifierSettings.Reset(); + DiffRunner.Disabled = true; + await Verify(TwoTargets("a", "b"), Settings()); + + Assert.Empty(settled); + Assert.Equal(2, Records().Count); + + VerifierSettings.Reset(); + DiffRunner.Disabled = false; + await Verify(TwoTargets("a", "b"), Settings()); + + Assert.Equal([first, second], settled.Order(StringComparer.Ordinal)); + Assert.Empty(Records()); + } + + /// + /// Accepting a delete removes its file, and a tray drops a delete whose file is missing, so the + /// record has nothing left to withdraw. The next run drops it rather than keeping it for good. + /// + [Fact] + public async Task ARecordWhoseFileHasGoneIsDropped() + { + var (first, second) = await SeedTwoTargets(); + await Assert.ThrowsAsync(() => Verify("a", Settings())); + Assert.Equal(2, Records().Count); + + // Both deletes accepted + File.Delete(first); + File.Delete(second); + + VerifierSettings.Reset(); + await Assert.ThrowsAsync(() => Verify("a", Settings())); + + Assert.Empty(Records()); + Assert.Empty(settled); + } + + /// + /// Where DiffEngine is off no delete is raised, so there is nothing to withdraw later. + /// + [Fact] + public async Task NothingIsRecordedWhileDiffEngineIsOff() + { + await SeedTwoTargets(); + DiffRunner.Disabled = true; + + await Assert.ThrowsAsync(() => Verify("a", Settings())); + + Assert.Empty(Records()); + } + + /// + /// Every verification of every codebase comes through here, so with nothing raised nothing may + /// reach the queue owner. + /// + [Fact] + public async Task NothingRaisedSettlesNothing() + { + await File.WriteAllTextAsync(Path.Combine(temp.Path, "Shared.verified.txt"), "a"); + + await Verify("a", Settings()); + + Assert.Empty(added); + Assert.Empty(settled); + } + + // Two targets with the same extension, which is what indexes their names + async Task<(string First, string Second)> SeedTwoTargets() + { + var first = Path.Combine(temp.Path, "Shared#00.verified.txt"); + var second = Path.Combine(temp.Path, "Shared#01.verified.txt"); + await File.WriteAllTextAsync(first, "a"); + await File.WriteAllTextAsync(second, "b"); + return (first, second); + } + + static List TwoTargets(string first, string second) => + [ + new("txt", first), + new("txt", second) + ]; + + VerifySettings Settings() + { + var settings = new VerifySettings(); + settings.UseDirectory(temp); + settings.UseFileName("Shared"); + // No diff tool is launched and no pending move is sent, so the delete and its settle are + // all that reach DiffEngine, and both are stood in for + settings.DisableDiff(); + settings.DisableRequireUniquePrefix(); + return settings; + } + + // Filtered to this test's files, so a record another test left behind cannot fail this one + List Records() + { + var directory = Path.Combine( + AttributeReader.GetIntermediateDirectory(typeof(RaisedDeleteTests).Assembly), + RaisedDeletes.DirectoryName); + if (!Directory.Exists(directory)) + { + return []; + } + + return Directory.EnumerateFiles(directory) + .Select(File.ReadAllText) + .Where(_ => _.StartsWith(temp.Path, StringComparison.OrdinalIgnoreCase)) + .ToList(); + } +} diff --git a/src/Verify/Serialization/VerifierSettings.cs b/src/Verify/Serialization/VerifierSettings.cs index 2df4c92c6..dcfec655c 100644 --- a/src/Verify/Serialization/VerifierSettings.cs +++ b/src/Verify/Serialization/VerifierSettings.cs @@ -180,6 +180,7 @@ internal static void Reset() EngineScrubberSet.InvalidateGlobalCache(); GlobalIgnoredParameters = null; GlobalIgnoreConstructorParameters = false; + RaisedDeletes.Reset(); } public static void UseStrictJson() diff --git a/src/Verify/Verifier/RaisedDeletes.cs b/src/Verify/Verifier/RaisedDeletes.cs new file mode 100644 index 000000000..8a2b74ae6 --- /dev/null +++ b/src/Verify/Verifier/RaisedDeletes.cs @@ -0,0 +1,205 @@ +/// +/// The deletes a run raised for verified files no target produced, recorded so a later run that +/// verifies against one of those files again can withdraw its delete. +/// +/// +/// A delete waits in the tray or the viewer for someone to accept it. Nothing withdrew it when its +/// file came back into use - a target that came back, or a snapshot that moved inline and then +/// back out when the switch was turned off - so accepting it removed a file a passing test depends +/// on, and the next run failed for a snapshot that had been fine. +/// +/// Withdrawing on every verification would cost a round trip to the queue owner per verified file, +/// for every codebase, and on Windows a connect to a port nothing listens on waits out its timeout. +/// So the run that raises a delete records the file, and a verification that uses a recorded file +/// withdraws the delete and drops the record. With nothing recorded, that costs one directory check +/// per process. +/// +/// +/// Kept in the intermediate directory beside the received maps, so per configuration and target +/// framework: a delete one framework's run raised is withdrawn by the next run of that framework +/// that uses the file. A record whose file has gone is dropped when the records are read, and all +/// of them whenever obj is cleaned. +/// +/// +static class RaisedDeletes +{ + internal const string DirectoryName = "VerifyDelete"; + + /// + /// Swapped in tests. What reaches the tray or the viewer is otherwise only observable from them. + /// + internal static Func AddDelete = DiffRunner.AddDeleteAsync; + + /// + internal static Action SettleDelete = DiffRunner.SettleDelete; + + // The file system decides when two spellings are one file, and the delete a tray holds is + // keyed the same way + static readonly StringComparer pathComparer = + RuntimeInformation.IsOSPlatform(OSPlatform.Windows) || + RuntimeInformation.IsOSPlatform(OSPlatform.OSX) + ? StringComparer.OrdinalIgnoreCase + : StringComparer.Ordinal; + + static Lock locker = new(); + + // Verified file to the record naming it. Read from disk once per process, the first time it is + // asked for, and kept current as this process raises and settles + static volatile ConcurrentDictionary? recorded; + + /// + /// Raises a delete for a verified file no target produced, recording it first so it is never + /// pending without a record. + /// + public static Task Raise(string file) + { + // Nothing is raised where DiffEngine is switched off, so there is nothing to withdraw later + if (!DiffRunner.Disabled) + { + Record(file); + } + + return AddDelete(file); + } + + /// + /// Called for every verified file a verification used, whatever the comparison found. A file in + /// use is not stale, so a delete an earlier run raised for it is withdrawn. + /// + public static void SettleIfRaised(string file) + { + // A settle answers to the same switch a delete does. The record stays for a run that can + // reach the owner, rather than being dropped with its delete still pending + if (DiffRunner.Disabled) + { + return; + } + + var records = Recorded(); + if (records.IsEmpty || + !records.TryRemove(file, out var record)) + { + return; + } + + SettleDelete(file); + TryDelete(record); + } + + static void Record(string file) + { + 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 the file as the file system compares it, so a re run overwrites the same + // record whatever case the path arrived in + var record = Path.Combine(directory, $"{Fnv1a.Hash(Fold(file))}.txt"); + File.WriteAllText(record, file); + Recorded()[file] = record; + } + catch + { + // Only ever used to withdraw the delete later, so failing to write one must not change + // the test outcome. + } + } + + static ConcurrentDictionary Recorded() + { + var current = recorded; + if (current is not null) + { + return current; + } + + using (locker.EnterScope()) + { + return recorded ??= Load(); + } + } + + static ConcurrentDictionary Load() + { + var records = new ConcurrentDictionary(pathComparer); + var intermediate = VerifierSettings.IntermediateDir; + if (intermediate is null) + { + return records; + } + + var directory = Path.Combine(intermediate, DirectoryName); + if (!Directory.Exists(directory)) + { + return records; + } + + string[] files; + try + { + files = Directory.GetFiles(directory, "*.txt"); + } + catch (Exception exception) + when (exception is IOException or UnauthorizedAccessException) + { + return records; + } + + foreach (var record in files) + { + string file; + try + { + file = File.ReadAllText(record); + } + catch (Exception exception) + when (exception is IOException or UnauthorizedAccessException) + { + continue; + } + + // A file that has gone takes its delete with it: accepted, or removed some other way, + // and a tray drops a delete whose file is missing. So there is nothing left for the + // record to withdraw, and keeping it would only grow this directory for good + if (file.Length == 0 || + !File.Exists(file)) + { + TryDelete(record); + continue; + } + + records[file] = record; + } + + return records; + } + + static string Fold(string path) => + ReferenceEquals(pathComparer, StringComparer.OrdinalIgnoreCase) + ? path.ToLowerInvariant() + : path; + + static void TryDelete(string record) + { + try + { + File.Delete(record); + } + catch (Exception exception) + when (exception is IOException or UnauthorizedAccessException) + { + // Settled either way. A record left behind is settled again by the next run that uses + // the file, which finds nothing to withdraw. + } + } + + internal static void Reset() => + recorded = null; +} diff --git a/src/Verify/Verifier/VerifyEngine.cs b/src/Verify/Verifier/VerifyEngine.cs index c6a11e8b6..4cd1ffbf3 100644 --- a/src/Verify/Verifier/VerifyEngine.cs +++ b/src/Verify/Verifier/VerifyEngine.cs @@ -205,6 +205,7 @@ void AddEquals(in FilePair item) public async Task ThrowIfRequired() { ProcessEquals(); + SettleRaisedDeletes(); var inlineFailed = false; if (inlineEngine is { } engine) @@ -330,11 +331,33 @@ async Task ProcessDeletes(string file) return true; } - await DiffRunner.AddDeleteAsync(file); + await RaisedDeletes.Raise(file); return false; } + /// + /// Every verified file this verification compared against is in use, whatever the comparison + /// found, so a delete an earlier run raised for one of them no longer describes a stale file. + /// + void SettleRaisedDeletes() + { + foreach (var item in equal) + { + RaisedDeletes.SettleIfRaised(item.VerifiedPath); + } + + foreach (var item in notEquals) + { + RaisedDeletes.SettleIfRaised(item.File.VerifiedPath); + } + + foreach (var item in @new) + { + RaisedDeletes.SettleIfRaised(item.File.VerifiedPath); + } + } + async Task ProcessNotEquals() { var verified = true;