Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: Run test-setproctitle where setting the process title is supported by libuv #11416

Closed
wants to merge 2 commits into from

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-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 16, 2017
Copy link
Contributor

@cjihrig cjihrig 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

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

mscdex commented Feb 16, 2017

@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

cjihrig commented Feb 16, 2017

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

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

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

@@ -21,6 +21,10 @@ assert.notStrictEqual(process.title, title);
process.title = title;
assert.strictEqual(process.title, title);

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

@richardlau richardlau Feb 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

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

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

gibfahn commented Feb 27, 2017

Landed in 893632e

@gibfahn gibfahn 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 <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
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 <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@italoacasas italoacasas mentioned this pull request Feb 28, 2017
@jasnell
Copy link
Member

jasnell commented Mar 7, 2017

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

@gibfahn
Copy link
Member

gibfahn commented Mar 7, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
process Issues and PRs related to the process subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants