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

Add timeoutAfter method #132

Merged
merged 5 commits into from
Jan 19, 2015
Merged

Add timeoutAfter method #132

merged 5 commits into from
Jan 19, 2015

Conversation

FredrikNoren
Copy link
Contributor

To automatically timeout long running tests

@Raynos
Copy link
Collaborator

Raynos commented Jan 16, 2015

@FredrikNoren

do you want a per test timeout or a global timeout ?
Generally I find that tests that time out are obvious, they just hang.

You need external timeouts anyway, because anything can hang including resource leaks and infinite loops.

What's the use-case for this ?

@FredrikNoren
Copy link
Contributor Author

@Raynos Per test preferably. The use case is any asynchronous code that might crash or fail to return for whatever reason. An example would be if you work with any kind of event driven or streaming code, and you want to test that: if you mess up but don't have timeouts the test will just stop at the particular example that doesn't work and never fail and continue to the other tests.

@Raynos
Copy link
Collaborator

Raynos commented Jan 16, 2015

@FredrikNoren the same holds for any other failures like thrown exceptions or infinite loops, it just halts your code.

@FredrikNoren
Copy link
Contributor Author

@Raynos I guess it's probably a matter of taste but exceptions are handled as failures in mocha. Infinite loops are hard to get around if it's the same process though.

The reason it might be advantageous to fail the tests is that if you fail them the rest of the tests will still run, so if one of the tests fails early on you can still know that everything else works. The way it is now it will just halt and you don't know if it's just this one test that fails or if all your tests fails (which might be completely unrelated to that first test which fails).

@Raynos
Copy link
Collaborator

Raynos commented Jan 16, 2015

@FredrikNoren I think this feature is legit.

Do we have a preference between

test('foo', function (t) {
    t.timeoutAfter(5000);
    ...
});

And

test('foo', {
    timeout: 5000
}, function (t) {
    ...
});

I favor the later as we already use that for { skip: true }

@FredrikNoren
Copy link
Contributor Author

@Raynos either is fine for me. I guess it would probably even be pretty easy to support both (i.e. if(opt.timeout) this.timeoutAfter(opt.timeout))

@FredrikNoren
Copy link
Contributor Author

@Raynos added support for it to the PR

@Raynos
Copy link
Collaborator

Raynos commented Jan 16, 2015

I have a preference for not extending the public interface of t personally, but maybe I'm weird.

I have a seperate PR that is trying to make the public interface of t be exactly node assert + .end().

We can support both, but it is nice to only have one interface.

@Raynos
Copy link
Collaborator

Raynos commented Jan 17, 2015

@FredrikNoren

let's leave the method in place and document the { timeout: 5000 } option in the README then its good to go.

@FredrikNoren
Copy link
Contributor Author

@Raynos Cool, I added opts to the documentation. From what I could tell skip was the only previously available one.

Raynos added a commit that referenced this pull request Jan 19, 2015
@Raynos Raynos merged commit 2a60e79 into tape-testing:master Jan 19, 2015
@Raynos
Copy link
Collaborator

Raynos commented Jan 19, 2015

@FredrikNoren sweet, thank you :)

@FredrikNoren
Copy link
Contributor Author

@Raynos Thanks for the quick release!

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.

None yet

2 participants