Repository navigation
test: isolate configure-nodedir from npm_config_nodedir - #3389
Open
gengjiawen wants to merge 1 commit into
Open
gengjiawen wants to merge 1 commit into
gengjiawen wants to merge 1 commit into
Conversation
parseArgv lets npm_config_* environment variables override command line options, so when the test harness exports npm_config_nodedir (as the Jenkins citgm-smoker job does) the --nodedir=/usr passed by the test is replaced and the spawn stub's assert.fail() throws. Because configure() is async and the test passed assert.fail as an ignored third argument, that rejection was never handled and the test timed out after 30s instead of reporting the assertion. Clear the nodedir environment variables for the suite and route configure() rejections to done(). Refs: nodejs/citgm#1202
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist
npm install && npm run lint && npm testpassesDescription of change
Fixes the remaining
node-gypfailure in Jenkins citgm-smoker reported in nodejs/citgm#1202, whereconfigure-nodedir › configure nodedir with node-gyp command linetimes out after 30s.The Jenkins citgm-smoker job runs
export npm_config_nodedir="$WORKSPACE/node"and passes--nodedirto citgm, and citgm putsnpm_config_nodedirin the module's test environment.parseArgvletsnpm_config_*override command line options, so the test's--nodedir=/usris replaced. The generatedcommon.gypipath then doesn't match, and the spawn stub callsassert.fail().That assertion never shows up because the test calls
configure(prog, [], assert.fail).configure()is async, so the third argument is ignored, the rejection goes unhandled,doneis never called, and mocha reports a timeout. A plaincitgm node-gyprun (e.g. in Docker) doesn't set nodedir, which is why it passes there.Repro on
main:This PR only changes the test:
npm_config_nodedirandnpm_package_config_node_gyp_nodedirfor the suite (they are restored afterwards).catch(done)at all fourconfigure()call sites, so any future mismatch fails right away with the real assertion instead of a timeoutThe env-over-CLI precedence for
nodediritself is unchanged.