★ wanayoo — archive 1999 https://github.com/PowerShell/PowerShellEditorServices/pull/741Nouvelle recherche | Portail wanayoo
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate to netstandard2.0 and PSStandard #741

Merged
merged 70 commits into from Oct 2, 2018

Conversation

@rjmholt
Copy link
Member

@rjmholt rjmholt commented Sep 5, 2018

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!

. .\ReferenceFileA.ps1
. .\ReferenceFileB.ps1
. .\ReferenceFileC.ps1
. ./ReferenceFileA.ps1

This comment has been minimized.

@rjmholt

rjmholt Sep 5, 2018
Author Member

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
Copy link
Collaborator

@SeeminglyScience SeeminglyScience commented Sep 5, 2018

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
Copy link
Member Author

@rjmholt rjmholt commented Sep 5, 2018

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

@rjmholt
Copy link
Member Author

@rjmholt rjmholt commented Sep 5, 2018

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

  1. It fails the build/CI check when the tests fail, and
  2. It doesn't claim to use XCode on Linux... (not sure what that's about...)
@rjmholt
Copy link
Member Author

@rjmholt rjmholt commented Sep 5, 2018

@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
Copy link
Member Author

@rjmholt rjmholt commented Sep 5, 2018

Now just the Host tests remain... 😎

@rjmholt
Copy link
Member Author

@rjmholt rjmholt commented Sep 5, 2018

The host tests are evil and I have disabled them

rjmholt added 2 commits Sep 5, 2018
@rjmholt
Copy link
Member Author

@rjmholt rjmholt commented Sep 17, 2018

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
Copy link
Member Author

@rjmholt rjmholt commented Sep 17, 2018

@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.

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

namespace Microsoft.PowerShell.EditorServices
{
#if PowerShellv5

// TODO: Restore this when we figure out how to support multiple

This comment has been minimized.

@rjmholt

rjmholt Sep 18, 2018
Author Member

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

Copy link
Collaborator

@SeeminglyScience SeeminglyScience left a comment

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

@rjmholt
Copy link
Member Author

@rjmholt rjmholt commented Sep 20, 2018

Ok this is actually really ready (@tylerl0706)

SeeminglyScience and others added 2 commits Sep 26, 2018
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
Copy link
Member

@TylerLeonhardt TylerLeonhardt left a comment

Looking good. Just a few more questions 😄


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

This comment has been minimized.

@TylerLeonhardt

TylerLeonhardt Oct 1, 2018
Member

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.

This comment has been minimized.

@rjmholt

rjmholt Oct 1, 2018
Author Member

Not parsing your comment -- possible to rephrase?

This comment has been minimized.

@TylerLeonhardt

TylerLeonhardt Oct 1, 2018
Member

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

This comment has been minimized.

@rjmholt

rjmholt Oct 1, 2018
Author Member

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


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

This comment has been minimized.

@TylerLeonhardt

TylerLeonhardt Oct 1, 2018
Member

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 Show resolved Hide resolved
return $DestinationPath
}

function Invoke-WithCreateDefaultHook {

This comment has been minimized.

@TylerLeonhardt

TylerLeonhardt Oct 1, 2018
Member

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

This comment has been minimized.

@rjmholt

rjmholt Oct 1, 2018
Author Member

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

PowerShellEditorServices.build.ps1 Show resolved Hide resolved
#if !CoreCLR
string tempDir = Environment.GetEnvironmentVariable("TEMP");
#else
string tempDir = Path.GetTempPath();

This comment has been minimized.

@TylerLeonhardt

TylerLeonhardt Oct 1, 2018
Member

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

@@ -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"

This comment has been minimized.

@TylerLeonhardt

TylerLeonhardt Oct 1, 2018
Member

You should probably do the proper clean up here too.

This comment has been minimized.

@TylerLeonhardt

TylerLeonhardt Oct 1, 2018
Member

like after you expand-archive

This comment has been minimized.

@rjmholt

rjmholt Oct 1, 2018
Author Member

Done!

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

task TestHost -If {
task TestHost {

This comment has been minimized.

@TylerLeonhardt

TylerLeonhardt Oct 1, 2018
Member

wait I thought these tests were evil? 😄

This comment has been minimized.

@rjmholt

rjmholt Oct 1, 2018
Author Member

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).

Copy link
Member

@TylerLeonhardt TylerLeonhardt left a comment

LGTM - with one little comment on clean up! 🎉

Are we ready for v2 then??

@TylerLeonhardt
Copy link
Member

@TylerLeonhardt TylerLeonhardt commented Oct 1, 2018

🛳 it

@rjmholt
Copy link
Member Author

@rjmholt rjmholt commented Oct 1, 2018

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

@rjmholt rjmholt merged commit 67af232 into PowerShell:2.0.0 Oct 2, 2018
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
rjmholt added a commit to rjmholt/PowerShellEditorServices that referenced this pull request Oct 2, 2018
* 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.
rjmholt added a commit to rjmholt/PowerShellEditorServices that referenced this pull request Oct 2, 2018
* 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.
@rjmholt rjmholt deleted the rjmholt:migrate-netstandard2.0 branch Dec 12, 2018
TylerLeonhardt added a commit to TylerLeonhardt/PowerShellEditorServices that referenced this pull request Dec 14, 2018
* 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.
rjmholt added a commit to rjmholt/PowerShellEditorServices that referenced this pull request Jan 18, 2019
* 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.
rjmholt added a commit to rjmholt/PowerShellEditorServices that referenced this pull request Jan 18, 2019
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.