Fix Enter-PSHostProcess tests flakiness #9007

TylerLeonhardt · 2019-02-28T07:04:23Z

PR Summary

This PR totally refactors the Enter-PSHostProcess tests.

PR Context

The biggest change was that rather then sharing an instance of PowerShell, the tests will each have their own instance of PowerShell to interact with.

PR Checklist

TylerLeonhardt · 2019-02-28T07:06:26Z

Just to be sure, I'd like to run CI a few times to see if the failure shows up. I'm going to 😴but if someone in some other place (where it's not late) wants to kick them off again by adding commits or something, that'd be helpful!

iSazonov · 2019-02-28T12:38:18Z

I'd can restart but it is already failed :-(

TylerLeonhardt · 2019-02-28T14:47:45Z

Jeez... OK... Ran this like 20 times in a container and couldn't repro the issue.

Back to the drawing board...

TylerLeonhardt · 2019-02-28T15:10:12Z

I can repro the same problem if I do this:

> "Enter-PSHostProcess -Id 40328 -ErrorAction Stop
                  `$pid
                  Exit-PSHostProcess" >  test.ps1

> ./test.ps1

iSazonov · 2019-02-28T15:16:37Z

Looks tricky!
Perhaps @JamesWTruher has ideas how make the tests reliable.

TylerLeonhardt · 2019-02-28T15:17:57Z

The odd thing is... I added -ErrorAction Stop to the Enter-PSHostProcess's so this this failure is even more peculiar because $pid should never get run. It should stop executing when Enter-PSHostProcess fails...

"Enter-PSHostProcess -Id $($pwsh.Id) -ErrorAction Stop
`$pid
Exit-PSHostProcess" | pwsh -c - | Should -Be $pwsh.Id

TEST FAILURES
Description: Can enter, exit, and re-enter another PSHost
Name:        Enter-PSHostProcess tests.By Process Id.Can enter, exit, and re-enter another PSHost
message:
Expected 872, but got '1432'.
stack-trace:
at <ScriptBlock>, D:\a\1\s\test\powershell\Modules\Microsoft.PowerShell.Core\Enter-PSHostProcess.Tests.ps1: line 47
47:                 Exit-PSHostProcess" | pwsh -c - | Should -Be $pwsh.Id

In my local testing... I noticed that:

Expected 872, but got '1432'.

1432 was typically the $pid of the pwsh that executed Enter-PSHostProcess but that could be a red herring

iSazonov · 2019-02-28T15:24:06Z

@TylerLeonhardt Maybe #1995?

vexx32 · 2019-02-28T15:27:14Z

@TylerLeonhardt does this repro the issue?

@'
try {{
    Enter-PSHostProcess -Id {0} -ErrorAction Stop
    $pid
}}
finally {{
    Exit-PSHostProcess
}}
'@ -f $pwsh.Id | pwsh -c - | Should -Be $pwsh.Id

In other words, is the -ErrorAction Stop behaving as we expect?

RDIL · 2019-02-28T15:27:53Z

test/powershell/Modules/Microsoft.PowerShell.Core/Enter-PSHostProcess.Tests.ps1


-        It "Should throw if CustomPipeName is too long on Linux or macOS" {
+        It "Should throw if CustomPipeName is too long on Linux or macOS" -Skip:($IsWindows) {
            $longPipeName = "DoggoipsumwaggywagssmolborkingdoggowithalongsnootforpatsdoingmeafrightenporgoYapperporgolongwatershoobcloudsbigolpupperlengthboy"


Why is this a thing?
(just wondering)

because on unix they're not called "Named Pipes" they're called "UNIX domain sockets" which have a limit on macOS and Linux.

I meant $longPipeName = "DoggoipsumwaggywagssmolborkingdoggowithalongsnootforpatsdoingmeafrightenporgoYapperporgolongwatershoobcloudsbigolpupperlengthboy"

+1 to generate needed length and add a comment.

TylerLeonhardt · 2019-02-28T15:29:19Z

@iSazonov Maybe... but I had 100% success rate from 20 runs on my mac and in an ubuntu container.

@vexx32 I'm open to give that a try... the test should still fail, but maybe we'll get more info as to why (also edited your comment to escape the {})

iSazonov · 2019-02-28T15:39:31Z

@TylerLeonhardt I suggest replace "-c" with "-file" to avoid escaping problem.

iSazonov · 2019-02-28T15:44:56Z

test.ps1

@@ -0,0 +1,3 @@
+Enter-PSHostProcess -Id 40328 -ErrorAction Stop
+                $pid
+                Exit-PSHostProcess


Please remove.

will do. Still getting data. moved to WIP

TylerLeonhardt · 2019-02-28T16:07:40Z

@iSazonov

I suggest replace "-c" with "-file" to avoid escaping problem.

Sorry, where exactly?

vexx32 · 2019-02-28T16:21:27Z

test/powershell/Modules/Microsoft.PowerShell.Core/Enter-PSHostProcess.Tests.ps1

+}}
+finally {{
+    Exit-PSHostProcess
+}}


double braces?

You need this for template strings to escape { and } otherwise it throws a bad formatting exception

TylerLeonhardt · 2019-02-28T16:59:15Z

jeez...

Description: Can enter, exit, and re-enter another PSHost
Name:        Enter-PSHostProcess tests.By Process Id.Can enter, exit, and re-enter another PSHost
message:
Expected 4564, but got $null.
stack-trace:
at <ScriptBlock>, D:\a\1\s\test\powershell\Modules\Microsoft.PowerShell.Core\Enter-PSHostProcess.Tests.ps1: line 56
56: '@ -f $pwsh.Id | pwsh -c - | Should -Be $pwsh.Id
Description: Can enter and exit another Windows PowerShell PSHost
Name:        Enter-PSHostProcess tests.By Process Id.Can enter and exit another Windows PowerShell PSHost
message:
Expected 3752, but got $null.
stack-trace:
at <ScriptBlock>, D:\a\1\s\test\powershell\Modules\Microsoft.PowerShell.Core\Enter-PSHostProcess.Tests.ps1: line 93
93: '@ -f $powershell.Id | pwsh -c - | Should -Be $powershell.Id
Description: Can enter, exit, and re-enter using CustomPipeName
Name:        Enter-PSHostProcess tests.By CustomPipeName.Can enter, exit, and re-enter using CustomPipeName
message:
Expected 360, but got $null.
stack-trace:
at <ScriptBlock>, D:\a\1\s\test\powershell\Modules\Microsoft.PowerShell.Core\Enter-PSHostProcess.Tests.ps1: line 139
139: '@ -f $pipeName | pwsh -c - | Should -Be $pwsh.Id
3 tests in test/powershell failed
At D:\a\1\s\build.psm1:1371 char:13
+             throw "$($x.'test-results'.failures) tests in $TestArea f ...
+             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : OperationStopped: (3 tests in test/powershell failed:String) [], RuntimeException
    + FullyQualifiedErrorId : 3 tests in test/powershell failed

iSazonov · 2019-02-28T17:08:52Z

test/powershell/Modules/Microsoft.PowerShell.Core/Enter-PSHostProcess.Tests.ps1


-        }
+        It "Can enter, exit, and re-enter another PSHost" {
+            # $pwsh = Start-PSProcess


Why commented?

You should probably just hold off on reviewing. This issue only repros in CI... so it'll be a lot of trial and error.

When I remove WIP, you can review for real.

Open to suggestions on how to fix the tests though 😅

My suggestion was above - use "-file" parameter and put the temporary test script in file.

TylerLeonhardt · 2019-03-01T08:45:58Z

If this succeeds CI, please restart a few times. I need 😴

If this works, I'll need to add a test to Startup.Tests.ps1 to test that pwsh -CustomPipeName foo actually works. And can move the last test into that as well.

TylerLeonhardt · 2019-03-01T09:08:25Z

it... passed once so far. Triggered again with some comments... now time for bed.

iSazonov · 2019-03-01T12:07:04Z

I restarted CI-windows as you ask (previous was Ok).

test/powershell/Modules/Microsoft.PowerShell.Core/Enter-PSHostProcess.Tests.ps1

PaulHigin · 2019-03-05T22:59:40Z

test/powershell/Modules/Microsoft.PowerShell.Core/Enter-PSHostProcess.Tests.ps1

 # Copyright (c) Microsoft Corporation. All rights reserved.
 # Licensed under the MIT License.

+function Wait-JobPid {


It seems like we should have a count limit in the loop, in case the script fails for some reason, and throw on failure.

added timeout

test/powershell/Modules/Microsoft.PowerShell.Core/Enter-PSHostProcess.Tests.ps1

TylerLeonhardt · 2019-03-06T23:57:33Z

I spoke to @PaulHigin about this and we agreed that since the tests have been reliable in this PR, we'll keep them in. If they become a problem again, we can remove the ones that use *-PSHostProcess

…ardt/PowerShell into fix-enter-pshostprocess-tests

adityapatwardhan · 2019-03-07T18:04:28Z

test/powershell/Host/ConsoleHost.Tests.ps1

+            $pipePath = Get-PipePath $pipeName
+
+            # The pipePath should be created by the time the -Command is executed.
+            pwsh -CustomPipeName $pipeName -Command "Test-Path '$pipePath'" | Should -BeTrue


use $powershell instead. Using pwsh will call the installed pwsh on the system, not the one which we build.

If what you say is true, then none of my CustomPipeName tests would be passing, right? I'm willing to change it though. Is that a magic variable or something?

Probably set during the build or on test initialization with Start-PSPester, I'd imagine. 🙂

Oh I see what you're saying now :)

I've fixed this so it's consistent

adityapatwardhan · 2019-03-07T18:05:56Z

test/powershell/Host/ConsoleHost.Tests.ps1

+
+            "`$pid" | & $powershell -CustomPipeName $longPipeName -c -
+            # 64 is the ExitCode for BadCommandLineParameter
+            $LASTEXITCODE | Should -Be 64


You can check for FullyQualifiedErrorId that is not localized.

adityapatwardhan · 2019-03-07T18:16:37Z

test/powershell/Modules/Microsoft.PowerShell.Core/Enter-PSHostProcess.Tests.ps1

+                    Should -BeTrue -Because "The script was able to re-enter another process and grab the pid of '$powershellId'."
+
+            } finally {
+                $powershellJob | Stop-Process -Force -ErrorAction SilentlyContinue


Should this be Stop-Job | Remove-Job instead?

you're totally right. Added.

adityapatwardhan · 2019-03-07T18:17:47Z

test/powershell/Modules/Microsoft.PowerShell.Core/Enter-PSHostProcess.Tests.ps1

-                $ps.AddScript('$pid').Invoke() | Should -Be $pwsh.Id
+                $ps.AddScript('$pid').Invoke() | Should -Be $pwshId
            } finally {
                $rs.Dispose()


Also add $ps.Dispose()

adityapatwardhan · 2019-03-07T20:38:46Z

@TylerLeonhardt - To fix the static analysis CI - update the link to https://www.itprotoday.com/powershell/powershell-basics-remote-management

adityapatwardhan · 2019-03-07T20:39:32Z

Restarted Windows CI

adityapatwardhan · 2019-03-07T21:26:55Z

@TravisEz13 can you update your review?

…ardt/PowerShell into fix-enter-pshostprocess-tests

adityapatwardhan · 2019-03-07T23:23:29Z

@TylerLeonhardt Just realized the tests that were changed are tagged as Feature. Please push an empty commit with the commit message as [Feature] to execute those tests.

…ardt/PowerShell into fix-enter-pshostprocess-tests

TylerLeonhardt · 2019-03-08T00:08:48Z

triggered

TravisEz13 · 2019-11-22T22:17:13Z

This has become unreliable again. I have filed #11168

[feature] fix enter-pshostprocess tests

6cb0faa

RDIL reviewed Feb 28, 2019

View reviewed changes

get more info

63fba3c

iSazonov reviewed Feb 28, 2019

View reviewed changes

TylerLeonhardt changed the title ~~Fix Enter-PSHostProcess tests flakiness~~ WIP: Fix Enter-PSHostProcess tests flakiness Feb 28, 2019

write the errors

f7de624

vexx32 reviewed Feb 28, 2019

View reviewed changes

remote into this machine

e6e9259

iSazonov reviewed Feb 28, 2019

View reviewed changes

TylerLeonhardt and others added 5 commits February 28, 2019 21:42

possible fix?

a710574

check platform for better logs on *nix

9a491bc

sleeps

36a0741

wip: switch to jobs

77f80c3

switch to all jobs

93d2a2b

add a couple comments

ab37807

PaulHigin suggested changes Mar 5, 2019

View reviewed changes

PaulHigin approved these changes Mar 6, 2019

View reviewed changes

TylerLeonhardt added 2 commits March 6, 2019 16:22

address all feedback

af77dff

Merge branch 'fix-enter-pshostprocess-tests' of github.com:TylerLeonh…

b5f298b

…ardt/PowerShell into fix-enter-pshostprocess-tests

iSazonov assigned adityapatwardhan Mar 7, 2019

iSazonov added the CL-Test Indicates that a PR should be marked as a test change in the Change Log label Mar 7, 2019

adityapatwardhan requested changes Mar 7, 2019

View reviewed changes

address aditya's feedback

3e8f707

adityapatwardhan approved these changes Mar 7, 2019

View reviewed changes

TylerLeonhardt requested a review from TravisEz13 March 7, 2019 22:33

TylerLeonhardt added 3 commits March 7, 2019 14:38

fixed two urls

009a76b

Merge branch 'fix-enter-pshostprocess-tests' of github.com:TylerLeonh…

bc12333

…ardt/PowerShell into fix-enter-pshostprocess-tests

Merge branch 'master' into fix-enter-pshostprocess-tests

c9a3438

TravisEz13 approved these changes Mar 7, 2019

View reviewed changes

TylerLeonhardt added 3 commits March 7, 2019 16:06

[feature] for CI

de0b604

Merge branch 'fix-enter-pshostprocess-tests' of github.com:TylerLeonh…

54b8409

…ardt/PowerShell into fix-enter-pshostprocess-tests

[feature] for CI

d7c0d1c

adityapatwardhan merged commit a26d639 into PowerShell:master Mar 11, 2019

TylerLeonhardt deleted the fix-enter-pshostprocess-tests branch March 11, 2019 18:15

daxian-dbw added this to the 6.2.0-GA-Consider milestone Mar 11, 2019

TravisEz13 added a commit that referenced this pull request Mar 21, 2019

Fix Enter-PSHostProcess tests flakiness (#9007)

e0d5dfd

TravisEz13 modified the milestones: 6.2.0-GA-Consider, 6.2.0 Mar 21, 2019

Fix Enter-PSHostProcess tests flakiness #9007

Fix Enter-PSHostProcess tests flakiness #9007

Uh oh!

Conversation

TylerLeonhardt commented Feb 28, 2019

PR Summary

PR Context

PR Checklist

Uh oh!

TylerLeonhardt commented Feb 28, 2019

Uh oh!

iSazonov commented Feb 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TylerLeonhardt commented Feb 28, 2019

Uh oh!

TylerLeonhardt commented Feb 28, 2019

Uh oh!

iSazonov commented Feb 28, 2019

Uh oh!

TylerLeonhardt commented Feb 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Feb 28, 2019

Uh oh!

vexx32 commented Feb 28, 2019 • edited by TylerLeonhardt Loading Uh oh! There was an error while loading. Please reload this page.

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!

iSazonov Feb 28, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

TylerLeonhardt commented Feb 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Feb 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TylerLeonhardt commented Feb 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TylerLeonhardt Feb 28, 2019 • 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!

TylerLeonhardt commented Feb 28, 2019

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!

TylerLeonhardt commented Mar 1, 2019

Uh oh!

TylerLeonhardt commented Mar 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Mar 1, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TylerLeonhardt commented Mar 6, 2019

Uh oh!

iSazonov commented Feb 28, 2019 •

edited

Loading

TylerLeonhardt commented Feb 28, 2019 •

edited

Loading

vexx32 commented Feb 28, 2019 •

edited by TylerLeonhardt

Loading

iSazonov Feb 28, 2019 •

edited

Loading

TylerLeonhardt commented Feb 28, 2019 •

edited

Loading

TylerLeonhardt Feb 28, 2019 •

edited

Loading

TylerLeonhardt commented Mar 1, 2019 •

edited

Loading