From f1393db1d10104f75a4490df429f9029004f7775 Mon Sep 17 00:00:00 2001 From: Andy Jordan <2226434+andyleejordan@users.noreply.github.com> Date: Tue, 16 Jun 2026 15:22:06 -0700 Subject: [PATCH 1/3] Make `OnIdle` tests deterministic by polling instead of sleeping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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> --- .../Session/PsesInternalHostTests.cs | 46 ++++++++++++++----- 1 file changed, 34 insertions(+), 12 deletions(-) diff --git a/test/PowerShellEditorServices.Test/Session/PsesInternalHostTests.cs b/test/PowerShellEditorServices.Test/Session/PsesInternalHostTests.cs index e9b5cf37c..e0ec4abb0 100644 --- a/test/PowerShellEditorServices.Test/Session/PsesInternalHostTests.cs +++ b/test/PowerShellEditorServices.Test/Session/PsesInternalHostTests.cs @@ -19,6 +19,38 @@ namespace PowerShellEditorServices.Test.Session using System.Management.Automation; using System.Management.Automation.Runspaces; + // Shared helpers for the OnIdle engine-event tests, whose handler actions are + // dispatched asynchronously by PowerShell's event manager. + internal static class OnIdleTestHelpers + { + // The OnIdle engine event's -Action scriptblock is not run inline when + // OnPowerShellIdle generates the event; PowerShell enqueues it as a pending + // action and dispatches it asynchronously around subsequent pipeline executions. + // So instead of sleeping a fixed amount, poll the handler variable until it + // reports true (each read is itself a pipeline, giving the engine another chance + // to drain the pending action). On timeout we return the last observed value so + // the caller's assertion still fails loudly. + internal static async Task> WaitForHandledAsync( + PsesInternalHost psesHost, string variableName) + { + using CancellationTokenSource cancellationSource = new(millisecondsDelay: 15000); + IReadOnlyList handled = Array.Empty(); + while (true) + { + handled = await psesHost.ExecutePSCommandAsync( + new PSCommand().AddScript(variableName), + CancellationToken.None); + + if ((handled.Count > 0 && handled[0]) || cancellationSource.IsCancellationRequested) + { + return handled; + } + + await Task.Delay(200); + } + } + } + [Trait("Category", "PsesInternalHost")] public class PsesInternalHostTests : IAsyncLifetime { @@ -203,12 +235,7 @@ await psesHost.ExecuteDelegateAsync( (_, _) => psesHost.OnPowerShellIdle(CancellationToken.None), CancellationToken.None); - // TODO: Why is this racy? - Thread.Sleep(2000); - - handled = await psesHost.ExecutePSCommandAsync( - new PSCommand().AddScript("$handled"), - CancellationToken.None); + handled = await OnIdleTestHelpers.WaitForHandledAsync(psesHost, "$handled"); Assert.Collection(handled, Assert.True); } @@ -303,12 +330,7 @@ await psesHost.ExecuteDelegateAsync( (_, _) => psesHost.OnPowerShellIdle(CancellationToken.None), CancellationToken.None); - // TODO: Why is this racy? - Thread.Sleep(2000); - - IReadOnlyList handled = await psesHost.ExecutePSCommandAsync( - new PSCommand().AddScript("$handledInProfile"), - CancellationToken.None); + IReadOnlyList handled = await OnIdleTestHelpers.WaitForHandledAsync(psesHost, "$handledInProfile"); Assert.Collection(handled, Assert.True); } From a3ba818d10c22036fa7da4ad242d588a31fa892a Mon Sep 17 00:00:00 2001 From: Andy Jordan <2226434+andyleejordan@users.noreply.github.com> Date: Wed, 17 Jun 2026 10:15:05 -0700 Subject: [PATCH 2/3] Move `OnIdle` assertion into helper and drop list return MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Small follow-up to review feedback: both call sites only ever asserted the handler variable became `$true`, so the `IReadOnlyList` 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> --- .../Session/PsesInternalHostTests.cs | 25 ++++++++----------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/test/PowerShellEditorServices.Test/Session/PsesInternalHostTests.cs b/test/PowerShellEditorServices.Test/Session/PsesInternalHostTests.cs index e0ec4abb0..77688c1a8 100644 --- a/test/PowerShellEditorServices.Test/Session/PsesInternalHostTests.cs +++ b/test/PowerShellEditorServices.Test/Session/PsesInternalHostTests.cs @@ -28,26 +28,25 @@ internal static class OnIdleTestHelpers // action and dispatches it asynchronously around subsequent pipeline executions. // So instead of sleeping a fixed amount, poll the handler variable until it // reports true (each read is itself a pipeline, giving the engine another chance - // to drain the pending action). On timeout we return the last observed value so - // the caller's assertion still fails loudly. - internal static async Task> WaitForHandledAsync( - PsesInternalHost psesHost, string variableName) + // to drain the pending action), and fail if it never does within the timeout. + internal static async Task AssertHandledAsync(PsesInternalHost psesHost, string variableName) { using CancellationTokenSource cancellationSource = new(millisecondsDelay: 15000); - IReadOnlyList handled = Array.Empty(); - while (true) + while (!cancellationSource.IsCancellationRequested) { - handled = await psesHost.ExecutePSCommandAsync( + IReadOnlyList handled = await psesHost.ExecutePSCommandAsync( new PSCommand().AddScript(variableName), CancellationToken.None); - if ((handled.Count > 0 && handled[0]) || cancellationSource.IsCancellationRequested) + if (handled.Count > 0 && handled[0]) { - return handled; + return; } await Task.Delay(200); } + + Assert.Fail($"Timed out waiting for the OnIdle handler to set '{variableName}'."); } } @@ -235,9 +234,7 @@ await psesHost.ExecuteDelegateAsync( (_, _) => psesHost.OnPowerShellIdle(CancellationToken.None), CancellationToken.None); - handled = await OnIdleTestHelpers.WaitForHandledAsync(psesHost, "$handled"); - - Assert.Collection(handled, Assert.True); + await OnIdleTestHelpers.AssertHandledAsync(psesHost, "$handled"); } [Fact] @@ -330,9 +327,7 @@ await psesHost.ExecuteDelegateAsync( (_, _) => psesHost.OnPowerShellIdle(CancellationToken.None), CancellationToken.None); - IReadOnlyList handled = await OnIdleTestHelpers.WaitForHandledAsync(psesHost, "$handledInProfile"); - - Assert.Collection(handled, Assert.True); + await OnIdleTestHelpers.AssertHandledAsync(psesHost, "$handledInProfile"); } } } From fc97a4025d028a70d44de75f1187a3046754b4fb Mon Sep 17 00:00:00 2001 From: Andy Jordan <2226434+andyleejordan@users.noreply.github.com> Date: Wed, 17 Jun 2026 10:42:28 -0700 Subject: [PATCH 3/3] 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> --- .../Session/PsesInternalHostTests.cs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/test/PowerShellEditorServices.Test/Session/PsesInternalHostTests.cs b/test/PowerShellEditorServices.Test/Session/PsesInternalHostTests.cs index 77688c1a8..3844cf01b 100644 --- a/test/PowerShellEditorServices.Test/Session/PsesInternalHostTests.cs +++ b/test/PowerShellEditorServices.Test/Session/PsesInternalHostTests.cs @@ -28,25 +28,25 @@ internal static class OnIdleTestHelpers // action and dispatches it asynchronously around subsequent pipeline executions. // So instead of sleeping a fixed amount, poll the handler variable until it // reports true (each read is itself a pipeline, giving the engine another chance - // to drain the pending action), and fail if it never does within the timeout. + // to drain the pending action), then assert it was set within the timeout. internal static async Task AssertHandledAsync(PsesInternalHost psesHost, string variableName) { using CancellationTokenSource cancellationSource = new(millisecondsDelay: 15000); - while (!cancellationSource.IsCancellationRequested) + bool handled = false; + while (!handled && !cancellationSource.IsCancellationRequested) { - IReadOnlyList handled = await psesHost.ExecutePSCommandAsync( + IReadOnlyList result = await psesHost.ExecutePSCommandAsync( new PSCommand().AddScript(variableName), CancellationToken.None); - if (handled.Count > 0 && handled[0]) + handled = result.Count > 0 && result[0]; + if (!handled) { - return; + await Task.Delay(200); } - - await Task.Delay(200); } - Assert.Fail($"Timed out waiting for the OnIdle handler to set '{variableName}'."); + Assert.True(handled, $"Timed out waiting for the OnIdle handler to set '{variableName}'."); } }