Fixed passing common test modules path to unelevated Powershell #4313

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

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

test/tools/OpenCover/OpenCover.psm1

-    $targetArgsElevated += @("-OutputFile $PesterLogElevated", "-OutputFormat $PesterLogFormat", "-Quiet")
-    $targetArgsUnelevated += @("-OutputFile $PesterLogUnelevated", "-OutputFormat $PesterLogFormat", "-Quiet")
+    $targetArgsElevated += @("-OutputFile $PesterLogElevated", "-OutputFormat $PesterLogFormat")
+    $targetArgsUnelevated += @("-OutputFile $PesterLogUnelevated", "-OutputFormat $PesterLogFormat")


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

-    $openCoverArgsElevated = "-target:$target","-register:user","-output:${outputLog}","-nodefaultfilters","-oldstyle","-hideskipped:all","-mergeoutput", "-filter:`"+[*]* -[Microsoft.PowerShell.PSReadLine]*`"", "-targetargs:`"$targetArgStringElevated`""
-    $openCoverArgsUnelevated = "-target:$target","-register:user","-output:${outputLog}","-nodefaultfilters","-oldstyle","-hideskipped:all", "-mergeoutput", "-filter:`"+[*]* -[Microsoft.PowerShell.PSReadLine]*`"", "-targetargs:`"$targetArgStringUnelevated`""
+    $openCoverArgsElevated = "-target:$target","-register:user","-output:${outputLog}","-nodefaultfilters","-oldstyle","-hideskipped:all","-mergeoutput", "-filter:`"+[*]* -[Microsoft.PowerShell.PSReadLine]*`"", "-targetargs:`"-EncodedCommand $base64targetArgsElevated`""
+    $openCoverArgsUnelevated = "-target:$target","-register:user","-output:${outputLog}","-nodefaultfilters","-oldstyle","-hideskipped:all", "-mergeoutput", "-filter:`"+[*]* -[Microsoft.PowerShell.PSReadLine]*`"", "-targetargs:`"-EncodedCommand $base64targetArgsUnelevated`""


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

@@ -519,6 +520,9 @@ function Invoke-OpenCover
            # poll for process exit every 60 seconds
            # timeout of 6 hours
            $timeOut = ([datetime]::Now).AddHours(6)


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

+    $updatedEnvPath = "${PowerShellExeDirectory}\Modules;$TestToolsModulesPath"
+
+    $startupArgs =  "Set-ExecutionPolicy Bypass -Force -Scope Process; `$env:PSModulePath = '${updatedEnvPath}';"
+    $targetArgs = "${startupArgs}", "Invoke-Pester","${TestDirectory}"


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 · 2017-07-21T00:12:01Z

test/tools/OpenCover/OpenCover.psm1

-    $targetArgsElevated += @("-OutputFile $PesterLogElevated", "-OutputFormat $PesterLogFormat", "-Quiet")
-    $targetArgsUnelevated += @("-OutputFile $PesterLogUnelevated", "-OutputFormat $PesterLogFormat", "-Quiet")
+    $targetArgsElevated += @("-OutputFile $PesterLogElevated", "-OutputFormat $PesterLogFormat")
+    $targetArgsUnelevated += @("-OutputFile $PesterLogUnelevated", "-OutputFormat $PesterLogFormat")


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

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

test/tools/OpenCover/OpenCover.psm1

-    $openCoverArgsElevated = "-target:$target","-register:user","-output:${outputLog}","-nodefaultfilters","-oldstyle","-hideskipped:all","-mergeoutput", "-filter:`"+[*]* -[Microsoft.PowerShell.PSReadLine]*`"", "-targetargs:`"$targetArgStringElevated`""
-    $openCoverArgsUnelevated = "-target:$target","-register:user","-output:${outputLog}","-nodefaultfilters","-oldstyle","-hideskipped:all", "-mergeoutput", "-filter:`"+[*]* -[Microsoft.PowerShell.PSReadLine]*`"", "-targetargs:`"$targetArgStringUnelevated`""
+    $openCoverArgsElevated = "-target:$target","-register:user","-output:${outputLog}","-nodefaultfilters","-oldstyle","-hideskipped:all","-mergeoutput", "-filter:`"+[*]* -[Microsoft.PowerShell.PSReadLine]*`"", "-targetargs:`"-EncodedCommand $base64targetArgsElevated`""
+    $openCoverArgsUnelevated = "-target:$target","-register:user","-output:${outputLog}","-nodefaultfilters","-oldstyle","-hideskipped:all", "-mergeoutput", "-filter:`"+[*]* -[Microsoft.PowerShell.PSReadLine]*`"", "-targetargs:`"-EncodedCommand $base64targetArgsUnelevated`""


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

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

JamesWTruher

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

test/tools/CodeCoverageAutomation/Start-CodeCoverageRun.ps1

    Write-LogPassThru -Message "openCoverPath : $openCoverTargetDirectory\OpenCover"
    Write-LogPassThru -Message "psbinpath : $psBinPath"
    Write-LogPassThru -Message "elevatedLog : $elevatedLogs"
    Write-LogPassThru -Message "unelevatedLog : $unelevatedLogs"
    Write-LogPassThru -Message "TestToolsPath : $testToolsPath"

    $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

        OpenCoverPath = "$openCoverTargetDirectory\OpenCover";
        PowerShellExeDirectory = "$psBinPath";
        PesterLogElevated = $elevatedLogs;
        PesterLogUnelevated = $unelevatedLogs;
        TestToolsModulesPath = "$testToolsPath\Modules";
    }

+    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


            # invoke OpenCover unelevated and poll for completion
-            "$openCoverBin $openCoverArgsUnelevatedString" | Out-File -FilePath "$env:temp\unelevated.ps1" -Force
+            "$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

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

test/tools/CodeCoverageAutomation/Start-CodeCoverageRun.ps1

@@ -1,7 +1,8 @@
 param(
    [Parameter(Mandatory = $true, Position = 0)] $coverallsToken,
    [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>]

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

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

B41A

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

Fixed passing common test modules path to unelevated Powershell #4313

Fixed passing common test modules path to unelevated Powershell #4313

Uh oh!

Conversation

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!