Migrate to netstandard2.0 and PSStandard #741

rjmholt · 2018-09-05T03:38:38Z

UPDATE: The tests are now all passing on all platforms. So I think this PR is ready to be reviewed.

Some outstanding points:

The tests that exist are not great, and need some work. Namely
- A number of tests should be separated out, since they test multiple bits and pieces in the same [Fact]
- A lot of tests should be [Theory]s instead
- A number of big features don't really have any tests
- The AsyncDebouncer interval test was failing intermittently in .NET 4.6.1 and might need looking at
- The paths in tests I fixed with a nasty hack method — ideally we should come up with a more reasonable solution. (I've put this on my internal list of reasons for a C# macro language)
There may be a bug in pathed reference resolution from dot sourced files (.\file.ps1) on *nix, but I don't think this work has caused that, just uncovered it.

Old WIP comments

~~I've migrated PSES to netstandard2.0 and PowerShell Standard and everything seems to work. That's the easy part.~~

But I'm also migrating the tests. I managed to get them to run on Linux by targeting the PSES build to the PowerShell SDK, and doing that I managed to find all the platform-specific parts of the existing tests (which were heavily Windows-oriented) and change them (there's one left that I think is best just commenting out for now, which tests the tooltip for Import-Module).

~~The current problem I'm having is:~~

~~The src project is built against PSStandard, which works fine when it's imported as a module into PowerShell~~
~~But the tests need to emulate the PowerShell hosting to get PowerShellContext to work~~
I tried building the tests against the PowerShell SDK, but I get a System.IO.FileNotFoundException when the tests run and the methods return the default null values from PSStandard's implementation

~~I've tried things like the binding redirect entry in dotnet/standard#481 and making sure I'm using the latest xUnit 2.4.1 preview build, but haven't had any luck there. (See also xunit/xunit#1807)~~

~~I also looked into trying to get the tests to recompile the src against the SDK instead with something like:~~

~~test/PowerShellEditorServices.Test/PowerShellEditorServices.Test.csproj:~~

...
<ItemGroup Condition=" '$(TargetFramework)' == 'netcoreapp2.1' ">
    <ProjectReference Include="..\..\src\PowerShellEditorServices\PowerShellEditorServices.csproj">
        <TargetFramework>netstandard2.0</TargetFramework>
        <TestPlatform>CoreCLR</TestPlatform>
    </ProjectReference>
</ItemGroup>
...

~~src/PowerShellEditorServices/PowerShellEditorServices.csproj:~~

...
<ItemGroup Condition=" '$(TestPlatform)' == '' ">
     <PackageReference Include="PowerShellStandard.Library" Version="5.1.0-preview06" />
</ItemGroup>
<ItemGroup Condition=" '$(TestPlatform)' == 'CoreCLR' ">
    <PackageReference Include="Microsoft.PowerShell.SDK" Version="6.1.0-rc.1" />
</ItemGroup>
...

(I drew upon https://stackoverflow.com/questions/45622882/can-i-pass-compilation-constants-to-a-project-reference and https://stackoverflow.com/questions/48526219/is-there-a-way-to-force-a-project-reference-to-net-standard-project-to-a-specif for info there -- trying to work out how msbuild works is painful...)

~~But this hasn't worked either yet.~~

~~So let me know if you have any suggestions!~~

…ring dependency

src/PowerShellEditorServices/Session/InvocationEventQueue.cs

test/PowerShellEditorServices.Test.Shared/Completion/CompletionExamples.psm1

rjmholt · 2018-09-05T03:48:19Z

test/PowerShellEditorServices.Test.Shared/References/ReferenceFileA.ps1

-. .\ReferenceFileA.ps1
-. .\ReferenceFileB.ps1
-. .\ReferenceFileC.ps1
+. ./ReferenceFileA.ps1


This change shouldn't be needed and I think represents a bug where .NET Core tries to allow \ in a filename, where PowerShell would try to normalise the path... So we should deal with this properly

SeeminglyScience · 2018-09-05T03:58:46Z

For testing, this was the best thing I came up with https://github.com/SeeminglyScience/PowerShellStandard-xunit-demo

And it's pretty awful imo.

rjmholt · 2018-09-05T03:59:52Z

Aha! I thought you might have been down this road @SeeminglyScience!

rjmholt · 2018-09-05T04:02:11Z

Also, I don't actually know how Travis works, but we should change it so that:

It fails the build/CI check when the tests fail, and
It doesn't claim to use XCode on Linux... (not sure what that's about...)

src/PowerShellEditorServices/Session/PowerShellContext.cs

src/PowerShellEditorServices/Session/PowerShell5Operations.cs

rjmholt · 2018-09-05T04:37:58Z

@SeeminglyScience I saw the PrivateAssets field in your example, and that's worked! I added PrivateAssets="all" to the PSStandard references in src projects and that seems to allow xUnit to retarget the bindings when it tests...

rjmholt · 2018-09-05T04:56:51Z

Now just the Host tests remain... 😎

rjmholt · 2018-09-05T07:20:26Z

The host tests are evil and I have disabled them

rjmholt · 2018-09-17T20:04:25Z

So I've tried various ways to coerce the SDK into find the modules it needs to work, mainly by using a powershell.config.json. I've saved my scripts as a gist here: https://gist.github.com/rjmholt/3b9916346af42d260eeeb34512ff5be7.

Unfortunately I just couldn't get them to work.

In the interests in moving forward with this PR, I've just made a test hook to use CreateDefault instead (by setting in the environment PSES_TEST_USE_CREATE_DEFAULT=1).

Hopefully we can come up with a better fix in another PR. In any case, the underlying problem in the SDK will be fixed eventually and we won't need to do any extra work.

rjmholt · 2018-09-17T21:09:33Z

@tylerl0706 @SeeminglyScience so I've added a workaround for the tests. Sadly not what I wanted, but the differences shouldn't be huge. And we still test with CreateDefault in Windows PowerShell.

Anyway, please let me know re approval/changes requested.

rjmholt · 2018-09-18T17:39:04Z

src/PowerShellEditorServices/Language/FindSymbolsVisitor2.cs

@@ -8,18 +8,16 @@

 namespace Microsoft.PowerShell.EditorServices
 {
-#if PowerShellv5
-
    // TODO: Restore this when we figure out how to support multiple


We'll be able to bring this back in after this PR is merged :)

test/PowerShellEditorServices.Test.Shared/Completion/CompletionExamples.psm1

SeeminglyScience

LGTM. Shame xunit testing is so obtuse with PowerShell Standard :/

rjmholt · 2018-09-20T18:06:17Z

Ok this is actually really ready (@tylerl0706)

To create a nested PowerShell instance you can't set the runspace because the target runspace is assumed to be the runspace in TLS.

Fix session details gathering in nested prompts

TylerLeonhardt

Looking good. Just a few more questions 😄

TylerLeonhardt · 2018-10-01T21:14:04Z

PowerShellEditorServices.build.ps1

+
+    if ($DestinationPath -eq $null) {
+        $DestinationPath = Join-Path $tmpDir $DllName
+    } elseif (Test-Path $DestinationPath -PathType Container) {


I think the Test-Path requires that the path exists. I would guess that we wouldn't expect the $DestinationPath to exist at this time.

Not parsing your comment -- possible to rephrase?

I'm not thinking straight. Ignore that comment 😄

However, I wonder if it's worth having an else (which I assume would mean we were given a full file path) that will check if the file's directory location exists

I'm going to invoke YAGNI for now. TBH I think I already over-engineered this function

TylerLeonhardt · 2018-10-01T21:17:07Z

PowerShellEditorServices.build.ps1

+
+    $packageDirPath = Join-Path $tmpDir "$PackageName.$PackageVersion"
+    if (-not (Test-Path $packageDirPath)) {
+        $tmpNupkgPath = Join-Path $tmpDir 'tmp.zip'


maybe instead of using tmp.zip, use like a random GUID so we don't accidentally remove something else. It's sitting in the temp dir... so I don't feel too strongly about this if you don't want to do it.

PowerShellEditorServices.build.ps1

TylerLeonhardt · 2018-10-01T21:19:15Z

PowerShellEditorServices.build.ps1

+    return $DestinationPath
+}
+
+function Invoke-WithCreateDefaultHook {


This makes me sad 😢 but I understand it's needed.

Only for a little while, then when the SDK switches back we'll be good

PowerShellEditorServices.build.ps1

src/PowerShellEditorServices/Session/InvocationEventQueue.cs

src/PowerShellEditorServices/Session/PowerShell5Operations.cs

test/PowerShellEditorServices.Test.Host/ServerTestsBase.cs

test/PowerShellEditorServices.Test.Shared/References/ReferenceFileA.ps1

TylerLeonhardt · 2018-10-01T21:59:59Z

test/PowerShellEditorServices.Test/Session/WorkspaceTests.cs

+#if !CoreCLR
+            string tempDir = Environment.GetEnvironmentVariable("TEMP");
+#else
+            string tempDir = Path.GetTempPath();


I thought this worked in .NET Framework? It looked like it worked in Windows PowerShell

TylerLeonhardt · 2018-10-01T23:17:22Z

PowerShellEditorServices.build.ps1

@@ -135,7 +135,8 @@ function Get-NugetAsmForRuntime {

    $packageDirPath = Join-Path $tmpDir "$PackageName.$PackageVersion"
    if (-not (Test-Path $packageDirPath)) {
-        $tmpNupkgPath = Join-Path $tmpDir 'tmp.zip'
+        $guid = New-Guid
+        $tmpNupkgPath = Join-Path $tmpDir "$guid.zip"


You should probably do the proper clean up here too.

like after you expand-archive

TylerLeonhardt · 2018-10-01T23:18:30Z

PowerShellEditorServices.build.ps1

        exec { & $script:dotnetExe xunit -f $script:TestRuntime.Core --fx-version $script:NetCoreTestingFrameworkVersion }
    }
 }

-task TestHost -If {
+task TestHost {


wait I thought these tests were evil? 😄

They are but ideally we can restore them at some point. The build rule exists but isn't called anywhere (used to be under task Test).

TylerLeonhardt

LGTM - with one little comment on clean up! 🎉

Are we ready for v2 then??

TylerLeonhardt · 2018-10-01T23:50:02Z

🛳 it

rjmholt · 2018-10-01T23:58:58Z

Aight I'm pushing the button. Hold onto your hats

* Move PSES builds to netstandard and use PSStandard APIs * Add shim modules for Windows PowerShell and PowerShell Core 6.0 * Migrate tests to run on .NET Core and on *nix * Make build file more declarative * Use latest xunit * Exclude host integration tests * Clean out csproj files * Fix up Travis build * Retire compatibility APIs for PowerShell v3/4 * Introduce test hook for using InitialSessionState.CreateDefault2() when running on PowerShell Core SDK * Fix session details gathering in nested prompts To create a nested PowerShell instance you can't set the runspace because the target runspace is assumed to be the runspace in TLS.

* Move PSES builds to netstandard and use PSStandard APIs * Add shim modules for Windows PowerShell and PowerShell Core 6.0 * Migrate tests to run on .NET Core and on *nix * Make build file more declarative * Use latest xunit * Exclude host integration tests * Clean out csproj files * Fix up Travis build * Retire compatibility APIs for PowerShell v3/4 * Introduce test hook for using InitialSessionState.CreateDefault2() when running on PowerShell Core SDK * Fix session details gathering in nested prompts To create a nested PowerShell instance you can't set the runspace because the target runspace is assumed to be the runspace in TLS. [Ignore] Fix test typos [Ignore] Use correct AppVeyor version

rjmholt added 10 commits August 29, 2018 15:08

Move PSES builds to netstandard and use PSStandard APIs

dc7cae2

Fix missed net451 reference in build file

624f7fe

First test fixes

3cda894

Add PowerShell SDK references to tests where needed

187aea6

Converted test paths, removed #ifs from src/

bdf008e

Add restore to build rule

3ba9c0a

Unskip some tests, use non-Windows specific tests, lose hash key orde…

3567a2d

…ring dependency

Fix alias tests

15dcb61

Fix platform specific tests, move build to netcoreapp2.1, break things

e622473

Use latest xunit

a929752

rjmholt requested review from TylerLeonhardt, rkeithhill and SeeminglyScience September 5, 2018 03:38

rjmholt commented Sep 5, 2018

View reviewed changes

src/PowerShellEditorServices/Session/InvocationEventQueue.cs Show resolved Hide resolved

rjmholt commented Sep 5, 2018

View reviewed changes

test/PowerShellEditorServices.Test.Shared/Completion/CompletionExamples.psm1 Show resolved Hide resolved

rjmholt commented Sep 5, 2018

View reviewed changes

src/PowerShellEditorServices/Session/PowerShellContext.cs Outdated Show resolved Hide resolved

rjmholt commented Sep 5, 2018

View reviewed changes

src/PowerShellEditorServices/Session/PowerShell5Operations.cs Show resolved Hide resolved

Use PrivateAssets to make PS SDK binding work

05d2f44

rjmholt added 2 commits September 5, 2018 16:28

Exclude host integration tests

f5114c8

Change dotnet xunit to dotnet test in test invocation

e189f87

rjmholt added 2 commits September 5, 2018 17:27

Improve test support on Windows

f220082

Move project references

071b7e0

TylerLeonhardt mentioned this pull request Sep 5, 2018

Enable PSReadline support in the integrated console (syntax highlighting, keyboard navigation) PowerShell/vscode-powershell#535

Closed

Robert Holt added 2 commits September 17, 2018 12:33

Try different permissions set

f9d3018

Give up and use a CreateDefault test hook instead

86601f4

Use dotnet xunit instead of dotnet test

3511ed6

rjmholt mentioned this pull request Sep 18, 2018

Error when profile loads (all hosts) SeeminglyScience/EditorServicesCommandSuite#31

Closed

rjmholt commented Sep 18, 2018

View reviewed changes

SeeminglyScience approved these changes Sep 18, 2018

View reviewed changes

Robert Holt added 2 commits September 20, 2018 10:34

Use simple pwsh process name

c69610e

Change test paths back to ensure backslack supported on *nix

131e51f

SeeminglyScience and others added 2 commits September 26, 2018 17:21

Fix session details gathering in nested prompts

271df79

To create a nested PowerShell instance you can't set the runspace because the target runspace is assumed to be the runspace in TLS.

Fix session details gathering in nested prompts

d99fbb6

Fix session details gathering in nested prompts

glennsarti mentioned this pull request Oct 1, 2018

Get tests working in PowerShell Core & and Non-Windows #648

Closed

TylerLeonhardt reviewed Oct 1, 2018

View reviewed changes

Address @tylerl0706's feedback

784cef5

TylerLeonhardt reviewed Oct 1, 2018

View reviewed changes

TylerLeonhardt approved these changes Oct 1, 2018

View reviewed changes

Clean up temp file for nupkgs

71c9ee9

rjmholt merged commit 67af232 into PowerShell:2.0.0 Oct 2, 2018

rjmholt deleted the migrate-netstandard2.0 branch December 12, 2018 05:59

rjmholt mentioned this pull request Jan 31, 2020

PipeSecurity (netstandard2.0) does not use .NET Framework implementation when loaded in Windows PowerShell dotnet/runtime#27331

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to netstandard2.0 and PSStandard #741

Migrate to netstandard2.0 and PSStandard #741

rjmholt commented Sep 5, 2018 •

edited

Loading

rjmholt Sep 5, 2018

SeeminglyScience commented Sep 5, 2018

rjmholt commented Sep 5, 2018

rjmholt commented Sep 5, 2018

rjmholt commented Sep 5, 2018 •

edited

Loading

rjmholt commented Sep 5, 2018

rjmholt commented Sep 5, 2018

rjmholt commented Sep 17, 2018

rjmholt commented Sep 17, 2018

rjmholt Sep 18, 2018

SeeminglyScience left a comment

rjmholt commented Sep 20, 2018

TylerLeonhardt left a comment

TylerLeonhardt Oct 1, 2018

rjmholt Oct 1, 2018

TylerLeonhardt Oct 1, 2018

rjmholt Oct 1, 2018

TylerLeonhardt Oct 1, 2018

TylerLeonhardt Oct 1, 2018

rjmholt Oct 1, 2018

TylerLeonhardt Oct 1, 2018

TylerLeonhardt Oct 1, 2018

TylerLeonhardt Oct 1, 2018

rjmholt Oct 1, 2018

TylerLeonhardt Oct 1, 2018

rjmholt Oct 1, 2018

TylerLeonhardt left a comment

TylerLeonhardt commented Oct 1, 2018

rjmholt commented Oct 1, 2018

Migrate to netstandard2.0 and PSStandard #741

Migrate to netstandard2.0 and PSStandard #741

Conversation

rjmholt commented Sep 5, 2018 • edited Loading

Old WIP comments

Choose a reason for hiding this comment

SeeminglyScience commented Sep 5, 2018

rjmholt commented Sep 5, 2018

rjmholt commented Sep 5, 2018

rjmholt commented Sep 5, 2018 • edited Loading

rjmholt commented Sep 5, 2018

rjmholt commented Sep 5, 2018

rjmholt commented Sep 17, 2018

rjmholt commented Sep 17, 2018

Choose a reason for hiding this comment

SeeminglyScience left a comment

Choose a reason for hiding this comment

rjmholt commented Sep 20, 2018

TylerLeonhardt left a 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

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

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

TylerLeonhardt left a comment

Choose a reason for hiding this comment

TylerLeonhardt commented Oct 1, 2018

rjmholt commented Oct 1, 2018

rjmholt commented Sep 5, 2018 •

edited

Loading

rjmholt commented Sep 5, 2018 •

edited

Loading