GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Fixed passing common test modules path to unelevated Powershell by adityapatwardhan · Pull Request #4313 · PowerShell/PowerShell

Conversation

@adityapatwardhan

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) commented Jul 20, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@adityapatwardhan
Aditya Patwardhan (adityapatwardhan) requested a review from James Truher (JamesWTruher) July 20, 2017 21:30
@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Jul 20, 2017
SteveL-MSFT
Steve Lee (SteveL-MSFT) requested changes Jul 20, 2017
View reviewed changes
Comment thread test/tools/OpenCover/OpenCover.psm1 Outdated

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jul 20, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Jul 20, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Yes, having a look if this can be simplified.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Jul 20, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Removed some redundancy by extracting out a function.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jul 21, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/tools/OpenCover/OpenCover.psm1 Outdated

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jul 20, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Jul 20, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Yes, having a look if this can be simplified.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jul 21, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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`"")

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/tools/OpenCover/OpenCover.psm1 Outdated

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jul 20, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Jul 20, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jul 20, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Jul 21, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/tools/OpenCover/OpenCover.psm1 Outdated

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jul 20, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Jul 20, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jul 20, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
JamesWTruher reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Jul 20, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@adityapatwardhan

Aditya Patwardhan (adityapatwardhan) commented Jul 21, 2017

Copy link
Copy Markdown
Member Author

Steve Lee (@SteveL-MSFT) Updated. Please have a look.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
SteveL-MSFT
Steve Lee (SteveL-MSFT) requested changes Jul 21, 2017
View reviewed changes
Comment thread test/tools/OpenCover/OpenCover.psm1 Outdated

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jul 21, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/tools/OpenCover/OpenCover.psm1 Outdated

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jul 21, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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`"")

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@adityapatwardhan

Aditya Patwardhan (adityapatwardhan) commented Jul 21, 2017

Copy link
Copy Markdown
Member Author

Steve Lee (@SteveL-MSFT) Had forgotten to push. Please have a look.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@adityapatwardhan

Aditya Patwardhan (adityapatwardhan) commented Jul 21, 2017

Copy link
Copy Markdown
Member Author

James Truher (@JamesWTruher) Can you have a look too?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Aditya Patwardhan (adityapatwardhan) added 4 commits July 21, 2017 09:42
@adityapatwardhan
Fixed passing common test modules path to unelevated Powershell
ab2df91
@adityapatwardhan
Added way to disable -Quiet for Pester
e7ccdf2
@adityapatwardhan
Addressed CR comments
36ff414
@adityapatwardhan
Changed parameter -Quiet to -SuppressQuiet
517d218
@adityapatwardhan
Aditya Patwardhan (adityapatwardhan) force-pushed the OpenCoverCommonModulesPath branch from 6311b6b to 517d218 Compare July 21, 2017 16:43
JamesWTruher
James Truher (JamesWTruher) approved these changes Jul 21, 2017
View reviewed changes

@JamesWTruher James Truher (JamesWTruher) left a comment

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

except for the comment about $SuppressQuiet this is fine

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/tools/CodeCoverageAutomation/Start-CodeCoverageRun.ps1

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

@JamesWTruher James Truher (JamesWTruher) Jul 21, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/tools/CodeCoverageAutomation/Start-CodeCoverageRun.ps1 Outdated

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

@JamesWTruher James Truher (JamesWTruher) Jul 21, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

should $SuppressQuiet be SuppressQuiet?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Jul 21, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Good catch. Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/tools/OpenCover/OpenCover.psm1 Outdated
$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`""

@JamesWTruher James Truher (JamesWTruher) Jul 21, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Jul 21, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Jul 21, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/tools/OpenCover/OpenCover.psm1 Outdated
"$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

@JamesWTruher James Truher (JamesWTruher) Jul 21, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Jul 21, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Removed the wait.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@adityapatwardhan
Addressed CR feedback
12f10ae
@adityapatwardhan

Aditya Patwardhan (adityapatwardhan) commented Jul 21, 2017

Copy link
Copy Markdown
Member Author

Steve Lee (@SteveL-MSFT) James Truher (@JamesWTruher) I have addressed all feedback, please have a look.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
SteveL-MSFT
Steve Lee (SteveL-MSFT) approved these changes Jul 21, 2017
View reviewed changes

@SteveL-MSFT Steve Lee (SteveL-MSFT) left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
daxian-dbw
Dongbo Wang (daxian-dbw) reviewed Jul 24, 2017
View reviewed changes
Comment thread test/tools/CodeCoverageAutomation/Start-CodeCoverageRun.ps1 Outdated
[Parameter(Mandatory = $true, Position = 1)] $codecovToken,
[Parameter(Position = 2)] $azureLogDrive = "L:\"
[Parameter(Position = 2)] $azureLogDrive = "L:\",
[Parameter] [switch] $SuppressQuiet

@daxian-dbw Dongbo Wang (daxian-dbw) Jul 24, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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>]

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Aug 2, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Aditya Patwardhan (adityapatwardhan) added 2 commits August 2, 2017 14:05
@adityapatwardhan
Removed Parameter type
796ba94
@adityapatwardhan
Fix unrecognized cmdline parameter error of opencover.console.exe
8da98bd 
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

Aditya Patwardhan (adityapatwardhan) commented Aug 3, 2017

Copy link
Copy Markdown
Member Author

Dongbo Wang (@daxian-dbw) Steve Lee (@SteveL-MSFT) please have another look, I have addressed the feedback and added another bug fix.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
SteveL-MSFT
Steve Lee (SteveL-MSFT) approved these changes Aug 4, 2017
View reviewed changes
@daxian-dbw
Dongbo Wang (daxian-dbw) merged commit 5cd0e85 into PowerShell:master Aug 4, 2017
@adityapatwardhan
Aditya Patwardhan (adityapatwardhan) deleted the OpenCoverCommonModulesPath branch August 4, 2017 17:08
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@adityapatwardhan @daxian-dbw
Fixed passing common test modules path to unelevated Powershell (Powe…
16a41ba 
…rShell#4313)

- 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.
- 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daxian-dbw Dongbo Wang (daxian-dbw) daxian-dbw left review comments
@SteveL-MSFT Steve Lee (SteveL-MSFT) SteveL-MSFT approved these changes
+1 more reviewer
@JamesWTruher James Truher (JamesWTruher) JamesWTruher approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@adityapatwardhan @daxian-dbw @JamesWTruher @SteveL-MSFT @msftclas
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.