Skip to content

Commit 20a0ed2

Browse files
Fix flaky CanRunOnIdleTask by polling instead of sleeping (#2314)
* Make `OnIdle` tests deterministic by polling instead of sleeping `CanRunOnIdleTask` (and its twin `CanRunOnIdleInProfileTask`) were flaky on the net462 (Windows PowerShell 5.1) CI leg — the former was just caught failing on PR #2298's Windows job. The root cause is that `PsesInternalHost.OnPowerShellIdle` calls `Events.GenerateEvent(PSEngineEvent.OnIdle, ...)`, which only *enqueues* the event. For a subscriber registered with `-Action {...}`, PowerShell doesn't run the action scriptblock inline; it becomes a pending action that the engine dispatches asynchronously on the pipeline thread, around subsequent pipeline invocations. So the action's execution was never synchronized with the test's `$handled` read, and the fixed `Thread.Sleep(2000)` was just a timing guess — sometimes too short on the slower WinPS leg, leaving `$global:handled` still `$false` at the assertion. The key realization is that each *additional* pipeline execution gives the engine another chance to drain the pending action, so re-reading the handler variable in a loop both waits for *and* drives completion. I replaced the sleep with a shared `WaitForHandledAsync` helper that polls the variable (~200ms apart, ~15s ceiling) until it reports `$true`, returning the last observed value on timeout so the assertion still fails loudly. This keeps the tests' intent intact and isn't merely a longer sleep. I validated both tests on net8.0 (green across repeated runs, ~0.4s each vs. the old fixed 2s); net462 can't run on macOS, but the mechanism is identical across targets and the 15s ceiling self-terminates on success, so it's strictly safer on the slow leg without slowing the fast one. Drafted by Copilot (Claude Opus 4.8). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Move `OnIdle` assertion into helper and drop list return Small follow-up to review feedback: both call sites only ever asserted the handler variable became `$true`, so the `IReadOnlyList<bool>` return was needless ceremony. `AssertHandledAsync` now owns the assertion — it returns once the variable reports `$true` and otherwise fails via `Assert.Fail` when the ~15s poll window elapses, which reads as "the OnIdle handler never ran." `Assert.Fail` is fine here — we're on xUnit 2.9.3 and already use it in the E2E tests. No behavior change to what's being verified; the call sites just shrink to a single `await OnIdleTestHelpers.AssertHandledAsync(...)`. Still green on net8.0. Drafted by Copilot (Claude Opus 4.8). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Assert `OnIdle` poll result with `Assert.True` Follow-up to review feedback: the helper short-circuited with a bare `return` on success and only asserted (`Assert.Fail`) on timeout, so the happy path had no explicit assertion. Restructure the loop to poll until the handler variable is `$true` or the ~15s window elapses, then assert the outcome once with `Assert.True(handled, ...)`. Same behavior, but the success and timeout paths now share a single, self-describing assertion. Still green on net8.0. Drafted by Copilot (Claude Opus 4.8). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 4067880 commit 20a0ed2

1 file changed

Lines changed: 33 additions & 16 deletions

File tree

test/PowerShellEditorServices.Test/Session/PsesInternalHostTests.cs

Lines changed: 33 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,37 @@ namespace PowerShellEditorServices.Test.Session
1919
using System.Management.Automation;
2020
using System.Management.Automation.Runspaces;
2121

22+
// Shared helpers for the OnIdle engine-event tests, whose handler actions are
23+
// dispatched asynchronously by PowerShell's event manager.
24+
internal static class OnIdleTestHelpers
25+
{
26+
// The OnIdle engine event's -Action scriptblock is not run inline when
27+
// OnPowerShellIdle generates the event; PowerShell enqueues it as a pending
28+
// action and dispatches it asynchronously around subsequent pipeline executions.
29+
// So instead of sleeping a fixed amount, poll the handler variable until it
30+
// reports true (each read is itself a pipeline, giving the engine another chance
31+
// to drain the pending action), then assert it was set within the timeout.
32+
internal static async Task AssertHandledAsync(PsesInternalHost psesHost, string variableName)
33+
{
34+
using CancellationTokenSource cancellationSource = new(millisecondsDelay: 15000);
35+
bool handled = false;
36+
while (!handled && !cancellationSource.IsCancellationRequested)
37+
{
38+
IReadOnlyList<bool> result = await psesHost.ExecutePSCommandAsync<bool>(
39+
new PSCommand().AddScript(variableName),
40+
CancellationToken.None);
41+
42+
handled = result.Count > 0 && result[0];
43+
if (!handled)
44+
{
45+
await Task.Delay(200);
46+
}
47+
}
48+
49+
Assert.True(handled, $"Timed out waiting for the OnIdle handler to set '{variableName}'.");
50+
}
51+
}
52+
2253
[Trait("Category", "PsesInternalHost")]
2354
public class PsesInternalHostTests : IAsyncLifetime
2455
{
@@ -203,14 +234,7 @@ await psesHost.ExecuteDelegateAsync(
203234
(_, _) => psesHost.OnPowerShellIdle(CancellationToken.None),
204235
CancellationToken.None);
205236

206-
// TODO: Why is this racy?
207-
Thread.Sleep(2000);
208-
209-
handled = await psesHost.ExecutePSCommandAsync<bool>(
210-
new PSCommand().AddScript("$handled"),
211-
CancellationToken.None);
212-
213-
Assert.Collection(handled, Assert.True);
237+
await OnIdleTestHelpers.AssertHandledAsync(psesHost, "$handled");
214238
}
215239

216240
[Fact]
@@ -303,14 +327,7 @@ await psesHost.ExecuteDelegateAsync(
303327
(_, _) => psesHost.OnPowerShellIdle(CancellationToken.None),
304328
CancellationToken.None);
305329

306-
// TODO: Why is this racy?
307-
Thread.Sleep(2000);
308-
309-
IReadOnlyList<bool> handled = await psesHost.ExecutePSCommandAsync<bool>(
310-
new PSCommand().AddScript("$handledInProfile"),
311-
CancellationToken.None);
312-
313-
Assert.Collection(handled, Assert.True);
330+
await OnIdleTestHelpers.AssertHandledAsync(psesHost, "$handledInProfile");
314331
}
315332
}
316333
}

0 commit comments

Comments
 (0)