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

Runnable.timeout(0) no longer allows silent exits #850

Merged
merged 1 commit into from
May 31, 2013

Conversation

Schoonology
Copy link
Contributor

Otherwise, it's possible to create an async test with a timeout of 0 and have Node exit silently. This way, it should be obvious the async test is malformed.

@Schoonology
Copy link
Contributor Author

Updated pull request to match the expectations of http://visionmedia.github.io/mocha/#test-specific-timeouts

});
test.run(function(err){
err.message.should.include('timeout');
done();
Copy link
Contributor

Choose a reason for hiding this comment

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

this would have passed before as well no? maybe I'm reading it wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would have. It's the pending test that would have caused all tests to exit. I removed that test when Sam Roberts and I discussed what this.timeout(0) should mean, and with the new behaviour (above), the test as it was implemented would instead cause the tests to never end.

If someone can come up with an awesome test for this.timeout(0), then the latter will no longer be pending. The former exists simply because there wasn't a test with this.timeout, as opposed to run.timeout. Minor, but it was useful for testing this issue, it could be useful for others.

Otherwise it's possible to create an async test with a timeout of 0 and have Node exit silently.
This way, it should be obvious the async test is malformed.
@Schoonology
Copy link
Contributor Author

Updated.

@tj
Copy link
Contributor

tj commented May 22, 2013

actually it still behaves the way I'd expect:

describe('something', function(){
  it('should work', function(done){
    this.timeout(0);
    setTimeout(function(){
      done();
    }, 6000);
  })
})

executing after the 6s and exiting

@tj tj closed this May 22, 2013
@tj
Copy link
Contributor

tj commented May 22, 2013

wrong button

@tj tj reopened this May 22, 2013
@Schoonology
Copy link
Contributor Author

The difference is the output if you don't have the setTimeout (making this hard to test):

describe('something', function () {
  it('should work', function (done) {
    this.timeout(0);
  })
})

I'll grant you that this is a malformed test. However, with a sufficiently complicated suite (like the one that elevated the issue), this kind of malformed test becomes less obvious than this. What the above test does is exit without a report. What it should do is never exit, indicating a problem, especially when used with a reporter like spec.

@tj
Copy link
Contributor

tj commented May 22, 2013

ah ok i see what you're saying now

@bitwiseman
Copy link
Contributor

👍

Once #860 is in, we can do this to test it:

describe('something', function () {
  it('should fail because timeout 0 waits forever', function (done) {
    this.timeout(0);
    setTimeout(function(){
      done('Boom!');
    }, 2000);
  })
})

tj added a commit that referenced this pull request May 31, 2013
Runnable.timeout(0) no longer allows silent exits
@tj tj merged commit 3abf8f6 into mochajs:master May 31, 2013
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