Skip to content

Move tests to PS7 and PS7.1 and fix IsNetCore check #1318

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

Merged

Conversation

TylerLeonhardt
Copy link
Member

@TylerLeonhardt TylerLeonhardt commented Jun 30, 2020

Fixes #1317

Fix IsNetCore logic, add a test for it that failed before the code change.

Also, now our tests run for PS7 and PS7.1.

I've removed PS6 as a target because:

  • It's almost EOL Sept
  • I had some troubles with the build I didn't want to dig into further
  • PS7 is gaining over PS6 on usage

@TylerLeonhardt TylerLeonhardt requested a review from rjmholt as a code owner June 30, 2020 04:15
completionResults.Completions[0].CompletionType
);

Assert.NotNull(completionResults.Completions[0].ToolTipText);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, the signature of Get-Random changed so I'm asserting on less things but I think it's fine.

@ghost ghost added Area-Startup Issue-Bug A bug to squash. labels Jun 30, 2020
@TylerLeonhardt TylerLeonhardt marked this pull request as draft June 30, 2020 04:25

task TestServerPS71 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nice -- should help us ensure forward compat


namespace Microsoft.PowerShell.EditorServices.Test.Utility
{
public class VersionUtilsTests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

@TylerLeonhardt
Copy link
Member Author

I had to remove the global.json since we use different SDKs in different places... not sure if there's a way to configure it correctly...

@@ -303,10 +320,6 @@ task LayoutModule -After Build {
# Copy Third Party Notices.txt to module folder
Copy-Item -Force -Path "$PSScriptRoot\Third Party Notices.txt" -Destination $psesOutputPath

# Copy UnixConsoleEcho native libraries
Copy-Item -Path "$script:PsesOutput/runtimes/osx-64/native/*" -Destination $psesDepsPath -Force
Copy-Item -Path "$script:PsesOutput/runtimes/linux-64/native/*" -Destination $psesDepsPath -Force
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't seem to be needed anymore... which is kinda odd to me... but moving to a newer SDK seems to not generate these folders anymore.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've had errors about those folders being missing since they were added tbh. I assumed they were important to CI but I've just been commenting them out.

If the UnixConsoleEcho dlls still end up where they need to be then 🤷 probably remove it

@TylerLeonhardt
Copy link
Member Author

hmm what happened to Codacy?

@TylerLeonhardt TylerLeonhardt marked this pull request as ready for review June 30, 2020 16:56
Comment on lines 77 to 78
'-Version'
$Version
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is arguably clearer as an array like @('-Version', $Version) so it's obvious that you're using array splatting

@TylerLeonhardt TylerLeonhardt merged commit 67987fc into PowerShell:master Jul 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VersionUtils.IsNetCore returns false in net 5.0, breaking LoadAssemblyInPsesLoadContext
3 participants