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: fix parallel/test-setproctitle.js on alpine #12413

Closed
wants to merge 1 commit into from

Conversation

DavidCai1111
Copy link
Member

@DavidCai1111 DavidCai1111 commented Apr 14, 2017

Fixes: #12399

As mentioned in the ref issue, since Busybox ps does not support -p switch, using ps -o and grep instead to get the process title and then check it.

The title variable in this test script was also renamed from 'testme' to 'test', because it seems that on alpine if trying to set the process title to a string whose length is >=5 like 'abcde', it will become {abcde} abcd in ps output:

> docker run -it node:alpine
> process.title = 'abcde'
'abcde'
> child_process.execSync('ps').toString().split('\n')
[ 'PID   USER     TIME   COMMAND',
  '    1 root       0:00 {abcde} abcd',
  '   20 root       0:00 /bin/sh -c ps',
  '   21 root       0:00 ps',
  '' ]
> docker run -it node:alpine
> process.title = 'abcd'
'abcd'
> child_process.execSync('ps').toString().split('\n')
[ 'PID   USER     TIME   COMMAND',
  '    1 root       0:00 abcd',
  '   20 root       0:00 /bin/sh -c ps',
  '   21 root       0:00 ps',
  '' ]
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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 14, 2017
@vsemozhetbyt vsemozhetbyt added the process Issues and PRs related to the process subsystem. label Apr 14, 2017
@vsemozhetbyt
Copy link
Contributor

@benjamingr
Copy link
Member

This breaks on FreeBSD apparently:

not ok 945 parallel/test-setproctitle
  ---
  duration_ms: 0.370
  severity: fail
  stack: |-
    assert.js:554
    assert.ifError = function ifError(err) { if (err) throw err; };
                                                      ^
    
    Error: Command failed: ps -o pid,args= | grep '^\s*4180 test'
    
        at ChildProcess.exithandler (child_process.js:247:12)
        at emitTwo (events.js:125:13)
        at ChildProcess.emit (events.js:213:7)
        at maybeClose (internal/child_process.js:882:16)
        at Socket.<anonymous> (internal/child_process.js:335:11)
        at emitOne (events.js:115:13)
        at Socket.emit (events.js:210:7)
        at Pipe._handle.close [as _onclose] (net.js:531:12)


Ping @nodejs/build regarding CI failures:

xcode-select: error: tool 'xcodebuild' requires Xcode, but active developer directory '/Library/Developer/CommandLineTools' is a command line tools instance

https://ci.nodejs.org/job/node-test-commit-osx/9018/nodes=osx1010/console

@lpinca
Copy link
Member

lpinca commented Apr 14, 2017

@benjamingr

xcode-select: error: tool 'xcodebuild' requires Xcode, but active developer directory '/Library/Developer/CommandLineTools' is a command line tools instance

This shouldn't be an issue.

@gibfahn
Copy link
Member

gibfahn commented Apr 14, 2017

@benjamingr the xcodebuild warnings aren't an issue (see nodejs/node-gyp#1057) xcodebuild is called twice but never actually used.

@jasnell
Copy link
Member

jasnell commented Apr 17, 2017

There are definite failures in CI with this that need looking at

@DavidCai1111 DavidCai1111 changed the title test: fix parallel/test-setproctitle.js on alpine [WIP]test: fix parallel/test-setproctitle.js on alpine Apr 18, 2017
@DavidCai1111 DavidCai1111 force-pushed the test/proc-title branch 11 times, most recently from bf0798a to 85a2592 Compare April 19, 2017 05:16
@DavidCai1111 DavidCai1111 changed the title [WIP]test: fix parallel/test-setproctitle.js on alpine test: fix parallel/test-setproctitle.js on alpine Apr 19, 2017
@DavidCai1111
Copy link
Member Author

DavidCai1111 commented Apr 19, 2017

Updated, and CI looks good now 😃

CI: https://ci.nodejs.org/job/node-test-pull-request/7510/

DavidCai1111 added a commit that referenced this pull request Apr 21, 2017
Since Busybox ps does not support -p switch, using ps -o and grep
instead to get the process title and then check it.

PR-URL: #12413
Fixes: #12399
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
@DavidCai1111
Copy link
Member Author

Landed in ee0620b

evanlucas pushed a commit that referenced this pull request Apr 25, 2017
Since Busybox ps does not support -p switch, using ps -o and grep
instead to get the process title and then check it.

PR-URL: #12413
Fixes: #12399
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
@evanlucas evanlucas mentioned this pull request May 1, 2017
evanlucas pushed a commit that referenced this pull request May 1, 2017
Since Busybox ps does not support -p switch, using ps -o and grep
instead to get the process title and then check it.

PR-URL: #12413
Fixes: #12399
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
Since Busybox ps does not support -p switch, using ps -o and grep
instead to get the process title and then check it.

PR-URL: #12413
Fixes: #12399
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
@gibfahn
Copy link
Member

gibfahn commented May 16, 2017

Doesn't backport cleanly for me to v6.x, would you be willing to backport? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

@DavidCai1111
Copy link
Member Author

@gibfahn Yeah...Backport PR: #13072

DavidCai1111 added a commit to DavidCai1111/node that referenced this pull request Jun 19, 2017
Since Busybox ps does not support -p switch, using ps -o and grep
instead to get the process title and then check it.

PR-URL: nodejs#12413
Fixes: nodejs#12399
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
MylesBorins pushed a commit that referenced this pull request Jul 10, 2017
Since Busybox ps does not support -p switch, using ps -o and grep
instead to get the process title and then check it.

Backport-PR-URL: #13072
PR-URL: #12413
Fixes: #12399
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
Since Busybox ps does not support -p switch, using ps -o and grep
instead to get the process title and then check it.

Backport-PR-URL: #13072
PR-URL: #12413
Fixes: #12399
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
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.

Test parallel/test-setproctitle.js fails on alpine
8 participants