Fixed passing common test modules path to unelevated Powershell by adityapatwardhan · Pull Request #4313 · PowerShell/PowerShell

adityapatwardhan · 2017-07-20T21:30:44Z

Fixed the way common test modules are passed to elevated and unelevated powershell. Earlier, only elevated powershell got those through inheritance as a child process. Now we add them to the startup of the process.
Fixed error reported by PSScriptAnalyzer about ? / Where-Object
Converted all the parameters passed to powershell.exe to be a base64 encoded string to avoid complications with quotes.
Removed code which was updated $env:PSModulePath as we do it in startup args for powershell process instead.
Added a way to disable -Quiet for Pester.

SteveL-MSFT · 2017-07-20T22:31:20Z

test/tools/OpenCover/OpenCover.psm1

Seems like there's lots of redundancy building the args, would it be better to have $targetArgs and then appending the unique parts of Elevated vs Unelevated closer to execution?

Yes, having a look if this can be simplified.

Removed some redundancy by extracting out a function.

If -OutputFormat is the same, why not have it in $targetArgs instead?

SteveL-MSFT · 2017-07-20T22:31:35Z

test/tools/OpenCover/OpenCover.psm1

Seems like there's lots of redundancy building the args, would it be better to have $targetArgs and then appending the unique parts of Elevated vs Unelevated closer to execution?

Yes, having a look if this can be simplified.

I think the only difference here is $base64targetArgs. You could have $openCoverArgs with all the common args, then on line 513, something like:

& $OpenCoverBin ($openCoverArgsElevated + "-targetargs:`"-EncodedCommand $base64targetArgsElevated`"")

SteveL-MSFT · 2017-07-20T22:32:37Z

test/tools/OpenCover/OpenCover.psm1

How long does it typically take? Not clear to me if 6 hours is a good value

It typically takes around 2.5 - 3 hours total, out of which maybe 2 hours for the unelevated one. '6 hours' was completely pulled out of thin air, to be substantially large enough. And we do not wait for 6 hours, we poll to check completion and end the run if complete.

I think it would be worth putting that data in the comment to give context to 6 hours

SteveL-MSFT · 2017-07-20T22:35:46Z

test/tools/OpenCover/OpenCover.psm1

Should we depend on Start-PSPester instead as we've started building more specialization in our test runs in that cmdlet?

Start-PSPester launches powershell.exe, we need to launch opencover.console.exe which would launch powershell.exe to collect coverage numbers. Adding this logic to build.psm1 does not seem right and would complicate the implementation on Start-PSPester further.

For example, start-pspester builds some test exes used by some of the tests. Perhaps not in this PR, but eventually we should probably consider having a separate module for test execution (perhaps merge Start-PSPester and OpenCover module?)

Yes, I like the idea of have a separate module for test execution. It would have 2 modes, with code coverage and regular.

adityapatwardhan · 2017-07-21T00:07:07Z

@SteveL-MSFT Updated. Please have a look.

SteveL-MSFT · 2017-07-21T00:12:01Z

test/tools/OpenCover/OpenCover.psm1

If -OutputFormat is the same, why not have it in $targetArgs instead?

SteveL-MSFT · 2017-07-21T00:18:31Z

test/tools/OpenCover/OpenCover.psm1

I think the only difference here is $base64targetArgs. You could have $openCoverArgs with all the common args, then on line 513, something like:

& $OpenCoverBin ($openCoverArgsElevated + "-targetargs:`"-EncodedCommand $base64targetArgsElevated`"")

adityapatwardhan · 2017-07-21T00:29:14Z

@SteveL-MSFT Had forgotten to push. Please have a look.

adityapatwardhan · 2017-07-21T00:31:29Z

@JamesWTruher Can you have a look too?

JamesWTruher

except for the comment about $SuppressQuiet this is fine

JamesWTruher · 2017-07-21T17:07:31Z

test/tools/CodeCoverageAutomation/Start-CodeCoverageRun.ps1


    $openCoverParams = @{outputlog = $outputLog;
-        TestDirectory = $testPath;
+        TestPath = $testPath;


since these hashtable entries are on separate lines, the ; is not really needed. I don't think it's a big deal, but something to watch in future

JamesWTruher · 2017-07-21T17:09:45Z

test/tools/CodeCoverageAutomation/Start-CodeCoverageRun.ps1


+    if($SuppressQuiet)
+    {
+        $openCoverParams.Add('$SuppressQuiet', $true)


should $SuppressQuiet be SuppressQuiet?

Good catch. Fixed.

JamesWTruher · 2017-07-21T17:17:33Z

test/tools/OpenCover/OpenCover.psm1

+    $base64targetArgs = [convert]::ToBase64String($bytes)
+
+    # the order seems to be important. Always keep -targetargs as the last parameter.
+    $cmdline = "-target:$target","-register:user","-output:${outputLog}","-nodefaultfilters","-oldstyle","-hideskipped:all","-mergeoutput", "-filter:`"+[*]* -[Microsoft.PowerShell.PSReadLine]*`"", "-targetargs:`"-EncodedCommand $base64targetArgs`""


having these all on one line is a difficult to read, this would work as well:

$cmdline = "-target:$target", "-register:user", "-output:${outputLog}", "-nodefaultfilters", "-oldstyle", "-hideskipped:all", "-mergeoutput", "-filter:`"+[*]* -[Microsoft.PowerShell.PSReadLine]*`"", "-targetargs:`"-EncodedCommand $base64targetArgs`""

Also converted it to a single string using -join " " before returning

JamesWTruher · 2017-07-21T17:39:08Z

test/tools/OpenCover/OpenCover.psm1

+            "$openCoverBin $cmdlineUnelevated" | Out-File -FilePath "$env:temp\unelevated.ps1" -Force
            runas.exe /trustlevel:0x20000 "powershell.exe -file $env:temp\unelevated.ps1"
            # wait for process to start
            Start-Sleep -Seconds 5


this 5 second sleep is probably not needed since you have the sleep in line 515

Removed the wait.

adityapatwardhan · 2017-07-21T17:58:26Z

@SteveL-MSFT @JamesWTruher I have addressed all feedback, please have a look.

SteveL-MSFT

LGTM

daxian-dbw · 2017-07-24T04:19:17Z

test/tools/CodeCoverageAutomation/Start-CodeCoverageRun.ps1

    [Parameter(Mandatory = $true, Position = 1)] $codecovToken,
-    [Parameter(Position = 2)] $azureLogDrive = "L:\"
+    [Parameter(Position = 2)] $azureLogDrive = "L:\",
+    [Parameter] [switch] $SuppressQuiet


This will make the parameter type of SuppressQuiet be [parameter], for example:

function bar { param( [Parameter] [switch] $SuppressQuiet ) } PS> gcm bar -Syntax bar [[-SuppressQuiet] <Parameter>]

Opencover.console.exe gets confused when the base64 encoded parameter is given with '&' invoke. Writing to a ps1 file and invoking the script works around the issue. This also makes it similar to how unelevated tests are invoked.

adityapatwardhan · 2017-08-03T22:43:16Z

@daxian-dbw @SteveL-MSFT please have another look, I have addressed the feedback and added another bug fix.

adityapatwardhan requested a review from JamesWTruher July 20, 2017 21:30

msftclas added the cla-not-required label Jul 20, 2017

SteveL-MSFT requested changes Jul 20, 2017

View reviewed changes

SteveL-MSFT requested changes Jul 21, 2017

View reviewed changes

adityapatwardhan added 4 commits July 21, 2017 09:42

Fixed passing common test modules path to unelevated Powershell

ab2df91

Added way to disable -Quiet for Pester

e7ccdf2

Addressed CR comments

36ff414

Changed parameter -Quiet to -SuppressQuiet

517d218

adityapatwardhan force-pushed the OpenCoverCommonModulesPath branch from 6311b6b to 517d218 Compare July 21, 2017 16:43

JamesWTruher approved these changes Jul 21, 2017

View reviewed changes

Addressed CR feedback

12f10ae

SteveL-MSFT approved these changes Jul 21, 2017

View reviewed changes

daxian-dbw reviewed Jul 24, 2017

View reviewed changes

adityapatwardhan added 2 commits August 2, 2017 14:05

Removed Parameter type

796ba94

SteveL-MSFT approved these changes Aug 4, 2017

View reviewed changes

daxian-dbw merged commit 5cd0e85 into PowerShell:master Aug 4, 2017

adityapatwardhan deleted the OpenCoverCommonModulesPath branch August 4, 2017 17:08

Conversation

adityapatwardhan commented Jul 20, 2017 • edited 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adityapatwardhan Jul 20, 2017 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adityapatwardhan commented Jul 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adityapatwardhan commented Jul 21, 2017

Uh oh!

adityapatwardhan commented Jul 21, 2017

Uh oh!

JamesWTruher left a comment

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!

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!

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!

adityapatwardhan commented Jul 21, 2017

Uh oh!

SteveL-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Jul 24, 2017 • 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!

adityapatwardhan commented Aug 3, 2017

Uh oh!

Reviewers

Assignees

adityapatwardhan commented Jul 20, 2017 •

edited

Loading

adityapatwardhan Jul 20, 2017 •

edited

Loading

daxian-dbw Jul 24, 2017 •

edited

Loading