Skip to content

test: improve parallel/test-setproctitle.js #14039

Description

@refack
  • Version: master
  • Platform: Windows
  • Subsystem: test,process

Current implementation skips a part of the test claiming 'Windows does not have "ps" utility' which is not strickly true — Windows has tasklist and PowerShell has ps aliasing Get-Proccess.

The rest of the test should be implemented for Windows.

Activity

  1. added
    good first issueIssues that are suitable for first-time contributors.
    processIssues and PRs related to the process subsystem.
    testIssues and PRs related to Node.js core tests and test infrastructure.
    windowsIssues and PRs related to the Windows platform.
    on Jul 2, 2017
  2. richardlau commented on Jul 2, 2017

    @richardlau
    Member

    I looked at this briefly when the skip was added (#11416 (review)) and it looked pretty ugly to do in PowerShell.

  3. refack commented on Jul 2, 2017

    @refack
    ContributorAuthor

    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
  4. richardlau commented on Jul 2, 2017

    @richardlau
    Member

    😲 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).

  5. refack commented on Jul 2, 2017

    @refack
    ContributorAuthor

    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 😞

  6. richardlau commented on Jul 2, 2017

    @richardlau
    Member

    Btw I am 👍 for implementing the currently skipped part of the test on Windows -- just wanted to indicate that it's non-trivial.

  7. refack commented on Jul 2, 2017

    @refack
    ContributorAuthor

    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)

  8. richardlau commented on Jul 2, 2017

    @richardlau
    Member

    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-Process doesn't know about parent process ids, so the script has to use Get-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 😆 ).

  9. refack commented on Jul 2, 2017

    @refack
    ContributorAuthor

    Yep so on Windows it's the window that gets the title, so I'm spinning up a new window with cmd /c start "not the mama" /MIN node... PR to come soon

  10. removed
    good first issueIssues that are suitable for first-time contributors.
    on Jul 6, 2017
  11. Trott commented on Jun 7, 2018

    @Trott
    Member

    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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    processIssues and PRs related to the process subsystem.testIssues and PRs related to Node.js core tests and test infrastructure.windowsIssues and PRs related to the Windows platform.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions