Skip to content

Conversation

@hhellyer
Copy link
Contributor

Setting the process title has been enabled in libuv on AIX and z/OS.
The latest level of libuv skips only skips testing of uv_set_process_title when __sun is #defined.
The Node.js test for supported platforms was getting long so now it only checks for the one unsupported platform the same way as libuv.

This change simplifies the skip test so the test is only skipped when common.isSunOS is true to match libuv. The corresponding libuv test is here:
https://github.com/nodejs/node/blob/master/deps/uv/test/test-process-title.c#L63

It was updated when support for setting the process title was added to AIX and z/OS and included in Node.js when libuv was upgraded under PR #11094

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Setting the process title has been enabled in libuv on AIX and z/OS. The latest level of libuv skips only skips testing of uv_set_process_title when __sun is #defined. This change simplifies the skip test so the test is only skipped when common.isSunOS is true to match libuv.
@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Feb 16, 2017
Copy link
Contributor

@cjihrigcjihrig left a comment

Choose a reason for hiding this comment

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

LGTM if the CI is happy

@mscdexmscdex added the process Issues and PRs related to the process subsystem. label Feb 16, 2017
@mscdex
Copy link
Contributor

@bnoordhuis
Copy link
Member

Test failures:

not ok 232 parallel/test-setproctitle --- duration_ms: 0.408 severity: fail stack: |- assert.js:382 assert.ifError = function ifError(err){if (err) throw err}; ^ Error: Command failed: ps -p 6340 -o args= ps: unknown option -- o Try `ps --help' for more information. 

@cjihrig
Copy link
Contributor

@hhellyer looks like a Windows check is needed as well.

Skip running the `ps` part of the test on Windows.
@hhellyer
Copy link
ContributorAuthor

@cjihrig - Done, I added a Windows check so it doesn't run ps, it does run the first half of the test. Sorry for the delay, I had to find a Windows machine.

I left the first trivial looking half of the test as on Windows the libuv stores the title by changing the console title and retrieves it by calling GetConsoleTitleW so the test will do a bit more than just check a variable contains the value it was set to one line above.

process.title=title;
assert.strictEqual(process.title,title);

// Test setting the title but do not try to run `ps` on Windows.
Copy link
Member

@richardlaurichardlauFeb 23, 2017

Choose a reason for hiding this comment

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

So I naively thought that it should be possible to do the equivalent (i.e. validate the process title change) on Windows via PowerShell. 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).

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. Quite happy to submit this as a separate PR and not overly complicate this one.
cc @nodejs/platform-windows

@gibfahn
Copy link
Member

gibfahn commented Feb 24, 2017

I'll land this tomorrow if there are no objections EDIT: or CI failures

@gibfahn
Copy link
Member

gibfahn commented Feb 24, 2017

CI 2: https://ci.nodejs.org/job/node-test-commit/8111/

EDIT: CI is green

Copy link
Member

@mhdawsonmhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@gibfahngibfahn self-assigned this Feb 24, 2017
@gibfahn
Copy link
Member

Landed in 893632e

@gibfahngibfahn closed this Feb 27, 2017
gibfahn pushed a commit that referenced this pull request Feb 27, 2017
Setting the process title has been enabled in libuv on AIX and z/OS. The latest level of libuv skips only skips testing of uv_set_process_title when __sun is #defined. This change simplifies the skip test so the test is only skipped when common.isSunOS is true to match libuv. Skip running the `ps` part of the test on Windows. PR-URL: #11416 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 28, 2017
Setting the process title has been enabled in libuv on AIX and z/OS. The latest level of libuv skips only skips testing of uv_set_process_title when __sun is #defined. This change simplifies the skip test so the test is only skipped when common.isSunOS is true to match libuv. Skip running the `ps` part of the test on Windows. PR-URL: nodejs#11416 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
@italoacasasitaloacasas mentioned this pull request Feb 28, 2017
@jasnell
Copy link
Member

If this should land on v6 or v4 at all, please open a backport PR

@gibfahn
Copy link
Member

This depends on #11094, so it should only be backported if that PR is.

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

Labels

processIssues and PRs related to the process subsystem.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@hhellyer@mscdex@bnoordhuis@cjihrig@gibfahn@jasnell@richardlau@mhdawson@nodejs-github-bot