From 2348cfd7d1ee9f0bcdb525b61702a5b09298acce Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Tue, 29 Sep 2026 08:28:36 -0400 Subject: [PATCH 1/5] Tests: the two ManagedConf migration classes no longer share a crash hook (#4773) --- .../ManagedConfCrashHookCollection.cs | 26 +++++ .../ManagedConfCrashHookCollectionTests.cs | 98 +++++++++++++++++++ .../ManagedConfMigrationRunnerTests.cs | 4 + .../ManagedConfMigrationStepsTests.cs | 4 + 4 files changed, 132 insertions(+) create mode 100644 Darling/Darling.Tests/ManagedConfCrashHookCollection.cs create mode 100644 Darling/Darling.Tests/ManagedConfCrashHookCollectionTests.cs diff --git a/Darling/Darling.Tests/ManagedConfCrashHookCollection.cs b/Darling/Darling.Tests/ManagedConfCrashHookCollection.cs new file mode 100644 index 0000000000..921e78f57e --- /dev/null +++ b/Darling/Darling.Tests/ManagedConfCrashHookCollection.cs @@ -0,0 +1,26 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using Xunit; + +namespace Darling.Tests; + +/// +/// Serializes the test classes that set ManagedConfMigrationSteps.FailBetweenSteps (#4773), the same +/// reason PgFileSettingsStaticsCollection exists for its capability statics. The hook is one static +/// field shared by the whole test process, and any WriteTwoSteps call that runs while it is set throws the +/// simulated crash. Two classes set it to throw, and run in parallel one class's crash landed inside the other +/// class's test: ManagedConfMigrationRunnerTests.RunStepA_Mismatch_RestoresAndListsTheKey failed with +/// the steps class's "simulated crash between steps". +/// also keeps every other class off the hook while these run, including a class that reaches +/// WriteTwoSteps only through DarlingManagedPostgres. +/// +[CollectionDefinition("managed-conf-crash-hook", DisableParallelization = true)] +public sealed class ManagedConfCrashHookCollection +{ +} diff --git a/Darling/Darling.Tests/ManagedConfCrashHookCollectionTests.cs b/Darling/Darling.Tests/ManagedConfCrashHookCollectionTests.cs new file mode 100644 index 0000000000..f94cbefd61 --- /dev/null +++ b/Darling/Darling.Tests/ManagedConfCrashHookCollectionTests.cs @@ -0,0 +1,98 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Collections.Generic; +using System.IO; +using System.Reflection; +using System.Runtime.CompilerServices; +using System.Text.RegularExpressions; +using Xunit; + +namespace Darling.Tests; + +/// +/// #4773: ManagedConfMigrationSteps.FailBetweenSteps is one static field for the whole test process, and +/// two test classes set it to throw. If they run at the same time, one class's simulated crash lands inside the +/// other class's WriteTwoSteps call and fails a test that never asked for it. Both classes carry +/// [Collection("managed-conf-crash-hook")], a collection that also runs alone, so nothing else can meet +/// the hook. These two tests fail when that stops being true, whether by removing the attribute, by dropping +/// DisableParallelization, or by adding a third class that sets the hook without joining the collection. +/// +public sealed class ManagedConfCrashHookCollectionTests +{ + private const string CollectionName = "managed-conf-crash-hook"; + + /// An assignment of anything but null to the hook: the assignment that arms a crash. + private static readonly Regex ArmsTheHook = new( + @"ManagedConfMigrationSteps\s*\.\s*FailBetweenSteps\s*=\s*(?!null\b)", + RegexOptions.CultureInvariant); + + [Fact] + public void TheClassesThatArmTheHook_ShareOneCollection_ThatRunsAlone() + { + foreach (var type in new[] { typeof(ManagedConfMigrationStepsTests), typeof(ManagedConfMigrationRunnerTests) }) + { + var collection = type.GetCustomAttribute(); + Assert.True( + collection is not null && collection.Name == CollectionName, + $"{type.Name} sets ManagedConfMigrationSteps.FailBetweenSteps, a static shared by the whole test " + + $"process, so it must carry [Collection(\"{CollectionName}\")]; found " + + $"{(collection is null ? "no [Collection] attribute" : $"[Collection(\"{collection.Name}\")]")}."); + } + + var definition = typeof(ManagedConfCrashHookCollection).GetCustomAttribute(); + Assert.NotNull(definition); + Assert.Equal(CollectionName, definition.Name); + Assert.True( + definition.DisableParallelization, + $"The \"{CollectionName}\" collection must disable parallelization: without it the two classes are " + + "serialized against each other only, and any other class that reaches WriteTwoSteps can still meet " + + "a crash that was armed for someone else."); + } + + [Fact] + public void EveryTestFileThatArmsTheHook_CarriesTheCollection() + { + var thisFile = ThisFile(); + var offenders = new List(); + var filesThatArmTheHook = 0; + + foreach (var path in Directory.EnumerateFiles(Path.GetDirectoryName(thisFile)!, "*.cs", SearchOption.TopDirectoryOnly)) + { + if (string.Equals(Path.GetFileName(path), Path.GetFileName(thisFile), StringComparison.OrdinalIgnoreCase)) + { + continue; + } + + var text = File.ReadAllText(path); + if (!ArmsTheHook.IsMatch(text)) + { + continue; + } + + filesThatArmTheHook++; + if (!text.Contains($"[Collection(\"{CollectionName}\")]", StringComparison.Ordinal)) + { + offenders.Add(Path.GetFileName(path)); + } + } + + Assert.True( + filesThatArmTheHook >= 2, + $"Expected the scan to find at least the two known classes that arm the hook, found {filesThatArmTheHook}; " + + "the scan has stopped seeing the test sources."); + Assert.True( + offenders.Count == 0, + "These test files arm ManagedConfMigrationSteps.FailBetweenSteps without " + + $"[Collection(\"{CollectionName}\")], so they can crash another class's WriteTwoSteps call: " + + string.Join(", ", offenders)); + } + + private static string ThisFile([CallerFilePath] string path = "") => path; +} diff --git a/Darling/Darling.Tests/ManagedConfMigrationRunnerTests.cs b/Darling/Darling.Tests/ManagedConfMigrationRunnerTests.cs index 2e9ef2e250..0322382cac 100644 --- a/Darling/Darling.Tests/ManagedConfMigrationRunnerTests.cs +++ b/Darling/Darling.Tests/ManagedConfMigrationRunnerTests.cs @@ -19,7 +19,11 @@ namespace Darling.Tests; /// /// (#4336): RunStepA and ResumePending over /// fake snapshot delegates, no database. +/// +/// One test here sets ManagedConfMigrationSteps.FailBetweenSteps, a static shared by the whole test +/// process, so this class shares a non-parallel collection with the other class that sets it (#4773). /// +[Collection("managed-conf-crash-hook")] public sealed class ManagedConfMigrationRunnerTests : IDisposable { private readonly string _dataDir; diff --git a/Darling/Darling.Tests/ManagedConfMigrationStepsTests.cs b/Darling/Darling.Tests/ManagedConfMigrationStepsTests.cs index cf3e79291b..f1435ded97 100644 --- a/Darling/Darling.Tests/ManagedConfMigrationStepsTests.cs +++ b/Darling/Darling.Tests/ManagedConfMigrationStepsTests.cs @@ -16,7 +16,11 @@ namespace Darling.Tests; /// /// (#4336): the backup, the two-step atomic write, and the /// verified stamp. Pure file I/O in throwaway temp directories, no database. +/// +/// One test here sets ManagedConfMigrationSteps.FailBetweenSteps, a static shared by the whole test +/// process, so this class shares a non-parallel collection with the other class that sets it (#4773). /// +[Collection("managed-conf-crash-hook")] public sealed class ManagedConfMigrationStepsTests : IDisposable { private readonly string _dataDir; From 91a430e9c3dfe3969ab0fd28742c477658b4169f Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Tue, 29 Sep 2026 08:35:01 -0400 Subject: [PATCH 2/5] Tests: QueryWindowTruncationTests no longer depends on another class's time settings (#4776) --- Lite.Tests/QueryWindowTruncationTests.cs | 7 ++ Lite.Tests/ServerTimeHelperCollectionTests.cs | 106 ++++++++++++++++++ 2 files changed, 113 insertions(+) create mode 100644 Lite.Tests/ServerTimeHelperCollectionTests.cs diff --git a/Lite.Tests/QueryWindowTruncationTests.cs b/Lite.Tests/QueryWindowTruncationTests.cs index 79249a4a6e..ba0ba8c9c2 100644 --- a/Lite.Tests/QueryWindowTruncationTests.cs +++ b/Lite.Tests/QueryWindowTruncationTests.cs @@ -33,7 +33,14 @@ namespace PerformanceMonitorLite.Tests; /// Own per test (not SharedDuckDbFixture) because the archive /// tests need control of the database's archive directory, to COPY hot rows out to parquet exactly like /// ArchiveViewDedupTests does. +/// +/// One test formats the same time twice with ServerTimeHelper.FormatServerTime (once inside +/// ServerTab.SetWindowTruncatedBanner, once in its assertion), and that reads two settings shared by the +/// whole test process, UtcOffsetMinutes and CurrentDisplayMode. This class joins the +/// server-time-helper collection so a class that changes either setting cannot run between the two +/// calls (#4776). /// +[Collection("server-time-helper")] public sealed class QueryWindowTruncationTests : IDisposable { private readonly int ServerId; diff --git a/Lite.Tests/ServerTimeHelperCollectionTests.cs b/Lite.Tests/ServerTimeHelperCollectionTests.cs new file mode 100644 index 0000000000..dfa99ad4be --- /dev/null +++ b/Lite.Tests/ServerTimeHelperCollectionTests.cs @@ -0,0 +1,106 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Collections.Generic; +using System.IO; +using System.Reflection; +using System.Runtime.CompilerServices; +using System.Text.RegularExpressions; +using Xunit; + +namespace PerformanceMonitorLite.Tests; + +/// +/// #4776: ServerTimeHelper.UtcOffsetMinutes and ServerTimeHelper.CurrentDisplayMode are settings shared +/// by the whole test process. Classes that write them carry [Collection("server-time-helper")] so xUnit runs +/// them one at a time. A class that only READS them (through FormatServerTime, ToServerTime and the +/// like) needs the same collection: if it formats one time twice, a writer running between the two calls makes the +/// two texts differ. QueryWindowTruncationTests did exactly that and failed by a 4 hour difference. +/// +public sealed class ServerTimeHelperCollectionTests +{ + private const string CollectionName = "server-time-helper"; + + /// The members of ServerTimeHelper that read or write one of the two shared settings. + private static readonly Regex ReadsTheSettings = new( + @"ServerTimeHelper\s*\.\s*(UtcOffsetMinutes|CurrentDisplayMode|ToServerTime|FormatServerTime|FormatServerClock)\b", + RegexOptions.CultureInvariant); + + private static readonly Regex Comments = new( + @"//[^\r\n]*|/\*.*?\*/", + RegexOptions.CultureInvariant | RegexOptions.Singleline); + + /// + /// Test files that name a setting in code text without reading it. Each entry says why; add one only with a + /// reason, because an entry switches the scan off for that file. + /// + private static readonly Dictionary NamesWithoutReading = new(StringComparer.OrdinalIgnoreCase) + { + ["ComparisonWindowUtcTests.cs"] = + "names ServerTimeHelper.UtcOffsetMinutes inside a string it searches the product source for", + }; + + [Fact] + public void QueryWindowTruncationTests_IsInTheServerTimeHelperCollection() + { + var collection = typeof(QueryWindowTruncationTests).GetCustomAttribute(); + + Assert.True( + collection is not null && collection.Name == CollectionName, + "QueryWindowTruncationTests formats one time twice with ServerTimeHelper.FormatServerTime, which reads " + + "settings shared by the whole test process, so it must carry " + + $"[Collection(\"{CollectionName}\")]; found " + + $"{(collection is null ? "no [Collection] attribute" : $"[Collection(\"{collection.Name}\")]")}."); + } + + [Fact] + public void EveryTestFileThatReadsTheSettings_CarriesTheCollection() + { + var thisFile = ThisFile(); + var testsDirectory = Path.GetDirectoryName(thisFile)!; + var offenders = new List(); + var filesThatRead = 0; + + foreach (var path in Directory.EnumerateFiles(testsDirectory, "*.cs", SearchOption.AllDirectories)) + { + var relative = Path.GetRelativePath(testsDirectory, path); + var segments = relative.Split(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar); + if (Array.Exists(segments, s => s is "bin" or "obj") + || string.Equals(Path.GetFullPath(path), Path.GetFullPath(thisFile), StringComparison.OrdinalIgnoreCase) + || NamesWithoutReading.ContainsKey(Path.GetFileName(path))) + { + continue; + } + + var code = Comments.Replace(File.ReadAllText(path), string.Empty); + if (!ReadsTheSettings.IsMatch(code)) + { + continue; + } + + filesThatRead++; + if (!code.Contains($"[Collection(\"{CollectionName}\")]", StringComparison.Ordinal)) + { + offenders.Add(relative); + } + } + + Assert.True( + filesThatRead >= 10, + $"Expected the scan to find the many classes that use ServerTimeHelper's shared settings, found {filesThatRead}; " + + "the scan has stopped seeing the test sources."); + Assert.True( + offenders.Count == 0, + "These test files use ServerTimeHelper's shared settings without " + + $"[Collection(\"{CollectionName}\")], so a class that changes them can run between two of their reads: " + + string.Join(", ", offenders)); + } + + private static string ThisFile([CallerFilePath] string path = "") => path; +} From ee487d15ac9185e3461d00d8ca1ccf900bd23aec Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Tue, 29 Sep 2026 09:55:57 -0400 Subject: [PATCH 3/5] Tests: the crash-hook census counts an assignment that arms the hook, not one that clears it (#4773) The pattern matched '= null' (the second whitespace run backtracked to nothing, so the lookahead saw the space rather than 'null') and '== null', although its doc says an assignment of anything but null. It now refuses '==' and looks past all the whitespace before it checks for null. A theory runs the pattern on strings: '= null', '=null', an extra space, a line break before null, and '== null' do not match; an assignment of a lambda does. --- .../ManagedConfCrashHookCollectionTests.cs | 29 +++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/Darling/Darling.Tests/ManagedConfCrashHookCollectionTests.cs b/Darling/Darling.Tests/ManagedConfCrashHookCollectionTests.cs index f94cbefd61..912d7b6714 100644 --- a/Darling/Darling.Tests/ManagedConfCrashHookCollectionTests.cs +++ b/Darling/Darling.Tests/ManagedConfCrashHookCollectionTests.cs @@ -28,9 +28,14 @@ public sealed class ManagedConfCrashHookCollectionTests { private const string CollectionName = "managed-conf-crash-hook"; - /// An assignment of anything but null to the hook: the assignment that arms a crash. + /// + /// An assignment of anything but null to the hook: the assignment that arms a crash. (?!=) keeps + /// out a comparison (== null). The last lookahead also refuses whitespace, so the run of spaces before + /// the value cannot give characters back and let null look like something else: it only passes once + /// it has seen past all of them. + /// private static readonly Regex ArmsTheHook = new( - @"ManagedConfMigrationSteps\s*\.\s*FailBetweenSteps\s*=\s*(?!null\b)", + @"ManagedConfMigrationSteps\s*\.\s*FailBetweenSteps\s*=(?!=)\s*(?!\s|null\b)", RegexOptions.CultureInvariant); [Fact] @@ -94,5 +99,25 @@ public void EveryTestFileThatArmsTheHook_CarriesTheCollection() + string.Join(", ", offenders)); } + /// + /// The scan's pattern, run on strings. Every test class in the process clears the hook in its constructor + /// (= null, however it is spaced), and a comparison (== null) reads it; neither arms a crash, so + /// neither may count, or the scan would tell every one of those classes to join the collection. An assignment + /// of a lambda arms it, whatever the lambda starts with. + /// + [Theory] + [InlineData(" = null;", false)] + [InlineData("=null;", false)] + [InlineData(" = null;", false)] + [InlineData(" =\r\n null;", false)] + [InlineData(" == null", false)] + [InlineData(" = () => throw new IOException()", true)] + [InlineData(" = _ => throw new IOException()", true)] + [InlineData(" =\r\n () => throw new IOException()", true)] + public void ArmsTheHook_MatchesAnAssignmentThatArms_NotOneThatClearsOrComparesTheHook(string afterTheName, bool expected) + { + Assert.Equal(expected, ArmsTheHook.IsMatch("ManagedConfMigrationSteps.FailBetweenSteps" + afterTheName)); + } + private static string ThisFile([CallerFilePath] string path = "") => path; } From 39db2daa6229eef5f6947ceeea1bf48972d436a5 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Tue, 29 Sep 2026 09:56:36 -0400 Subject: [PATCH 4/5] Tests: the server-time collection guard covers every ServerTimeHelper member that reads the shared settings (#4776) ConvertForDisplay, DisplayTimeToServerTime and GetTimezoneLabel read the shared offset but were missing from the member list, so a test class that called only one of them was never asked to join the server-time-helper collection. They are in the list now. The two-argument DisplayTimeToServerTime reads the offset; the three-argument overload takes it as an argument and reads nothing, but the pattern cannot tell them apart, which the doc comment says. Every test file that calls one of the added members already carries the collection, so no attribute or exemption was needed. --- Lite.Tests/ServerTimeHelperCollectionTests.cs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/Lite.Tests/ServerTimeHelperCollectionTests.cs b/Lite.Tests/ServerTimeHelperCollectionTests.cs index dfa99ad4be..5e1fc2e416 100644 --- a/Lite.Tests/ServerTimeHelperCollectionTests.cs +++ b/Lite.Tests/ServerTimeHelperCollectionTests.cs @@ -27,9 +27,15 @@ public sealed class ServerTimeHelperCollectionTests { private const string CollectionName = "server-time-helper"; - /// The members of ServerTimeHelper that read or write one of the two shared settings. + /// + /// The members of ServerTimeHelper that read or write one of the two shared settings. The conversions + /// (ConvertForDisplay, DisplayTimeToServerTime) and GetTimezoneLabel read the offset, in + /// the modes that use it. The pattern cannot tell the two DisplayTimeToServerTime overloads apart: the + /// one that takes the offset as an argument reads no setting, so a file that calls only that one goes in + /// NamesWithoutReading. + /// private static readonly Regex ReadsTheSettings = new( - @"ServerTimeHelper\s*\.\s*(UtcOffsetMinutes|CurrentDisplayMode|ToServerTime|FormatServerTime|FormatServerClock)\b", + @"ServerTimeHelper\s*\.\s*(UtcOffsetMinutes|CurrentDisplayMode|ToServerTime|ConvertForDisplay|DisplayTimeToServerTime|GetTimezoneLabel|FormatServerTime|FormatServerClock)\b", RegexOptions.CultureInvariant); private static readonly Regex Comments = new( From 277efae9ccfdedb6128b5f5a35525eed4ed54486 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Tue, 29 Sep 2026 10:00:02 -0400 Subject: [PATCH 5/5] Tests: correct the comment on the crash-hook pattern theory (#4773) The comment said every test class clears the hook in its constructor; only the two ManagedConf classes do. It now says what the cases protect: a class that only clears or compares the hook must not be counted as arming it. --- .../Darling.Tests/ManagedConfCrashHookCollectionTests.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Darling/Darling.Tests/ManagedConfCrashHookCollectionTests.cs b/Darling/Darling.Tests/ManagedConfCrashHookCollectionTests.cs index 912d7b6714..5c9cfd6ff1 100644 --- a/Darling/Darling.Tests/ManagedConfCrashHookCollectionTests.cs +++ b/Darling/Darling.Tests/ManagedConfCrashHookCollectionTests.cs @@ -100,10 +100,10 @@ public void EveryTestFileThatArmsTheHook_CarriesTheCollection() } /// - /// The scan's pattern, run on strings. Every test class in the process clears the hook in its constructor - /// (= null, however it is spaced), and a comparison (== null) reads it; neither arms a crash, so - /// neither may count, or the scan would tell every one of those classes to join the collection. An assignment - /// of a lambda arms it, whatever the lambda starts with. + /// The scan's pattern, run on strings. A test class that only clears the hook (= null, however it is + /// spaced) or compares it (== null) cannot crash another class, so it must not count as arming the + /// hook, or the scan would tell that class to join the collection. An assignment of a lambda arms it, + /// whatever the lambda starts with. /// [Theory] [InlineData(" = null;", false)]