Repository navigation
test: improve parallel/test-setproctitle.js #14039
Description
Activity
- addedgood first issueIssues that are suitable for first-time contributors.Issues that are suitable for first-time contributors.processIssues and PRs related to the process subsystem.Issues and PRs related to the process subsystem.testIssues and PRs related to Node.js core tests and test infrastructure.Issues and PRs related to Node.js core tests and test infrastructure.windowsIssues and PRs related to the Windows platform.Issues and PRs related to the Windows platform.
on Jul 2, 2017 I looked at this briefly when the skip was added (#11416 (review)) and it looked pretty ugly to do in PowerShell.
More ugly than:
const cmd = common.isLinux ? `ps -o pid,args | grep '${process.pid} ${title}' | grep -v grep` : `ps -p ${process.pid} -o args=`;
😜
P.S. https://blogs.msdn.microsoft.com/powershell/2009/05/22/get-windowtitle-ps1/
PS C:\WINDOWS\system32> Get-Process |where {$_.mainWindowTItle} |format-table id,name,mainwindowtitle –AutoSize Id Name MainWindowTitle -- ---- --------------- 12184 ApplicationFrameHost Calculator 8392 Calculator Calculator 16228 chrome Get-WindowTitle.ps1 | PowerShell Team Blog - Google Chrome 11276 ConEmu64 cmd (Admin) 13504 DontSleep_x64 DontSleep 13420 explorer Downloads 19240 explorer D:\code\0tni 21440 mmc Event Viewer 14860 powershell Administrator: PowerShell 20176 pycharm64 node - [D:\code\node] - ...\test\parallel\test-setproctitle.js - PyCharm 2017.2 EAP 4616 ShellExperienceHost Windows Shell Experience Host 21464 SystemSettings Settings 4632 ThrottleStop ThrottleStop 6.00 11528 vivaldi player.gl.bynetcdn.com/Players/ByPlayer/EmbedPlayer/GLZ?ClipID=glglz&type=live
😲 At the time #11416 landed that was a one liner:
exec(`ps -p ${process.pid} -o args=`, function callback(error, stdout, stderr) {
And yes, still uglier than the current. Quoting myself from the review:
Turns out it's more complicated because on Windows changing the process title changes the title of the console Window which is not necessarily the same process as the node.exe process (e.g. if you open a cmd.exe Window and then launch node.exe inside it, changing the process title in Node.js changes the Window title of the cmd.exe process).
Turns out it's more complicated because on Windows changing the process title changes the title of the console Window which is not necessarily the same process as the node.exe process (e.g. if you open a cmd.exe Window and then launch node.exe inside it, changing the process title in Node.js changes the Window title of the cmd.exe process).
True 😞
Btw I am 👍 for implementing the currently skipped part of the test on Windows -- just wanted to indicate that it's non-trivial.
I've found a better than nothing one liner:
const powerShell = 'powershell -ExecutionPolicy Unrestricted -Command'; return `${powerShell} "ps | ? {$_.mainWindowTItle -eq '${title}'}"`;
It's a bit looser (will find any window with the title, so might have false positives)
Might as well copy the rest of my original review:
Anyway I ended up with this hideous command line/PowerShell script, which given a process id will walk up the parent process ids until it finds a process with a non-empty Window title and then print it out:
powershell "$p = <pid>; $t = Get-Process -pid $p ; while ($t.MainWindowTitle -eq '') { $p = Get-WmiObject win32_process | where {$_.processId -eq $p } | select -ExpandProperty parentProcessId; $t = Get-Process -pid $p } Write-Host $t.MainWindowTitle"Of course
Get-Processdoesn't know about parent process ids, so the script has to useGet-WmiObject win32_process(which doesn't know about Window titles).I make no claims to being a PowerShell expert.
I think that will avoid false positives, but IMHO the script is difficult to read/follow (and I was the one who hacked it together 😆 ).
Yep so on Windows it's the
windowthat gets the title, so I'm spinning up a newwindowwithcmd /c start "not the mama" /MIN node...PR to come soonReacted by Aditya Anand M C- removedgood first issueIssues that are suitable for first-time contributors.Issues that are suitable for first-time contributors.
on Jul 6, 2017 Closing as #14042 was closed and there's been no further activity in almost a year. Feel absolutely free to re-open and/or comment if you think this should be open.
masterWindowsCurrent implementation skips a part of the test claiming
'Windows does not have "ps" utility'which is not strickly true — Windows hastasklistand PowerShell haspsaliasingGet-Proccess.The rest of the test should be implemented for Windows.