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

Issues with grunt build, grunt test. #356

Closed
EtaiG opened this issue Oct 17, 2014 · 21 comments
Closed

Issues with grunt build, grunt test. #356

EtaiG opened this issue Oct 17, 2014 · 21 comments

Comments

@EtaiG
Copy link
Contributor

EtaiG commented Oct 17, 2014

I just forked the repo and did npm install.
I have two issues:

  1. grunt build fails with the following error:
Running "build" task
Error: ./src/timers.js:1:14 Non-ASCII character:
 (U+000D)
    at c:\Projects\bluebird\Gruntfile.js:20:23
    at Array.forEach (native)
    at checkAscii (c:\Projects\bluebird\Gruntfile.js:13:30)
    at c:\Projects\bluebird\Gruntfile.js:576:33
From previous event:
    at c:\Projects\bluebird\Gruntfile.js:575:30
    at Array.map (native)
    at build (c:\Projects\bluebird\Gruntfile.js:573:29)
    at Object.<anonymous> (c:\Projects\bluebird\Gruntfile.js:713:9)
    at Object.thisTask.fn (c:\Projects\bluebird\node_modules\grunt\lib\grunt\tas

However this is a clean fork. I removed all the carriage returns (the offending char) and it still happens, so I'm at a loss. I'm on windows 7.

  1. If I comment out the checkAscii function in the Gruntfile, so that the build passes, grunt test fails for me too:
Running "testrun" task
Running test mocha/2.3.3.js
Running test mocha/bind.js
Running test mocha/unhandled_rejections.js
Running test mocha/2.1.2.js
Running test mocha/2.1.3.js
Running test mocha/2.2.1.js
Running test mocha/2.2.2.js
Running test mocha/2.2.3.js
Running test mocha/2.2.4.js
Running test mocha/2.2.5.js
Running test mocha/2.2.6.js
Running test mocha/2.2.7.js
Running test mocha/2.3.1.js
Test mocha/2.2.5.js succeeded
Running test mocha/2.3.2.js
Test mocha/2.2.1.js succeeded
Running test mocha/2.3.4.js
Test mocha/2.1.2.js succeeded
Running test mocha/3.2.1.js
Test mocha/2.1.3.js succeeded
Running test mocha/3.2.2.js
Test mocha/2.2.4.js succeeded
Running test mocha/3.2.3.js
Test mocha/2.2.2.js succeeded
Running test mocha/3.2.4.js
Test mocha/2.3.1.js succeeded
Running test mocha/3.2.5.js
Test mocha/2.2.3.js succeeded
Running test mocha/3.2.6.js
Test mocha/2.2.6.js succeeded
Running test mocha/api_exceptions.js
Test mocha/3.2.1.js succeeded
Running test mocha/async.js
Test mocha/2.3.2.js succeeded
Running test mocha/bluebird-multiple-instances.js
Immediately rejected with non instanceof Error
Error: timeout of 500ms exceeded
    at null.<anonymous> (c:\Projects\bluebird\node_modules\mocha\lib\runnable.js
:139:19)
    at Timer.listOnTimeout [as ontimeout] (timers.js:112:15)
Test mocha/3.2.4.js succeeded
Running test mocha/call.js
Fatal error: spawn OK

Can you help me get the local build & tests working?

@dmethvin
Copy link

It's the end-of-line conversion in Windows. I fixed it with a .gitattributes file in the root:

# Files must always use LF for tools to work on Windows
*.js eol=lf
*.md eol=lf
*.json eol=lf

After a git reset --hard the files are there without newlines carriage returns and the build/test works fine (at least under msysgit and Windows 8).

@EtaiG
Copy link
Contributor Author

EtaiG commented Oct 18, 2014

after a git reset --hard I lose the .gitattributes, and if I add it to git, then it still doesn't work.
However, you pointed me in the right direction, and in webstorm I chose the line endings to be \n (UNIX) instead of \n\r and it worked (for the build).

The same test still fails though, and now all my files are marked as changed due to the lf / crlf change

@domenic
Copy link

domenic commented Oct 18, 2014

In general when working on cross-platform projects you should set eol to lf and autocrlf to input:

$ git config --global core.eol lf
$ git config --global core.autocrlf input

and of course you should make sure you IDE is not messing up the line endings. I'd suggest making those config changes then doing a clean checkout.

@EtaiG
Copy link
Contributor Author

EtaiG commented Oct 20, 2014

@domenic, thanks. That solved my lf/crlf issue correctly and is great advice in general.

I still have problems with the tests. Do you have any suggestions?

@petkaantonov
Copy link
Owner

Can you pull latest master and try again? The test runner now outputs which file failed

@EtaiG
Copy link
Contributor Author

EtaiG commented Dec 19, 2014

Thanks for not forgetting about the issue.

I pulled, rebuilt and ran tests.

It does show the filename it's running which is where the error occurs, but it's not always the same file. There's still a timeout error.

So I ran it with --verbose and saw some errors - my first intuition was to xdescribe a few tests to see if I can pinpoint it:

In unhandled_rejections.js:
    xdescribe("Will report rejections that are not instanceof Error", 

In when_map.js - xdescribed both
In cancel.js - xdescribe("issues"   // line 222. This is because gh-166 causes a failure.
In bind.js - xdescribe("with timeout",   // line 111: 
In when_reduce.js - xdescribed

when_reduce is inconsistent. Sometimes it would cause a failure, and sometimes it wouldn't, so I xdescribed it for consistency.

Finally it 'all' passed...

So I went to mocharun.js and changed the timeout to 1s, and started returning the tests.
I was able to return them all except for cancel.js (gh-166), which still had non-deterministic behavior and would cause timeouts more often than not.

All in all it looks like the tests with the most inconsistency are the ones which use setTimeout in them.

In our projects, we use jasmine and mock the clock.
Mocha doesn't have this functionality, but after some research I found that you can use another library for the mocks such as sinon.js in conjunction with mocha, which can be used to mock the clock, as explained in this SO answer. You can see Sinon's mock clock api here.

This way you won't rely on the actual time passing, so you can have consistent behavior in the tests. It should also allow the slower tests to run much faster.

I hope this helps.

@petkaantonov
Copy link
Owner

That fake clock is synchronous. All the tests that use timers use it for the reason of emulating a real asynchronous operation taking the given amount of time. It is weird that the timeout of 500ms is not enough for your machine.

@EtaiG
Copy link
Contributor Author

EtaiG commented Dec 19, 2014

Indeed it is weird.
I'm using Windows 8, x64, 8GB ram with i7-3517U cpu, so this laptop shouldn't really have any performance issues.

@petkaantonov
Copy link
Owner

One thing, just to make sure, you are running these in command line, right? Because if you run in browser for example Chrome will make all timers have a 1000ms minimum time when the tab is not focused.

@EtaiG
Copy link
Contributor Author

EtaiG commented Dec 19, 2014

Yea, running it through the CLI - grunt testrun (after I got fed up with the build each time during test).

Regarding the clock- it's synchronous, but in your tests you want to emulate an asynchronous operation as well. I think emulating the clock should suffice for you.

@petkaantonov
Copy link
Owner

I'll write a fake asynchronous clock, stay tuned

@EtaiG
Copy link
Contributor Author

EtaiG commented Dec 19, 2014

Sounds good 👍

I'm guessing you mean something like:

mockClock.tick(501, function(){
   // mutation observer / messagechannel / settimeout(0) brought us here, but the clock moved 501
});

?

@petkaantonov
Copy link
Owner

I am just doing this in mochaRun.js:

var currentTime = 0;
var timers = {};
var currentId = 0;

function checkTimers() {
    Object.keys(timers).forEach(function(key) {
        var timer = timers[key];
        if (currentTime > timer.ends) {
            delete timers[key];
            timer.fn.call(global);
        }
    });
}

function setTimeout(fn, time) {
    var id = currentId++;
    time = (+time || 0) | 0;
    if (time < 0) time = 0;
    timers[id] = {
        fn: fn,
        ends: time + currentTime
    };
    return id;
}

function clearTimeout(id) {
    delete timers[id];
}

(function tick() {
    currentTime += 10;
    checkTimers();
    setImmediate(tick);
})();

global.setTimeout = setTimeout;
global.clearTimeout = clearTimeout;

Seems to work great, all the tests run instantly for me :D Ill push it to master now

@EtaiG
Copy link
Contributor Author

EtaiG commented Dec 19, 2014

looks good, except - setImmediate ?

@petkaantonov
Copy link
Owner

Well you can't call nextTick(tick) because it would crash to recursion

@EtaiG
Copy link
Contributor Author

EtaiG commented Dec 19, 2014

yes but setImmediate is not cross-browser, so if you run the tests in browsers it might explode

@petkaantonov
Copy link
Owner

This file is not even used in browser tests, if it works for you i'll implement a similar thing for the browser runner as well

@EtaiG
Copy link
Contributor Author

EtaiG commented Dec 19, 2014

👍 waiting to pull.
FYI, I have to go in about 45min, so if it's not pushed by then, I'll try to check it Sunday.

@petkaantonov
Copy link
Owner

hmm I pushed it 10 minutes ago, 40426a6

@EtaiG
Copy link
Contributor Author

EtaiG commented Dec 19, 2014

pulled & verified on several runs.
Tests are much faster now, and more importantly, no errors :)

You can close this issue :)

@petkaantonov
Copy link
Owner

Cheers

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

No branches or pull requests

4 participants