diff --git a/src/DiffEngine.Tests/MaxInstancePrecedenceTests.cs b/src/DiffEngine.Tests/MaxInstancePrecedenceTests.cs new file mode 100644 index 00000000..8377ff43 --- /dev/null +++ b/src/DiffEngine.Tests/MaxInstancePrecedenceTests.cs @@ -0,0 +1,62 @@ +/// +/// DiffEngine_MaxInstances was read ahead of the app domain value, and it persists per user - +/// DiffEngineTray writes it to the user environment on every options save. So on a machine that +/// had ever saved Options, silently did nothing, +/// and a test calling it to keep diff windows shut still had them open. +/// +[NotInParallel] +public class MaxInstancePrecedenceTests +{ + const string variable = "DiffEngine_MaxInstances"; + + [Test] + public async Task Setting_it_beats_the_environment() + { + Environment.SetEnvironmentVariable(variable, "10"); + MaxInstance.ResetAppDomainValue(); + + DiffRunner.MaxInstancesToLaunch(0); + + await Assert.That(MaxInstance.MaxInstancesToLaunch).IsEqualTo(0); + } + + /// + /// The environment is still what an app domain that sets nothing gets, which is how the tray + /// and DiffEngine_MaxInstances go on working. + /// + [Test] + public async Task The_environment_is_the_default_when_nothing_sets_it() + { + Environment.SetEnvironmentVariable(variable, "10"); + MaxInstance.ResetAppDomainValue(); + + await Assert.That(MaxInstance.MaxInstancesToLaunch).IsEqualTo(10); + } + + /// + /// A user scope save is the later explicit set, so it takes the value back off the app domain + /// rather than losing to it. This is what the tray does on an options save. + /// + [Test] + public async Task A_user_set_after_an_app_domain_set_wins() + { + // Not 3, so the save is a change and SetForUser does not skip it. Whatever this machine + // happens to have set is not allowed to decide that. + Environment.SetEnvironmentVariable(variable, "10"); + DiffRunner.MaxInstancesToLaunch(0); + + MaxInstance.SetForUser(3); + + await Assert.That(MaxInstance.MaxInstancesToLaunch).IsEqualTo(3); + } + + string? original = Environment.GetEnvironmentVariable(variable); + + [After(Test)] + public void Restore() + { + Environment.SetEnvironmentVariable(variable, original); + MaxInstance.ResetAppDomainValue(); + MaxInstance.ResetCount(); + } +} diff --git a/src/DiffEngine.Tests/MaxInstanceReplacementTests.cs b/src/DiffEngine.Tests/MaxInstanceReplacementTests.cs index 50387dc6..9bc8b6b2 100644 --- a/src/DiffEngine.Tests/MaxInstanceReplacementTests.cs +++ b/src/DiffEngine.Tests/MaxInstanceReplacementTests.cs @@ -47,14 +47,11 @@ public async Task ADifferentPairStillHitsTheLimit() } /// - /// Through the environment variable, because that is what MaxInstance reads first and this - /// machine may well have one set - DiffEngine_MaxInstances persists per user, so the app - /// domain setting alone silently loses to it. Process scoped, so nothing outlives the run. + /// The app domain value beats DiffEngine_MaxInstances, which this machine may well have set - + /// it persists per user - so this is all it takes to pin the limit for the test. /// static void LimitTo(int value) { - Environment.SetEnvironmentVariable(variable, value.ToString()); - // Forces MaxInstance to re-read, since it caches the first answer DiffRunner.MaxInstancesToLaunch(value); MaxInstance.ResetCount(); } @@ -105,8 +102,7 @@ string Write(string name) public void Dispose() { - Environment.SetEnvironmentVariable(variable, original); - DiffRunner.MaxInstancesToLaunch(5); + MaxInstance.ResetAppDomainValue(); MaxInstance.ResetCount(); try { @@ -122,8 +118,6 @@ public void Dispose() // Per test, not static: two tests sharing paths means the second one's first launch finds // the first one's tool still open and is treated as a replacement - const string variable = "DiffEngine_MaxInstances"; - string? original = Environment.GetEnvironmentVariable(variable); string directory = Path.Combine(Path.GetTempPath(), $"DiffEngine.MaxInstance.{Guid.NewGuid():N}"); ResolvedTool tool; string temp; diff --git a/src/DiffEngine.Tests/MaxInstanceUserWriteTests.cs b/src/DiffEngine.Tests/MaxInstanceUserWriteTests.cs new file mode 100644 index 00000000..84a29f1b --- /dev/null +++ b/src/DiffEngine.Tests/MaxInstanceUserWriteTests.cs @@ -0,0 +1,108 @@ +/// +/// The tray persisted DiffEngine_MaxInstances on every settings save, whether or not the limit was +/// part of what changed - the tray's SettingsHelper.Write calls SetForUser unconditionally, and +/// the value it passes is whatever the options form was populated with, which is the value already +/// in effect. Saves of unrelated settings go through the same path, including the "always kill +/// locking processes" prompt, which has no options form at all. So opening the tray once was enough +/// to leave a user scope variable behind on a machine that had never chosen a limit. +/// +[NotInParallel] +public class MaxInstanceUserWriteTests +{ + const string variable = "DiffEngine_MaxInstances"; + + /// + /// The case that put the variable on machines that never asked for it: nothing set, so the + /// form opens at the default and saves it straight back. + /// + [Test] + public async Task Saving_the_default_on_a_machine_with_nothing_set_writes_nothing() + { + Environment.SetEnvironmentVariable(variable, null); + MaxInstance.ResetAppDomainValue(); + + MaxInstance.SetForUser(MaxInstance.MaxInstancesToLaunch); + + await Assert.That(Environment.GetEnvironmentVariable(variable)).IsNull(); + } + + [Test] + public async Task Saving_the_value_already_set_writes_nothing() + { + Environment.SetEnvironmentVariable(variable, "10"); + MaxInstance.ResetAppDomainValue(); + + MaxInstance.SetForUser(10); + + await Assert.That(Environment.GetEnvironmentVariable(variable)).IsEqualTo("10"); + await Assert.That(MaxInstance.MaxInstancesToLaunch).IsEqualTo(10); + } + + /// + /// Choosing the default is a way back to an unset machine, rather than a pin at whatever the + /// default happens to be today. Same as SetTargetOnLeft with false. + /// + [Test] + public async Task Saving_the_default_clears_a_value_that_was_set() + { + var @default = Default(); + Environment.SetEnvironmentVariable(variable, "10"); + MaxInstance.ResetAppDomainValue(); + + MaxInstance.SetForUser(@default); + + await Assert.That(Environment.GetEnvironmentVariable(variable)).IsNull(); + await Assert.That(MaxInstance.MaxInstancesToLaunch).IsEqualTo(@default); + } + + /// + /// Including a value that was pinned at the default explicitly - it says nothing the absent + /// variable does not, and leaving it would keep the machine pinned. + /// + [Test] + public async Task Saving_the_default_over_a_redundant_pin_clears_it() + { + var @default = Default(); + Environment.SetEnvironmentVariable(variable, @default.ToString()); + MaxInstance.ResetAppDomainValue(); + + MaxInstance.SetForUser(@default); + + await Assert.That(Environment.GetEnvironmentVariable(variable)).IsNull(); + } + + /// + /// What the limit is with nothing set, which is what defaultMax is, without reaching into it. + /// + static int Default() + { + Environment.SetEnvironmentVariable(variable, null); + MaxInstance.ResetAppDomainValue(); + return MaxInstance.MaxInstancesToLaunch; + } + + /// + /// And a save that does change the limit still lands, which is the point of the setting. + /// + [Test] + public async Task Saving_a_different_value_writes_it() + { + Environment.SetEnvironmentVariable(variable, "10"); + MaxInstance.ResetAppDomainValue(); + + MaxInstance.SetForUser(7); + + await Assert.That(Environment.GetEnvironmentVariable(variable)).IsEqualTo("7"); + await Assert.That(MaxInstance.MaxInstancesToLaunch).IsEqualTo(7); + } + + string? original = Environment.GetEnvironmentVariable(variable); + + [After(Test)] + public void Restore() + { + Environment.SetEnvironmentVariable(variable, original); + MaxInstance.ResetAppDomainValue(); + MaxInstance.ResetCount(); + } +} diff --git a/src/DiffEngine.Tests/ModuleInitializer.cs b/src/DiffEngine.Tests/ModuleInitializer.cs index 01cc0ab1..2796ae7d 100644 --- a/src/DiffEngine.Tests/ModuleInitializer.cs +++ b/src/DiffEngine.Tests/ModuleInitializer.cs @@ -9,9 +9,18 @@ public static void Initialize() FileExtensions.AddTextFileConvention(_ => _.EndsWith(".txtConvention".AsSpan())); Logging.Enable(); DiffRunner.Disabled = false; + KeepEnvironmentWritesInProcess(); DetachFromPendingFileSurfaces(); } + /// + /// Tests must not write the user environment of the machine running them. The test projects + /// run as parallel processes over the one registry key, so a capture in one and a restore in + /// the other race, and the value that loses is gone. + /// + static void KeepEnvironmentWritesInProcess() => + EnvironmentHelper.Set = EnvironmentHelper.SetProcessOnly; + /// /// Launching sends a real pending move to whatever owns the queue on this machine. On a /// developer box that is the tray, started at login, and an accept or discard from it kills the diff --git a/src/DiffEngine/EnvironmentHelper.cs b/src/DiffEngine/EnvironmentHelper.cs index fb47dcc2..12d1cf77 100644 --- a/src/DiffEngine/EnvironmentHelper.cs +++ b/src/DiffEngine/EnvironmentHelper.cs @@ -1,8 +1,25 @@ static class EnvironmentHelper { - public static void Set(string name, string? value) + /// + /// Both scopes, so a setting chosen in the tray outlives the process that chose it. + /// + /// Swapped for by the test projects. A test would otherwise + /// write the user environment of the machine running it, and cannot put it back reliably: the + /// test projects run as parallel processes over the one registry key, so a capture in one and + /// a restore in the other race, and the value that loses is gone. + /// + /// + internal static Action Set = SetUserAndProcess; + + static void SetUserAndProcess(string name, string? value) { Environment.SetEnvironmentVariable(name, value, EnvironmentVariableTarget.User); Environment.SetEnvironmentVariable(name, value, EnvironmentVariableTarget.Process); } -} \ No newline at end of file + + /// + /// Nothing outside the process. For tests. + /// + internal static void SetProcessOnly(string name, string? value) => + Environment.SetEnvironmentVariable(name, value, EnvironmentVariableTarget.Process); +} diff --git a/src/DiffEngine/MaxInstance.cs b/src/DiffEngine/MaxInstance.cs index 6bd7fb13..319e2be2 100644 --- a/src/DiffEngine/MaxInstance.cs +++ b/src/DiffEngine/MaxInstance.cs @@ -7,9 +7,20 @@ static class MaxInstance static int launchedInstances; const int defaultMax = 5; + /// + /// An explicit set wins over the environment, which is only the ambient default for a run that + /// sets nothing. + /// + /// DiffEngine_MaxInstances persists per user - DiffEngineTray writes it to the user + /// environment on every options save - so reading it first meant that on a machine which had + /// ever saved Options, silently did nothing. A + /// test suppressing diff windows with it still opened them. Same order as + /// , where an explicit set also pins the value. + /// + /// static int GetMaxInstances() => - GetEnvironmentValue() ?? appDomainMaxInstancesToLaunch ?? + GetEnvironmentValue() ?? defaultMax; static int? GetEnvironmentValue() @@ -38,10 +49,61 @@ public static void SetForAppDomain(int value) ResetCapturedValue(); } + /// + /// Persists a user scope value, as does for + /// DiffEngine_TargetOnLeft. + /// + /// A save that changes nothing writes nothing. The tray calls this for every settings save - + /// including saves of unrelated settings, like the "always kill locking processes" prompt - + /// and the value it passes is whatever the options form was populated with. So a machine that + /// had never chosen a limit still ended up with DiffEngine_MaxInstances in its user + /// environment, at the value that was already in effect. + /// + /// + /// And choosing the default clears the variable rather than persisting it, so there is a way + /// back to an unset machine through the options form. + /// + /// public static void SetForUser(int value) { Guard.AgainstNegative(value, nameof(value)); - EnvironmentHelper.Set("DiffEngine_MaxInstances", value.ToString()); + + // The default needs nothing persisted to mean what it means, so returning to it takes the + // variable back out rather than pinning it at what the default happens to be today. + string? desired; + if (value == defaultMax) + { + desired = null; + } + else + { + desired = value.ToString(); + } + + // Only when what is persisted would actually change. Read raw rather than through + // GetEnvironmentValue, because this is a comparison against the stored text, and because + // a setter is no place to throw over an existing unparseable value it is about to + // overwrite anyway. + if (Environment.GetEnvironmentVariable("DiffEngine_MaxInstances") == desired) + { + return; + } + + EnvironmentHelper.Set("DiffEngine_MaxInstances", desired); + // The later explicit set is the one that counts, so an app domain value from earlier in + // the process cannot shadow what the user just chose. + appDomainMaxInstancesToLaunch = null; + ResetCapturedValue(); + } + + /// + /// Forgets an explicit , so the value is read from the + /// environment again. For tests, which is where anything sets it and then wants the ambient + /// value back. + /// + internal static void ResetAppDomainValue() + { + appDomainMaxInstancesToLaunch = null; ResetCapturedValue(); } @@ -57,4 +119,4 @@ public static bool Reached() var count = Interlocked.Increment(ref launchedInstances); return count > MaxInstancesToLaunch; } -} \ No newline at end of file +} diff --git a/src/DiffEngineTray.Tests/DiffRunnerCanKillTest.cs b/src/DiffEngineTray.Tests/DiffRunnerCanKillTest.cs index 639eed66..c7fb9012 100644 --- a/src/DiffEngineTray.Tests/DiffRunnerCanKillTest.cs +++ b/src/DiffEngineTray.Tests/DiffRunnerCanKillTest.cs @@ -8,7 +8,6 @@ public class DiffRunnerCanKillTest : { string tempFile = Path.GetTempFileName(); bool originalDisabled = DiffRunner.Disabled; - string? originalMaxInstances = Environment.GetEnvironmentVariable("DiffEngine_MaxInstances"); public DiffRunnerCanKillTest() { @@ -16,9 +15,7 @@ public DiffRunnerCanKillTest() DiffEngine.DiffEngineTray.IsRunning = true; DiffRunner.Disabled = false; // Force the "too many running" branch so no real process is launched, while a move - // payload is still sent to the tray. The env var takes precedence over the app-domain - // value, so set it too; MaxInstancesToLaunch resets the cached lookup. - Environment.SetEnvironmentVariable("DiffEngine_MaxInstances", "0"); + // payload is still sent to the tray. DiffRunner.MaxInstancesToLaunch(0); } @@ -107,8 +104,7 @@ public void Dispose() { DiffEngine.DiffEngineTray.IsRunning = false; DiffRunner.Disabled = originalDisabled; - Environment.SetEnvironmentVariable("DiffEngine_MaxInstances", originalMaxInstances); - DiffRunner.MaxInstancesToLaunch(5); + MaxInstance.ResetAppDomainValue(); File.Delete(tempFile); } } diff --git a/src/DiffEngineTray.Tests/ModuleInitializer.cs b/src/DiffEngineTray.Tests/ModuleInitializer.cs index 54bd7ef8..a94093cd 100644 --- a/src/DiffEngineTray.Tests/ModuleInitializer.cs +++ b/src/DiffEngineTray.Tests/ModuleInitializer.cs @@ -5,9 +5,18 @@ public static void Initialize() { VerifyWinForms.Initialize(); VerifierSettings.UseSsimForPng(PngSsimThreshold); + KeepEnvironmentWritesInProcess(); PointAtAClosedPort(); } + /// + /// Tests must not write the user environment of the machine running them. The test projects + /// run as parallel processes over the one registry key, so a capture in one and a restore in + /// the other race, and the value that loses is gone. + /// + static void KeepEnvironmentWritesInProcess() => + EnvironmentHelper.Set = EnvironmentHelper.SetProcessOnly; + /// /// Effectively "the same pixels", rather than Verify's 0.98 default. These screens are mostly /// flat background, so 0.98 is far looser than it sounds on them: a whole missing row of text diff --git a/src/DiffEngineTray.Tests/SettingsHelperTests.cs b/src/DiffEngineTray.Tests/SettingsHelperTests.cs index 0d1ac165..9d27c39e 100644 --- a/src/DiffEngineTray.Tests/SettingsHelperTests.cs +++ b/src/DiffEngineTray.Tests/SettingsHelperTests.cs @@ -1,13 +1,52 @@ public class SettingsHelperTests { + const string maxInstances = "DiffEngine_MaxInstances"; + + /// + /// Saving used to leave DiffEngine_MaxInstances in the user environment on a machine that had + /// never chosen a limit. Write is handed whatever the options form was populated with, which + /// is the value already in effect, and unrelated saves come through here too - the "always + /// kill locking processes" prompt persists itself this way and has no options form at all. + /// + [Test] + public async Task Write_leaves_an_unchanged_max_instances_unpersisted() + { + var originalPath = SettingsHelper.FilePath; + var originalProcess = Environment.GetEnvironmentVariable(maxInstances); + var tempFile = Path.Combine(Path.GetTempPath(), $"SettingsHelperTests_{Guid.NewGuid()}.json"); + try + { + SettingsHelper.FilePath = tempFile; + // The "never chose a limit" machine, whatever the one running the test has set + Environment.SetEnvironmentVariable(maxInstances, null); + MaxInstance.ResetAppDomainValue(); + + await SettingsHelper.Write( + new() + { + // As the options form populates it, and as an unrelated save leaves it + MaxInstancesToLaunch = MaxInstance.MaxInstancesToLaunch + }); + + await Assert.That(Environment.GetEnvironmentVariable(maxInstances)).IsNull(); + } + finally + { + SettingsHelper.FilePath = originalPath; + File.Delete(tempFile); + Environment.SetEnvironmentVariable(maxInstances, originalProcess); + MaxInstance.ResetAppDomainValue(); + } + } + [Test] public async Task ReadWrite() { - // SettingsHelper.Write persists MaxInstances/TargetOnLeft to the User-scope environment - // (registry on Windows), so capture and restore them to leave the machine untouched. + // SettingsHelper.Write persists MaxInstances/TargetOnLeft. The module initializer keeps + // that inside this process, so these restore the process scope it writes. var originalPath = SettingsHelper.FilePath; - var originalMaxInstances = Environment.GetEnvironmentVariable("DiffEngine_MaxInstances", EnvironmentVariableTarget.User); - var originalTargetOnLeft = Environment.GetEnvironmentVariable("DiffEngine_TargetOnLeft", EnvironmentVariableTarget.User); + var originalMaxInstances = Environment.GetEnvironmentVariable("DiffEngine_MaxInstances"); + var originalTargetOnLeft = Environment.GetEnvironmentVariable("DiffEngine_TargetOnLeft"); var tempFile = Path.Combine(Path.GetTempPath(), $"SettingsHelperTests_{Guid.NewGuid()}.json"); try {