Skip to content

Commit d0ab67f

Browse files
committed
Pin that killing a diff tool actually kills it
LaunchAndKill and LaunchAndKillAsync asserted that the tool was no longer running after DiffRunner.Kill. That assertion cannot fail: FakeDiffTool sleeps for five seconds and then exits by itself, and WaitForRunning polls for ten, so "gone" is true whether the kill worked or did nothing whatsoever. Both tests pass with ProcessCleanup.Kill short circuited to a bare return - I checked. Assert on how the process ended rather than on whether it is still there. WindowsProcess.TryTerminateProcess passes -1 to TerminateProcess, and a FakeDiffTool that ran out its sleep returns 0, so the exit code separates the two with no timing in it at all. Reading it needs a handle opened before the process goes: Process.GetProcessById holds none of its own, so OpenLaunched touches Handle while the tool is still running. That is the same point ProcessEx makes on the tray side. Neutering Kill now fails both tests on the exit code.
1 parent d5b319f commit d0ab67f

1 file changed

Lines changed: 48 additions & 10 deletions

File tree

src/DiffEngine.Tests/DiffRunnerTests.cs

Lines changed: 48 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -175,10 +175,14 @@ public async Task LaunchAndKill()
175175
await WaitForRunning(true);
176176
await Assert.That(IsRunning()).IsTrue();
177177
await Assert.That(ProcessCleanup.IsRunning(command)).IsTrue();
178+
179+
using var launched = OpenLaunched();
178180
DiffRunner.Kill(file1, file2);
181+
179182
await WaitForRunning(false);
180183
await Assert.That(IsRunning()).IsFalse();
181184
await Assert.That(ProcessCleanup.IsRunning(command)).IsFalse();
185+
await AssertTerminated(launched);
182186
}
183187

184188
[Test]
@@ -192,27 +196,61 @@ public async Task LaunchAndKillAsync()
192196
await WaitForRunning(true);
193197
await Assert.That(IsRunning()).IsTrue();
194198
await Assert.That(ProcessCleanup.IsRunning(command)).IsTrue();
199+
200+
using var launched = OpenLaunched();
195201
DiffRunner.Kill(file1, file2);
202+
196203
await WaitForRunning(false);
197204
await Assert.That(IsRunning()).IsFalse();
198205
await Assert.That(ProcessCleanup.IsRunning(command)).IsFalse();
206+
await AssertTerminated(launched);
207+
}
208+
209+
/// <summary>
210+
/// Opens a handle on the process the launch just started, before anything can kill it.
211+
/// <para>
212+
/// Process.GetProcessById holds no OS handle of its own, and a handle opened after the process
213+
/// has gone cannot report how it went. Touching Handle here is what makes the exit code
214+
/// readable afterwards.
215+
/// </para>
216+
/// </summary>
217+
Process OpenLaunched()
218+
{
219+
var match = ProcessCleanup.FindAll().Single(_ => _.Command == Expected);
220+
var process = Process.GetProcessById(match.Process);
221+
_ = process.Handle;
222+
return process;
223+
}
224+
225+
/// <summary>
226+
/// That the process was killed, rather than that it is merely gone.
227+
/// <para>
228+
/// The distinction is the whole point of this assertion. FakeDiffTool sleeps for five seconds
229+
/// and then exits on its own, and WaitForRunning polls for ten, so "no longer running" is
230+
/// true whether the kill worked or did nothing at all - these tests passed with
231+
/// ProcessCleanup.Kill short circuited to a bare return. The exit code tells them apart:
232+
/// WindowsProcess.TryTerminateProcess passes -1 to TerminateProcess, and a FakeDiffTool that
233+
/// ran out its sleep returns 0.
234+
/// </para>
235+
/// </summary>
236+
static async Task AssertTerminated(Process process)
237+
{
238+
await Assert.That(process.WaitForExit(5000)).IsTrue();
239+
await Assert.That(process.ExitCode).IsNotEqualTo(0);
199240
}
200241

201242
// Match this test's exact command, not any FakeDiffTool: DiffEngineTray.Tests
202243
// runs concurrently in the same CI job and launches its own FakeDiffTool
203244
// instances, which a machine-wide substring scan would see.
204-
bool IsRunning()
205-
{
206-
var expected = command;
207-
if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
208-
{
209-
expected = expected.Replace("\"", "");
210-
}
245+
string Expected =>
246+
RuntimeInformation.IsOSPlatform(OSPlatform.Windows)
247+
? command
248+
: command.Replace("\"", "");
211249

212-
return ProcessCleanup
250+
bool IsRunning() =>
251+
ProcessCleanup
213252
.FindAll()
214-
.Any(_ => _.Command == expected);
215-
}
253+
.Any(_ => _.Command == Expected);
216254

217255
// Process spawn and kill are asynchronous, so poll instead of guessing with a
218256
// fixed sleep. Also used at test start: the previous test's kill may still be

0 commit comments

Comments
 (0)