Improve E2E Test Fixtures to be less flaky by JustinGrote · Pull Request #2208 · PowerShell/PowerShellEditorServices

JustinGrote · 2024-12-05T02:11:39Z

Rewrite Pses Stdio host to be task-based and interface-based
Replace LoggingStream with DebugOutputStream to log LSP communication to debug console when debugging tests
Fix up Tests that require help by offline updating help in all platforms/CI

~~- [ ] Wire up LSP Client logging to XUnit ITestOutputHelper~~
~~- [ ] Wire up PSES Initial File Logging to debug console~~

JustinGrote · 2024-12-05T07:14:08Z

@andyleejordan @SeeminglyScience Did a lot of cleanup here, DAP tests run much faster and I should have cleaned up the race conditions, they've been completely reliable for me so far. The tests should also read fairly cleaner now as well.

andyleejordan · 2024-12-10T19:28:15Z

@JustinGrote running this through the internal pipeline right now!

andyleejordan · 2024-12-10T19:51:07Z

@JustinGrote running this through the internal pipeline right now!

Sooo actually per 20fa478 we cannot currently run the tests on OneBranch since it's stuck with PowerShell 7.3...so this will have to wait until January 😵

andyleejordan · 2025-02-26T22:11:56Z

Hi @JustinGrote, we can finally run the tests again in ADO. Would you want to get this PR updated? Note that I had to skip the getCommand test, it just hung (@SeeminglyScience's guess was that the ADO PowerShell might have network shares in PSModulePath but yeah it just ran for twenty minutes before I killed it).

JustinGrote · 2025-02-28T00:30:36Z

@andyleejordan rebased and passed locally for me. Lets see if ADO likes it.

JustinGrote · 2025-02-28T00:47:54Z

Looks like it works. I can do the logging fixups as a separate item especially now that the other logging stuff was implemented.

Copilot

PR Overview

This PR improves the stability and clarity of the E2E test fixtures by reworking the PSES process host to use task- and interface-based patterns and by replacing the legacy LoggingStream with DebugOutputStream for logging LSP communication.

Converted the StdioLanguageServerProcessHost and related test fixtures to use updated process management with task completion sources.
Replaced LoggingStream with DebugOutputStream to log stream activity only when a debugger is attached.
Updated references in tests and LSP fixtures to use the new host implementation.

Reviewed Changes

File	Description
test/PowerShellEditorServices.Test.E2E/Hosts/StdioLanguageServerProcessHost.cs	Refactored host start/stop logic to use task-based state tracking.
test/PowerShellEditorServices.Test.E2E/Hosts/PsesStdioLanguageServerProcessHost.cs	Updated host constructor and environment variable references for improved test stability.
test/PowerShellEditorServices.Test.E2E/Hosts/DebugOutputStream.cs	Introduced DebugOutputStream to log read/write operations when debugging tests.
test/PowerShellEditorServices.Test.E2E/DebugAdapterProtocolMessageTests.cs	Updated tests to use the new host and logging methods; removed dependency on LoggingStream.
test/PowerShellEditorServices.Test.E2E/LanguageServerProtocolMessageTests.cs, LSPTestsFixtures.cs, IDebugAdapterClientExtensions.cs	Updated usage and references from the legacy process host to the new one.
Various Process files	Removed legacy PsesStdioProcess and related files to consolidate host management.
src/PowerShellEditorServices/Server/PsesDebugServer.cs	Minor formatting adjustment in the debug server response initialization.

Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.

test/PowerShellEditorServices.Test.E2E/DebugAdapterProtocolMessageTests.cs

andyleejordan

Nice, I like this a lot, just some questions. Also as far as ADO goes at this point we just have to YOLO it. I can't easily test a PR against the pipeline that would actually block us from releasing if it broke. 🤷 (Long story, really annoying.)

test/PowerShellEditorServices.Test.E2E/LanguageServerProtocolMessageTests.cs

andyleejordan · 2025-03-04T19:20:08Z

test/PowerShellEditorServices.Test.E2E/PowerShellEditorServices.Test.E2E.csproj

+
+  <ItemGroup>
+    <!-- Used for Update-Help for some test fixtures -->
+    <Content Include="..\PowerShellEditorServices.Test.Shared\PSHelp\**\*"


Would this fix all the tests that depend on help, and could we remove the build step where we Update-Help for the tests?

Only if the tests use Microsoft.PowerShell.Utility cmdlets for their reference. This is only required for Windows PowerShell since it doesn't ship this help in-box, all pwsh flavors do ship help in-box and so we don't have to do this. Assuming we find/adjust all help-based tests to use Microsoft.PowerShell.Utility, then yes this should fix them in AzDO CI

andyleejordan · 2025-03-04T19:21:33Z

test/PowerShellEditorServices.Test.E2E/LanguageServerProtocolMessageTests.cs

        {
-            Skip.If(PsesStdioProcess.RunningInConstrainedLanguageMode && PsesStdioProcess.IsWindowsPowerShell,
+            Skip.If(PsesStdioLanguageServerProcessHost.RunningInConstrainedLanguageMode && PsesStdioLanguageServerProcessHost.IsWindowsPowerShell,
                "Windows PowerShell doesn't trust PSScriptAnalyzer by default so it won't load.");


Hey, is this why people are having problems with the formatter "hanging" when they use Windows PowerShell? Food for thought...

It might be! Maybe I can get some LSP trace logs from them and stick in some telemetry to find out.

andyleejordan · 2025-03-04T19:21:57Z

test/PowerShellEditorServices.Test.E2E/LanguageServerProtocolMessageTests.cs

        [SkippableFact]
        public async Task CanSendHoverRequestAsync()
        {
            Skip.If(IsLinux, "This depends on the help system, which is flaky on Linux.");


Do we still need to skip this test now that the help is mocked?

Maybe not? As you said lets YOLO it as a test.

I also think I separately fixed this test in the other branch so there might be a conflict, I'll rebase.

andyleejordan · 2025-03-04T19:22:31Z

test/PowerShellEditorServices.Test.E2E/LanguageServerProtocolMessageTests.cs

                    new EvaluateRequestArguments
                    {
-                        Expression = "Import-Module Microsoft.PowerShell.Archive -Prefix Slow"
+                        Expression = $"Update-Help Microsoft.Powershell.Utility -SourcePath {s_binDir};Import-Module Microsoft.PowerShell.Utility -Prefix Test -Force"


Ahh sweet, ok yeah, I think there's an Update-Help in the build script too...

andyleejordan · 2025-03-04T19:23:36Z

test/PowerShellEditorServices.Test.E2E/Hosts/PsesStdioLanguageServerProcessHost.cs

+        s_binDir, $"pses_test_logs_{Path.GetRandomFileName()}");
+
+    private const string s_logLevel = "Diagnostic";
+    private static readonly string[] s_featureFlags = { "PSReadLine" };


Do we really pass this through in feature flags still? I didn't think we had anything in feature flags.

I just looked and I didn't see this feature flag actually referenced anywhere, so it can probably be removed but I think that should be the scope of another PR. You're probably seeing it here as a "change" because I moved the file to a new folder I belieeve.

It took longer than 1 minute in Github Actions!

andyleejordan · 2025-03-06T05:47:29Z

@andyleejordan the FindSymbolsInDSCFile test failure appears to be because of 0135baa #1984. You updated the IDs of DSC symbols to include a prefix, but the test doesn't check for that new prefix.

PowerShellEditorServices/src/PowerShellEditorServices/Services/Symbols/Visitors/SymbolVisitor.cs

Lines 275 to 276 in de0eea1

"configuration " + name + " { }",

nameExtent,

I will "fix" this test but please confirm if you're expecting something else. Also, completions allow for a sortText property in the LSP, maybe we should implement this more explicitly if that's the intention for the prefix?

Oops. I don't think I'm expecting something else. And if I remember correctly on the sortText, VS Code completely ignored it and applies their own sorting.

…ifact size.

JustinGrote · 2025-03-06T21:19:20Z

@andyleejordan this should be ready for re-review and an internal pipeline test. Changes summary:

Fixed help-based tests to all use Microsoft.PowerShell.Archive, because it has a small help file to be vendored into the codebase
Added update help build script for all pwsh platforms with verify check.
Added upterm to the CI process pre-build. This allows, if you explicitly re-run a job with debug build, to be able to SSH into the runner to interactively troubleshoot/debug issues. Only runs on debug re-runs
Most tests conditional on platforms fixed or mitigated.

andyleejordan

Sorry I was being picky about my build script lol, going to commit these, test, and merge!

PowerShellEditorServices.build.ps1

andyleejordan · 2025-03-18T16:52:44Z

.github/workflows/ci-test.yml


+      - name: If Debugging, start upterm for interactive pipeline troubleshooting
+        if: ${{ runner.debug == 1 }}
+        uses: lhotari/action-upterm@v1


Well that's cool...I need to share with Steve. He's been asking for something exactly like this.

I'm working on a version that lets you vscode remote into the github action, the tricky part is the authentication but it's definitely doable :)

PowerShellEditorServices.build.ps1

andyleejordan · 2025-03-18T17:05:50Z

That's what I get for doing this on GitHub.

andyleejordan · 2025-03-18T17:09:24Z

It was because I added a single quote...inside single quotes 🫨

JustinGrote · 2025-03-18T17:40:10Z

@andyleejordan thank you!

JustinGrote self-assigned this Dec 5, 2024

JustinGrote changed the title ~~Initial Pass that implements DebugAdapterServerTestBase~~ Improve E2E Test Fixtures to be less flaky Dec 5, 2024

JustinGrote force-pushed the test/DapE2ETestUpdates branch 3 times, most recently from 5cb7349 to 1d8a574 Compare December 5, 2024 06:47

JustinGrote force-pushed the test/DapE2ETestUpdates branch from 7f0083c to 7ecdd5a Compare December 6, 2024 17:46

JustinGrote force-pushed the test/DapE2ETestUpdates branch from 7ecdd5a to b1bafa3 Compare December 20, 2024 19:32

JustinGrote force-pushed the test/DapE2ETestUpdates branch from b1bafa3 to b5c00a3 Compare February 28, 2025 00:26

JustinGrote marked this pull request as ready for review February 28, 2025 00:48

JustinGrote requested review from andyleejordan and Copilot February 28, 2025 00:48

Copilot AI reviewed Feb 28, 2025

View reviewed changes

test/PowerShellEditorServices.Test.E2E/DebugAdapterProtocolMessageTests.cs Show resolved Hide resolved

JustinGrote force-pushed the test/DapE2ETestUpdates branch from e12f94b to 5642221 Compare February 28, 2025 00:55

andyleejordan reviewed Mar 4, 2025

View reviewed changes

andyleejordan force-pushed the test/DapE2ETestUpdates branch from 5642221 to 4e68bf6 Compare March 4, 2025 19:28

JustinGrote added 2 commits March 4, 2025 13:52

Improve E2E Test Fixtures to be less flaky

9b05bef

Adjust test skips

52feb6c

JustinGrote force-pushed the test/DapE2ETestUpdates branch from 4e68bf6 to 52feb6c Compare March 4, 2025 21:52

Fix dangling IsLinux reference

a476e9f

JustinGrote force-pushed the test/DapE2ETestUpdates branch from e0e8c1e to a476e9f Compare March 4, 2025 22:13

JustinGrote added 4 commits March 4, 2025 15:46

Kick Hover Test issue can down the road

9f0babf

Make CanSendGetCommandRequestAsync Skip in CI Again

bfd87c3

Remove unneded using namespace

66346ce

Bump GetCommandRequest to 2 minutes

c8f1c3e

It took longer than 1 minute in Github Actions!

JustinGrote force-pushed the test/DapE2ETestUpdates branch from 22993ea to 34347ba Compare March 6, 2025 03:01

Expand help update to all potential pwsh versions

b22510a

JustinGrote force-pushed the test/DapE2ETestUpdates branch from 34347ba to b22510a Compare March 6, 2025 03:07

Add PS7 Help

cd20348

JustinGrote added 3 commits March 5, 2025 23:30

Try Help CAB again

36a8998

Apparently it's a zip not a cab on Linux...

1cca650

Fixup PS5.1 Help Script

5a46259

JustinGrote force-pushed the test/DapE2ETestUpdates branch 2 times, most recently from 3eb0adb to 50a806f Compare March 6, 2025 20:41

JustinGrote mentioned this pull request Mar 6, 2025

Test PowerShell Preview in CI instead of Daily #2225

Merged

Switch to Microsoft.PowerShell.Archive for help tests for smaller art…

2a65e3d

…ifact size.

JustinGrote force-pushed the test/DapE2ETestUpdates branch from 50a806f to 2a65e3d Compare March 6, 2025 20:58

JustinGrote marked this pull request as ready for review March 6, 2025 21:12

JustinGrote requested a review from a team as a code owner March 6, 2025 21:12

JustinGrote requested a review from andyleejordan March 6, 2025 21:12

andyleejordan requested changes Mar 18, 2025

View reviewed changes

andyleejordan approved these changes Mar 18, 2025

View reviewed changes

andyleejordan enabled auto-merge (squash) March 18, 2025 17:05

Clean up build script comments

4cddce2

andyleejordan force-pushed the test/DapE2ETestUpdates branch from fd596da to 4cddce2 Compare March 18, 2025 17:09

andyleejordan merged commit 7935cf8 into main Mar 18, 2025
8 checks passed

andyleejordan deleted the test/DapE2ETestUpdates branch March 18, 2025 17:18

andyleejordan added Issue-Enhancement A feature request (enhancement). Area-Build & Release Area-Test labels Mar 18, 2025

Conversation

JustinGrote commented Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JustinGrote commented Dec 5, 2024

Uh oh!

andyleejordan commented Dec 10, 2024

Uh oh!

andyleejordan commented Dec 10, 2024

Uh oh!

andyleejordan commented Feb 26, 2025

Uh oh!

JustinGrote commented Feb 28, 2025

Uh oh!

JustinGrote commented Feb 28, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

PR Overview

Reviewed Changes

Uh oh!

Uh oh!

andyleejordan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JustinGrote Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andyleejordan commented Mar 6, 2025

Uh oh!

JustinGrote commented Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andyleejordan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andyleejordan commented Mar 18, 2025

Uh oh!

andyleejordan commented Mar 18, 2025

Uh oh!

Uh oh!

JustinGrote commented Mar 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

JustinGrote commented Dec 5, 2024 •

edited

Loading

JustinGrote Mar 4, 2025 •

edited

Loading

JustinGrote commented Mar 6, 2025 •

edited

Loading