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

Fix mocha.cmd binary lookup #8

Merged
merged 3 commits into from
Sep 17, 2014
Merged

Fix mocha.cmd binary lookup #8

merged 3 commits into from
Sep 17, 2014

Conversation

blah238
Copy link
Contributor

@blah238 blah238 commented Sep 16, 2014

This line of code was looking for mocha.cmd in Mocha's own bin folder.

This is incorrect; there is no mocha.cmd file there. The mocha.cmd file is in the node_modules/.bin folder.

Tested on Windows. Not sure about *nix.

There is probably a better way to get to the node_modules/.bin folder than this, but it "works for me (tm)".

This line of code was looking for `mocha.cmd` in Mocha's own `bin` folder.

This is incorrect; there is no `mocha.cmd` file there. The `mocha.cmd` file is in the `node_modules/.bin` folder.

Tested on Windows. Not sure about *nix.

There is probably a better way to get to the `node_modules/.bin` folder than this, but it "works for me (tm)".
@knpwrs
Copy link
Owner

knpwrs commented Sep 17, 2014

Provided that gulp automatically switches the cwd to the directory of the gulpfile I'll go ahead and merge this. Linking to current CLI documentation for reference: https://github.com/gulpjs/gulp/blob/54197db5fde8138f8e850f9c8ab35f5de0aa0f61/docs/CLI.md

knpwrs added a commit that referenced this pull request Sep 17, 2014
Fix mocha.cmd binary lookup
@knpwrs knpwrs merged commit f83347d into knpwrs:master Sep 17, 2014
@knpwrs
Copy link
Owner

knpwrs commented Sep 17, 2014

Published to npm as 0.1.7-beta.4.

@knpwrs
Copy link
Owner

knpwrs commented Sep 17, 2014

So, on the other hand, there is child_process::fork.

We could do the equivalent of the following:

var fork = require('child_process').fork,
    path = require('path');
var args = [/*...*/];
fork(path.join(require.resolve('mocha'), 'bin', '_mocha'), args);

bin/_mocha is a node module which means we could fork it off. It's typically used for code coverage because then libraries like istanbul can inject instrumentation code with no effort required on the developers' part. This should support multiple platforms and make the planned coverage support in #10 a lot easier.

Thoughts?

@blah238
Copy link
Contributor Author

blah238 commented Sep 17, 2014

@KenPowers Worth a try, IMO, so long as all the CLI arguments are supported (the problem with Mocha's programmatic interface is that many of the CLI args aren't supported).

But then you might have to rename this module gulp-fork-mocha 😉

@knpwrs
Copy link
Owner

knpwrs commented Sep 18, 2014

I actually thought of that myself, but then I read

This is a special case of the spawn() functionality for spawning Node processes.

So I think we're good. :P

@knpwrs
Copy link
Owner

knpwrs commented Sep 18, 2014

Also, it seems like it works as with all flags (such as --debug-brk) as long as fork is called on mocha and not on _mocha. In order for code coverage with istanbul to work we'll need to call _mocha which means arguments like --debug-brk won't work when doing coverage.

@knpwrs
Copy link
Owner

knpwrs commented Sep 18, 2014

fork appears to work on a Windows test VM I just set up:

gulp-spawn-mocha on Windows

@knpwrs
Copy link
Owner

knpwrs commented Sep 18, 2014

Published 0.3.0 to npm with fork.

@scott2449
Copy link

I will give it a shot tomorrow, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants