Fixes #2534 by replacing expensive WMI query with Win32 API calls - #2535
Conversation
|
Hi Steve Lee (@SteveL-MSFT), I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
TTYL, MSBOT; |
| cimProcess => | ||
| Convert.ToInt32(cimProcess.CimInstanceProperties["ParentProcessId"].Value, | ||
| CultureInfo.CurrentCulture)).FirstOrDefault(); | ||
| int parentPid = 0; |
There was a problem hiding this comment.
Do we have any Pester tests that cover this code change? If not, please create a new test or port some internal tests to the new environment.
There was a problem hiding this comment.
Working on how to test this indirectly
There was a problem hiding this comment.
Test added
| @@ -0,0 +1,24 @@ | |||
| Describe "Native Command Processor" -tags "CI" { | |||
| # Find where test/powershell is so we can find the createchildprocess command relative to it | |||
| $powershellTestDir = $PSScriptRoot | |||
There was a problem hiding this comment.
This should be put in a BeforeAll. Test code should not exist outside a defined scope for Pester.
| { | ||
| Push-Location $tool | ||
| try { | ||
| dotnet publish --output bin |
There was a problem hiding this comment.
This does not seem like a good solution to your test issue. Is there a better way to handle it? Packaging scoops up all files in the publish directory, so these tools will get shipped along with PowerShell Core during release.
There was a problem hiding this comment.
I was just following an existing precedent used by echoargs.exe tests. However, I checked my install and it doesn't include the echoargs.exe test binary. Are you sure it gets picked up?
There was a problem hiding this comment.
https://docs.microsoft.com/en-us/dotnet/articles/core/tools/dotnet-publish
It looks like it works because it specifies a different output directory
| $async = $ps.BeginInvoke() | ||
| $ps.Stop() | Out-Null | ||
|
|
||
| Get-Process CreateChildProcess -ErrorAction SilentlyContinue | Should BeNullOrEmpty |
There was a problem hiding this comment.
Is the inconsistency with capitalization intentional?
There was a problem hiding this comment.
Not intentional, VSCode did the Pascal casing on it, will make it consistently lowercase
| for (uint i = 0; i < num; i++) | ||
| { | ||
| Process child = new Process(); | ||
| child.StartInfo.FileName = Process.GetCurrentProcess().MainModule.FileName; |
There was a problem hiding this comment.
What about implementing this within a PowerShell function? It would simplify the publishing and execution of it.
There was a problem hiding this comment.
Need this as a native executable to execute a particular code path in powershell for handing native commands
| child.Start(); | ||
| } | ||
| } | ||
| Thread.Sleep(100000); |
There was a problem hiding this comment.
Why the 100 second sleep?
There was a problem hiding this comment.
I thought it was better to have it "timeout" rather than hang if the process wasn't successfully terminated. Maybe I should just leave it to the CI system to kill if it takes too long, but may be better to have this test fail and allow other tests to run within the CI time limit.
added check that test processes are created before we try to kill them
|
Steve Lee (@SteveL-MSFT) GitHub cannot recognize the author of the commits. Can you please fix it? |
|
Looks like the complaint about slee@ntdev.microsoft.com is because I pushed from my Windows box which I rebuilt and didn't explicitly define user.email so it picked it up incorrectly with the domain. I've since fixed this. |
… calls (PowerShell#2535) * Fixes PowerShell#2534 by replacing expensive WMI query with Win32 API calls * fix break on unix build * added tests for PowerShell#2535 * although test passed, fixing exception that shows up * fixed Describe text * addressing code review feedback * addressing review feedback to comment on why sleep is needed added check that test processes are created before we try to kill them * fixed test to timeout and pending fix for PowerShell#2561
Address #2534