Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

test: Calculate timeout in milliseconds #5214

Closed

Conversation

paulfryzel
Copy link

This changes the timeout granularity from seconds to milliseconds. This is useful in issues such as #4932.

The default test timeout is now 30000ms (30s) per @isaacs suggestion, down from 60s.

@Nodejs-Jenkins
Copy link

Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion.

The following commiters were not found in the CLA:

  • Paul Fryzel

Please see CONTRIBUTING.md for more information

@paulfryzel
Copy link
Author

Sorry, CLA signed.

@bnoordhuis
Copy link
Member

Maybe I missed a discussion but why milliseconds? Capping the timeout is something I agree with in principle but I don't need sub-second granularity. 30 seconds is good enough for me, I don't have a pressing need for a 29.75s timeout.

@paulfryzel
Copy link
Author

@bnoordhuis From #4932, "It'd be great to have most tests run in under 50ms, or perhaps as much as 200ms for cases where timing is actually relevant to the test."

While initially looking those tests, I thought this would be a useful change in targeting that overall goal. I talked with @isaacs briefly on IRC and he seemed to agree, as well as suggesting the new default. Now we can be a bit more specific:

$ python tools/test.py -t 200 -p tap simple/test-fs-watch | grep 'not ok'

@isaacs
Copy link

isaacs commented Apr 11, 2013

Yeah, I think 30s is great for a default, but I'd also like to reduce the overall time it takes to run tests, and setting a lower timeout can help flush those problems out.

@tjfontaine
Copy link

The tap output now includes duration of each test, and in jenkins you can sort by how long it takes for each test to run, http://jenkins.nodejs.org/view/node/job/nodejs-v0.10/lastCompletedBuild/DESTCPU=x64,label=linux/tapTestReport/

The majority of our tests complete in under a second, and there's only a handful that even take longer than 2 seconds to run.

@jasnell
Copy link
Member

jasnell commented Aug 3, 2015

Closing due to lack of activity. Can revisit if someone is interested in updating the PR. Given that this is a feature request, however, it would need to be targeted at either http://github.com/nodejs/io.js or http://github.com/nodejs/node

@jasnell jasnell closed this Aug 3, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants