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

Tests: Unbreak test:cli due to unknown 'qunit' command #1339

Merged
merged 1 commit into from
Dec 21, 2018
Merged

Conversation

Krinkle
Copy link
Member

@Krinkle Krinkle commented Dec 18, 2018

Follows-up 362e241, which removed 'npm link' from the 'test:cli'
script command.

Initially, I thought the reason the child processes can't find qunit is due to our use of execa.shell instead of regular execa. Specifically, execa.shell() spawns a process that starts with a new shell and no inherited environment variables. execa() on the other hand inherits environment variables like PATH by default, and it also has a preferLocal option that ensures node_modules/.bin is in thePATH even if you're running the tests without npm run .. (e.g. if you'd use bin/qunit test/cli/*.js directly).

Switching to execa() is non-trivial because it requires an array as input and there aren't built-in or trust-worthy modules that parse a command string into an array. Trying it with brute-force I realised the problem is actually that qunit isn't visible at all even in the original environment created by npm-run.

While npm-run does add node_modules/.bin, and bins from (dev)dependencies are indeed linked from there, the bins from the current package itself (qunit) are never linked in .bin.

So... keeping it simple and just string-replacing 'qunit' with 'bin/qunit', which also matches the way we invoke the cli from other commands in package.json already.

@Krinkle Krinkle requested a review from trentmwillis December 18, 2018 05:01
Copy link
Member

@trentmwillis trentmwillis left a comment

Choose a reason for hiding this comment

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

Seems good to me. Looks like you'll need to make the same changes in

qunit/test/cli/watch.js

Lines 12 to 21 in 62374ea

function execute( command ) {
const cwd = process.cwd();
process.chdir( path.join( __dirname, "fixtures" ) );
const execution = exec( command, { stdio: [ null, null, null, "ipc" ] } );
process.chdir( cwd );
return execution;
}
though. The function is roughly duplicated due to exec needing some different arguments to pipe output a specific way.

Follows-up 362e241, which removed 'npm link' from the 'test:cli'
script command.

Initially, I thought the reason the child proceses can't find qunit
is due to our use of execa.shell instead of regular execa. Specifically,
execa.shell() spawns a process that starts with a new shell and no
inherited environment variables. execa() on the other hand inherits
environment variables like PATH by default, and it also has a
'preferLocal' option that ensures node_modules/.bin is in the PATH
even if you're running the tests without 'npm run ..' (e.g. if you'd
use 'bin/qunit test/cli/*.js' directly).

Switching to execa() is non-trivial because it requires an array
as input and there aren't built-in or trust-worthy modules that
parse a command string into an array. Trying it with brute-force
I realised the problem is actually that 'qunit' isn't visible
at all even in the original environment created by npm-run.

While npm-run does add node_modules/.bin, and bins from (dev)dependencies
are indeed linked from there, the bins from the current package itself
(qunit) are never linked in .bin.

So... keeping it simple and just string-replacing 'qunit' with
'bin/qunit', which also matches the way we invoke the cli from
other commands in package.json already.
Copy link
Member

@trentmwillis trentmwillis left a comment

Choose a reason for hiding this comment

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

Thanks!

@trentmwillis trentmwillis merged commit b385b83 into master Dec 21, 2018
@trentmwillis trentmwillis deleted the bin-bin branch December 21, 2018 04:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants