Coverage script failing

[am11]

Redirected from: travis-ci/travis-ci#3347

Hi, we have a coverage script in node-sass for TravisCI. It was last updated on Nov 4 2014. It was reporting coverage to covealls till yesterday when I bumped mocha version sass/node-sass@c9ed930. Now we get permission denied on TravisCI: https://travis-ci.org/sass/node-sass/jobs/53495198#L1320 on running the coverage script (after passing all tests with mocha).

node-sass@2.0.1 coverage /home/travis/build/sass/node-sass
node scripts/coverage.js

/home/travis/build/sass/node-sass/node_modules/.bin/_mocha: 1: /home/travis/build/sass/node-sass/node_modules/.bin/_mocha: /bin: Permission denied

If I downgrade mocha to 2.1.0, the result is: https://travis-ci.org/am11/node-sass/jobs/53533514#L1310:

$ npm run-script coverage
node-sass@2.0.1 coverage /home/travis/build/am11/node-sass
node scripts/coverage.js

Done. Your build exited with 0

Is it related to #1567?

[danielstjules]

I think you might be right! Because the permissions are certainly correct:

$ ls -al . ../mocha/bin
.:
total 16
drwxr-xr-x  4 danielstjules  staff  136 Mar  8 21:14 .
drwxr-xr-x  4 danielstjules  staff  136 Mar  8 21:14 ..
lrwxr-xr-x  1 danielstjules  staff   19 Mar  8 21:14 _mocha -> ../mocha/bin/_mocha
lrwxr-xr-x  1 danielstjules  staff   18 Mar  8 21:14 mocha -> ../mocha/bin/mocha

../mocha/bin:
total 32
drwxr-xr-x   4 danielstjules  staff    136 Mar  8 21:14 .
drwxr-xr-x  12 danielstjules  staff    408 Mar  8 21:14 ..
-rwxr-xr-x   1 danielstjules  staff  11947 Mar  5 22:56 _mocha
-rwxr-xr-x   1 danielstjules  staff   1722 Feb 18 11:41 mocha

[bcoe]

I'm having a similar issue with coverage scripts running mocha-text-cov:

https://www.npmjs.com/package/mocha-text-cov

haven't dug in too much yet, but I bet coverage fails to output.

[danielstjules]

@bcoe @am11 Can you try upgrading to 2.2.1 and let us know if the issues persist?

[bcoe]

I'm continuing to see some issues on 2.2.1, I've created a tracking issue here;

seanmonstar/mocha-text-cov#1

I wouldn't be shocked if there's something legitimate that needs to be patched in mocha-text-cov, I'll help dig into things when I have some cycles this week.

[am11]

Same here: with mocha 2.2.1, I am still seeing the permission denied.

[danielstjules]

Looks like 1430c2b is the start of this permissions issue. It switches things up a bit for anyone looking to spawn mocha. E.g. not using #!/usr/bin/env node, which is a necessary change for the issue listed in that commit's description.

For @am11's failing build, I pushed a test PR at sass/node-sass#745. It reverts that commit, as seen in: https://github.com/danielstjules/mocha/commit/d84dd02d8fd20243de915dd510af14c5dd9549ff And the results are a passing build https://travis-ci.org/sass/node-sass/builds/53701623. :)

So hopefully we can just update the coverage scripts to fix this (and it's not an issue with travis at all). I'll take a look into this tonight (just on lunch).

[danielstjules]

@am11 @bcoe Please see sass/node-sass#746 for an example fix

[am11]

@danielstjules, it worked! But normally in node_modules/.bin/_mocha itself is equivalent to node node_modules/mocha/bin/_mocha, isn't it? I mean all the runner files in .bin directory are prepended by runtime executable. (less, sass, coffeescript, livescript etc.)

[danielstjules]

I think the idea was to handle the case where you're invoking a script or mocha with a different node bin than your default, improving flexibility. (#1548)

For example (just set this up, my environment isn't usually like this)

dstjules:~
$ node --version
v0.12.0

dstjules:~
$ iojs --version
v1.4.2

dstjules:~
$ cat example.js
#!/usr/bin/env node
console.log(process.execPath);

dstjules:~
$ iojs example.js
/usr/local/bin/iojs

dstjules:~
$ ./example.js
/usr/local/bin/node

dstjules:~
$ node example.js
/usr/local/bin/node

I suppose the change broke backwards compatibility for that part of the API, though I'm not sure what the best way to handle it is. We can move forward with this interface, but now we have a bin/_mocha file that isn't actually executable on its own since it's missing a #!/usr/bin/env node. @travisjeffery would probably have a better idea.

In any case, glad my PR worked for you. :)

[danielstjules]

@boneskull Got a feeling you're online after merging that PR :) Any thoughts on this?

[yamadapc]

I got problems capturing the global._$jscoverage in my mocha-spec-cov-alt reporter on mocha versions bigger than 2.1. I've tested the built-in json-cov reporter and it's also not capturing the variable.

[yamadapc]

Yeah... Definitely caused by

mocha/bin/mocha

Line 73 in 8a60787

var proc = spawn(process.execPath, args, { stdio: 'inherit' });

Reverting this to process.argv[0] fixes the problem. I'm sure there's a reason why it changed though.

[yamadapc]

Ops... Sorry, hadn't read all the comments above.

[yamadapc]

I'm pretty sure the problem caused here, in blanket:

https://github.com/alex-seville/blanket/blob/ad53a928d19d16f48bfa6731856f78b1cbacf579/src/index.js#L203-L215

When running with process.argv[0], the child process gets process.argv[0] === 'node', but (on my machine), running with process.execPath gives the child process.argv[0] === '/usr/local/bin/node'. Changing L203 to if(path.basename(process.argv[0]) === 'node' ... fixes the issue for me, but won't fix the problem for people using iojs. If someone submits a PR there, we'll be fixed.

[yamadapc]

Let's try to push this forward?

[danielstjules]

I should have time to tackle this tonight

[yamadapc]

I think I solved it in this PR to blanket [1], but there's a big list of open PRs there, so IDK.

[1] - alex-seville/blanket#479

[danielstjules]

@yamadapc I did the same for node-sass in sass/node-sass#746 However, since the current state of bin/_mocha will result in a string of PRs across projects that use it, it's probably best that we come up with a solution for mocha itself.

[boneskull]

@danielstjules I don't understand why 3p scripts are executing _mocha directly. Can you explain?

[boneskull]

(We can revert the change, but I would expect that to be considered "unspported behavior" which should be dropped with a major release.)

[danielstjules]

@boneskull I don't think it's worth reverting 1430c2b, if that's what you mean. It handled a valid issue, and it's a good solution. A small quirk was fixed in #1567, and #1616 reverts that bin file to once again being executable.

As for why _mocha is being used... that's a good question! I'm not sure that there is a reason. node-sass is setting an env var, and piping to coveralls in https://github.com/sass/node-sass/blob/d9a643ba7f666a4da083dc8b24f18710883883b6/scripts/coverage.js Custom env vars added in process by node-sass, for example, would still be passed to _mocha by mocha.

// -------------------------------------
// example.js
// -------------------------------------
it('test', function() {
  console.log(process.env.CUSTOM);
});

// -------------------------------------
// run.js, similar to coverage.js
// -------------------------------------
var path = require('path'),
    spawn = require('child_process').spawn;

process.env.CUSTOM = 'SET_BY_RUN';

var mocha = spawn('./bin/mocha', ['example.js'], {
  env: process.env
});

mocha.on('error', function(err) {
  console.error(err);
  process.exit(1);
});

mocha.stderr.setEncoding('utf8');
mocha.stderr.on('data', function(err) {
  console.error(err);
  process.exit(1);
});

mocha.stdout.pipe(process.stdout);

// -------------------------------------
// test
// -------------------------------------
$ node ./run.js

SET_BY_RUN

  ․

  1 passing (4ms)

This query reveals some projects relying on _mocha being executable: https://github.com/search?utf8=%E2%9C%93&q=spawn+%2B+%22_mocha%22+extension%3Ajs&type=Code&ref=searchresults (I'm not sure how to enforce the underscore in the query) And enforcing a js extension in the query allows us to ignore those who checked in node_modules, and hence bin/mocha. I didn't spend too too long going through the results, but noticed a couple dozen instances. A lot of them seem to be coverage related, so I wonder if there's some behavior I'm unaware of.

[bcoe]

@boneskull I agree that I don't think it's worth reverting, I've put a pull into Blanket (based on @yamadapc's hard work debugging) that solves the issues I've seen.

Having seen the cause of the bug, I think the onus is on Blanket to fix the issue (hopefully they start merging things soon) CC: @alex-seville.

[danielstjules]

@boneskull In the meantime, before we intentionally deprecate invoking _mocha directly, think #1616 is an acceptable change?

[danielstjules]

Closing via #1616. Anyone relying on _mocha will be able to continue to do so as of the next patch release, though it's probably discouraged. If there's reason in which mocha cannot be used, please let us know!