Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

Update process spawning in coverage.js for mocha 2.2.x #746

Merged
merged 1 commit into from
Mar 9, 2015
Merged

Update process spawning in coverage.js for mocha 2.2.x #746

merged 1 commit into from
Mar 9, 2015

Conversation

danielstjules
Copy link
Contributor

Bumps mocha to 2.2.1, and fixes the error highlighted in mochajs/mocha#1585:

0.80s$ npm run-script coverage
> [email protected] 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
npm ERR! [email protected] coverage: `node scripts/coverage.js`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the [email protected] coverage script.
npm ERR! This is most likely a problem with the node-sass package,
npm ERR! not with npm itself.
npm ERR! Tell the author that this fails on your system:
npm ERR!     node scripts/coverage.js
npm ERR! You can get their info via:
npm ERR!     npm owner ls node-sass
npm ERR! There is likely additional logging output above.
npm ERR! System Linux 2.6.32-042stab090.5
npm ERR! command "/home/travis/.nvm/v0.10.36/bin/node" "/home/travis/.nvm/v0.10.36/bin/npm" "run-script" "coverage"
npm ERR! cwd /home/travis/build/sass/node-sass
npm ERR! node -v v0.10.36
npm ERR! npm -v 1.4.28
npm ERR! code ELIFECYCLE
npm ERR! 
npm ERR! Additional logging details can be found in:
npm ERR!     /home/travis/build/sass/node-sass/npm-debug.log
npm ERR! not ok code 0

am11 added a commit that referenced this pull request Mar 9, 2015
Update process spawning in coverage.js for mocha 2.2.x
@am11 am11 merged commit ff7f9f9 into sass:master Mar 9, 2015
@am11
Copy link
Contributor

am11 commented Mar 9, 2015

Thanks @danielstjules! 👍 🎉

@danielstjules danielstjules deleted the mocha-coverage branch March 11, 2015 05:40
@am11
Copy link
Contributor

am11 commented Mar 14, 2015

@danielstjules, @dougwilson,
9 of our CLI tests are failing on AppVeyor with timeout of 2000ms exceeded:
https://ci.appveyor.com/project/sass/node-sass/build/715/job/fdom3ikwcnejwm43#L952. No doubt v2.2.1 fixed a lot of other issues we were having on AppVeyor (as reported here: mochajs/mocha#333 (comment)). But now all 676+ tests pass locally and those nine fail on AppVeyor.
Any ideas how to investigate this issue further?

@danielstjules
Copy link
Contributor Author

Have you tried bumping up the timeouts for those tests? Also, it might be worth setting up a branch that watches stdout and stderr for those child processes. See if the child process is doing anything when running on appveyor.

@dougwilson
Copy link

I'll take a closer look soon, but a timeout bump may help, since it sometimes seems like AppVeyor does execute a bit slow, so since the timeouts are based on wall clock time and not CPU time, it can be easy to trip them on a slow environment.

Another possible thing to try locally is just to run your tests piped to more and see if that causes them to fail. I do this a lot because it sometimes brings up issues I see on AppVeyor and not on my local. Ex npm test | more.

@am11
Copy link
Contributor

am11 commented Mar 16, 2015

Thanks guys. I tested with this diff:

diff --git a/test/cli.js b/test/cli.js
index 78edace..f9c1d3c 100644
--- a/test/cli.js
+++ b/test/cli.js
@@ -7,6 +7,8 @@ var assert = require('assert'),
     fixture = path.join.bind(null, __dirname, 'fixtures');

 describe('cli', function() {
+  this.timeout(0);
+
   describe('node-sass < in.scss', function() {
     it('should read data from stdin', function(done) {
       var src = fs.createReadStream(fixture('simple/index.scss'));

(disabling suite-specific timeout)

and the result is a never-ending failure: https://ci.appveyor.com/project/am11/node-sass/build/job/36c5pdnjvexmjd4n.

@dougwilson
Copy link

I don't see you listening to the close or exit event from the spawn in this test:

node-sass/test/cli.js

Lines 135 to 152 in c97411e

it('should emit `warn` on file change when using --watch option', function(done) {
var src = fixture('simple/tmp.scss');
fs.writeFileSync(src, '');
var bin = spawn(cli, ['--watch', src]);
bin.stderr.setEncoding('utf8');
bin.stderr.once('data', function(data) {
assert(data.trim() === '=> changed: ' + src);
fs.unlinkSync(src);
done();
});
setTimeout(function() {
fs.appendFileSync(src, 'body {}');
}, 500);
});

@am11
Copy link
Contributor

am11 commented Mar 16, 2015

Is it necessary? We are using .once('data' though. Could it be that it exits the child-process before execution?

@dougwilson
Copy link

Right, but that's the test it's stuck on. Clearly if the child process never sends data (perhaps because of a test failure), then that test will hang forever, which is what's currently happening.

@am11
Copy link
Contributor

am11 commented Mar 16, 2015

@dougwilson, tried both close and exit, it is still stuck at same place.
Commits: am11@e69d50d and am11@bdd98f8.

@dougwilson
Copy link

You need to listen on thie child process, not the stream. i.e. bin.once('exit' or bin.once('close'. More information can be found in the Node.js documentation: https://nodejs.org/api/child_process.html#child_process_class_childprocess

@am11
Copy link
Contributor

am11 commented Mar 17, 2015

Sorry I missed that and thanks for the link! 8-)
Unfortunately, it seems like it does not fire those events either: https://ci.appveyor.com/project/am11/node-sass/build/job/as3t5so2i09tab1e, corresponding commit: am11@2069bcf.

@dougwilson
Copy link

Ah, gotcha. I just realized you're calling ./bin/node-sass --watch. I suppose that process would never exit, now would it :/? I mean, if it's hanging there, it would seem that the node-sass bin is just never detecting the file change. What happens if you change https://github.com/am11/node-sass/blob/2069bcf9b0af68c08608cfeea3f75167cf335abe/test/cli.js#L162 to like 5000? Maybe the file is being touched before the child process is actually ready to start watching?

@saper
Copy link
Member

saper commented Apr 28, 2015

@dougwilson good catch, I have added necessary kills in #888.

jiongle1 pushed a commit to scantist-ossops-m2/node-sass that referenced this pull request Apr 7, 2024
Correct output for unary operation on strings
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants