Skip to content

Conversation

@DavidCai1111
Copy link
Member

@DavidCai1111DavidCai1111 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-botnodejs-github-bot added the test Issues and PRs related to the tests. label Apr 14, 2017
@vsemozhetbytvsemozhetbyt 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

@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

@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

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

@DavidCai1111DavidCai1111 changed the title test: fix parallel/test-setproctitle.js on alpine[WIP]test: fix parallel/test-setproctitle.js on alpineApr 18, 2017
@DavidCai1111DavidCai1111force-pushed the test/proc-title branch 11 times, most recently from bf0798a to 85a2592CompareApril 19, 2017 05:16
@DavidCai1111DavidCai1111 changed the title [WIP]test: fix parallel/test-setproctitle.js on alpinetest: fix parallel/test-setproctitle.js on alpineApr 19, 2017
@DavidCai1111
Copy link
MemberAuthor

DavidCai1111 commented Apr 19, 2017

Updated, and CI looks good now 😃

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

DavidCai1111 pushed 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: #12413Fixes: #12399 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]>
@DavidCai1111
Copy link
MemberAuthor

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: #12413Fixes: #12399 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]>
@evanlucasevanlucas 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: #12413Fixes: #12399 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]>
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: #12413Fixes: #12399 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]>
@gibfahn
Copy link
Member

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
MemberAuthor

@gibfahn Yeah...Backport PR: #13072

DavidCai1111 pushed 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#12413Fixes: nodejs#12399 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]>
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: #12413Fixes: #12399 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]>
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: #12413Fixes: #12399 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Jul 18, 2017
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.

Test parallel/test-setproctitle.js fails on alpine

9 participants

@DavidCai1111@vsemozhetbyt@benjamingr@lpinca@gibfahn@jasnell@hiroppy@nodejs-github-bot@davidtaikocha