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
{