Run new PSReadLine test on Windows #1522

andyleejordan · 2021-07-06T23:45:17Z

Finishing #1514. Also replaces #1516.

This reverts commit 75a7175. We are reverting this in favor of using a runtime condition set by the test itself because this was fragile (for one, it did not work with OmniSharp running tests).

andyleejordan · 2021-07-07T23:31:47Z

Finally stole your code @dkattan

andyleejordan · 2021-07-08T17:20:32Z

src/PowerShellEditorServices/Services/PowerShellContext/Session/PSReadLinePromptContext.cs

                var psReadLineType = Type.GetType("Microsoft.PowerShell.PSConsoleReadLine, Microsoft.PowerShell.PSReadLine2");

                if (psReadLineType == null)
                {
-                    logger.LogWarning("PSConsoleReadline type not found: {Reason}", pwsh.HadErrors ? pwsh.Streams.Error[0].ToString() : "<Unknown reason>");
-                    return false;
+                    logger.LogTrace("PSConsoleReadline type not found using Type.GetType(), searching all loaded assemblies...");


@rjmholt This should only happen in the case that it needs to. When Type.GetType works, then this logic is skipped. So it feels fine, though I still wish I knew why it was necessary.

Not saying it's "right" but I keep seeing code similar to this come up when I research the issue. I think we will get to the bottom of it as we continue to refactor but this works.

My suspicion is that this always worked in script because it always runs on the pipeline thread and through PowerShell's own type search logic. When we sift through the whole AppDomain (which is kind of a dead concept) for a type, we ignore assembly load contexts, and so that might be why the direction reflection query fails but the search succeeds. Just a theory though. An ALC shouldn't be thread-dependent though and I don't think anyone is using something like EnterContextualReflection()

andyleejordan · 2021-07-08T18:02:12Z

test/PowerShellEditorServices.Test/Session/PowerShellContextTests.cs

        {
-            Skip.If(IsWindows, "This test doesn't work on Windows for some reason.");


For so many reasons 😆

dkattan · 2021-07-08T23:41:08Z

src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs

@@ -2129,75 +2144,6 @@ private static string GetStringForPSCommand(PSCommand psCommand)
            return stringBuilder.ToString();
        }

-        private void SetExecutionPolicy()


Huge fan of removing this and relying on the initialSessionState. I look forward to bringing the entire InitialSessionState in instead of just the LanguageMode so we more accurately create runspaces similar to the parent.

dkattan

Looks good!

src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs

andyleejordan · 2021-07-09T01:00:19Z

@rjmholt I've been looking at Start-EditorServices and the use of -BundledModulesPath there and things seem...fishy. Let's look tomorrow if you have time, because this PR as is does not respect the user settings. So we haven't quite fixed that yet.

rjmholt · 2021-07-09T17:19:13Z

src/PowerShellEditorServices/Services/PowerShellContext/Session/PSReadLinePromptContext.cs

+                    var allAssemblies = AppDomain.CurrentDomain.GetAssemblies();
+                    var assemblies = allAssemblies.FirstOrDefault(a => a.FullName.Contains("Microsoft.PowerShell.PSReadLine2"));
+                    psReadLineType = assemblies?.ExportedTypes?.FirstOrDefault(a => a.FullName == "Microsoft.PowerShell.PSConsoleReadLine");


Suggested change

var allAssemblies = AppDomain.CurrentDomain.GetAssemblies();

var assemblies = allAssemblies.FirstOrDefault(a => a.FullName.Contains("Microsoft.PowerShell.PSReadLine2"));

psReadLineType = assemblies?.ExportedTypes?.FirstOrDefault(a => a.FullName == "Microsoft.PowerShell.PSConsoleReadLine");

Type psrlType = AppDomain.CurrentDomain

.GetAssemblies()

.FirstOrDefault(asm => asm.GetName().Name.Equals("Microsoft.PowerShell.PSReadLine2"))

?.ExportedTypes

?.FirstOrDefault(type => type.FullName.Equals("Microsoft.PowerShell.PSConsoleReadLine"));

rjmholt · 2021-07-09T17:28:44Z

src/PowerShellEditorServices/Services/PowerShellContext/Session/PSReadLinePromptContext.cs

                var psReadLineType = Type.GetType("Microsoft.PowerShell.PSConsoleReadLine, Microsoft.PowerShell.PSReadLine2");

                if (psReadLineType == null)
                {
-                    logger.LogWarning("PSConsoleReadline type not found: {Reason}", pwsh.HadErrors ? pwsh.Streams.Error[0].ToString() : "<Unknown reason>");
-                    return false;
+                    logger.LogTrace("PSConsoleReadline type not found using Type.GetType(), searching all loaded assemblies...");


My suspicion is that this always worked in script because it always runs on the pipeline thread and through PowerShell's own type search logic. When we sift through the whole AppDomain (which is kind of a dead concept) for a type, we ignore assembly load contexts, and so that might be why the direction reflection query fails but the search succeeds. Just a theory though. An ALC shouldn't be thread-dependent though and I don't think anyone is using something like EnterContextualReflection()

This eliminates the need to have `SetExecutionPolicy` which is a slow function that first queries a bunch of policies and then within PowerShell itself (as in, using a cmdlet) sets it to `Bypass`. However, we just want the process scope (AKA runspace scope) to have the execution policy set to `Bypass` all the time so that we can import our bundled modules, such as PSReadLine.

xUnit infuriatingly defaults to using its own custom app domains (and thus shadow copying of assemblies), which completely messes up our assembly locations, our ability to find modules such as PSReadLine, and resolving types from PSReadLine too. We were not actually configuing xUnit correctly either.

This is sometimes necessary (especially in CI). Co-authored-by: Andrew Schwartzmeyer <[email protected]>

This also simplifies our testing scenario where that path needs to be configured at runtime, too. Co-authored-by: Andrew Schwartzmeyer <[email protected]>

andyleejordan · 2021-07-09T21:54:28Z

So to actually assert that we now respect BundledModulePath threw me on a bit of a loop (thanks @rjmholt for the help). E.g. with this JSON setting:

"powershell.developer.bundledModulesPath": "/Users/andschwa/Downloads/PowerShellEditorServices"

Two things have to be true: 1. the extension must be run in the VS Code debugger (such that "developer" is honored) and 2. a previous dev was "smart" and added silent fallback logic to use the extension's path if the supplied bundledModulesPath did not contain the actual modules, so it can only be tested with a perfect layout. I had originally set it to just /Users/andschwa/ expecting a failure instead of a silent ignoring of the path. It's fine, just unexpected and something I had to learn.

andyleejordan · 2021-07-09T21:58:19Z

src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs

+            // behavior for consistency such that unit tests can pass in a similar environment.
+            if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
+            {
+                initialSessionState.ExecutionPolicy = ExecutionPolicy.Bypass;


@dkattan So we resurrected the SetExecutionPolicy() function because on closer inspection, the actual issue was that in the unit test scenario we weren't having Bypass set (because the codebase's expectation is that the startup script is run with -ExecutionPolicy Bypass, like what the extension does), and the SetExecutionPolicy() actually sets the execution policy back to the original inherited scoped policy, not the process policy. That is, it override's the process/runspace's policy after everything is loaded to be equivalent to the user's machine/user policy.

@rjmholt I might rename that function ResetExecutionPolicy() 😆

That works. I'm a fan of removing large chunks of code, as long as it doesn't change expected functionality :D

This redoes prior work that was lost during the rewrite. Specifically this actually respects the user configuration of `BundledModulePath` (also used by unit tests to provide compatibililty with xUnit), and forces the use of only our bundled PSReadLine dependency. Essentially this redoes #1514 and #1522.

andyleejordan force-pushed the andschwa/tests branch 5 times, most recently from 088fdb6 to 85e60dd Compare July 7, 2021 21:51

andyleejordan added 3 commits July 7, 2021 14:58

Fix unawaited async Task tests by changing to void

361b29e

Revert "Define TEST constant when running tests"

3c4453d

This reverts commit 75a7175. We are reverting this in favor of using a runtime condition set by the test itself because this was fragile (for one, it did not work with OmniSharp running tests).

Fix module path during testing with runtime hook

6d5e709

andyleejordan force-pushed the andschwa/tests branch 5 times, most recently from d41e90e to d03f3d0 Compare July 7, 2021 22:49

andyleejordan marked this pull request as ready for review July 7, 2021 23:33

andyleejordan requested a review from rjmholt as a code owner July 7, 2021 23:33

andyleejordan force-pushed the andschwa/tests branch from e615399 to ed5f1ba Compare July 7, 2021 23:34

andyleejordan commented Jul 8, 2021

View reviewed changes

andyleejordan force-pushed the andschwa/tests branch 2 times, most recently from 5e350a3 to 06893e7 Compare July 8, 2021 23:15

dkattan reviewed Jul 8, 2021

View reviewed changes

dkattan approved these changes Jul 8, 2021

View reviewed changes

andyleejordan commented Jul 9, 2021

View reviewed changes

src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs Outdated Show resolved Hide resolved

rjmholt approved these changes Jul 9, 2021

View reviewed changes

andyleejordan force-pushed the andschwa/tests branch 2 times, most recently from 338c3f4 to 19a9a8f Compare July 9, 2021 20:19

andyleejordan added 2 commits July 9, 2021 14:23

dkattan and others added 2 commits July 9, 2021 14:23

Search all assemblies for PSConsoleReadLine

773e8b6

This is sometimes necessary (especially in CI). Co-authored-by: Andrew Schwartzmeyer <[email protected]>

Respect BundledModulePath user option

f911ef1

This also simplifies our testing scenario where that path needs to be configured at runtime, too. Co-authored-by: Andrew Schwartzmeyer <[email protected]>

andyleejordan force-pushed the andschwa/tests branch from 19a9a8f to f911ef1 Compare July 9, 2021 21:23

andyleejordan merged commit 6f5df13 into master Jul 9, 2021

andyleejordan deleted the andschwa/tests branch July 9, 2021 21:48

andyleejordan commented Jul 9, 2021

View reviewed changes

dkattan mentioned this pull request Jul 13, 2021

Honor BundledModulesPath when loading modules in PowerShellContextService #1516

Closed

andyleejordan added Area-Test Issue-Enhancement A feature request (enhancement). labels Jul 13, 2021

andyleejordan mentioned this pull request Nov 30, 2021

Fix BundledModulePath and PSReadLine loading (redux) #1638

Merged

SydneyhSmith mentioned this pull request Jan 24, 2022

Recover PRs lost in pipeline rewrite rebase #1677

Closed

4 tasks

andyleejordan mentioned this pull request Jun 16, 2022

Regression tests #1733

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run new PSReadLine test on Windows #1522

Run new PSReadLine test on Windows #1522

andyleejordan commented Jul 6, 2021 •

edited

Loading

andyleejordan commented Jul 7, 2021

andyleejordan Jul 8, 2021

dkattan Jul 8, 2021

rjmholt Jul 9, 2021

andyleejordan Jul 8, 2021

dkattan Jul 8, 2021

dkattan left a comment

andyleejordan commented Jul 9, 2021

rjmholt Jul 9, 2021

rjmholt Jul 9, 2021

andyleejordan commented Jul 9, 2021

andyleejordan Jul 9, 2021

andyleejordan Jul 9, 2021

dkattan Jul 9, 2021

		{
		Skip.If(IsWindows, "This test doesn't work on Windows for some reason.");

Run new PSReadLine test on Windows #1522

Run new PSReadLine test on Windows #1522

Conversation

andyleejordan commented Jul 6, 2021 • edited Loading

andyleejordan commented Jul 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkattan left a comment

Choose a reason for hiding this comment

andyleejordan commented Jul 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyleejordan commented Jul 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyleejordan commented Jul 6, 2021 •

edited

Loading