Skip to content
This repository has been archived by the owner on May 13, 2019. It is now read-only.

Review the use of timeouts #19

Closed
santigimeno opened this issue Feb 24, 2016 · 2 comments
Closed

Review the use of timeouts #19

santigimeno opened this issue Feb 24, 2016 · 2 comments

Comments

@santigimeno
Copy link
Member

I'm not sure if this deserves an issue or if there's anything actionable, but anyway...

Lately, most of the failing tests I've found: nodejs/node#5342, nodejs/node#5339, nodejs/node#5129 were flaky because of the use (misuse?) of timers. It seems to me that a lot of times, the use of timers is not needed, as we can rely on the test runner timeout, and they become a source of flakiness. I could not agree more with this post from @Trott nodejs/node#5342 (comment). Should we set a position on this and maybe document it somewhere?

@Trott
Copy link
Member

Trott commented Feb 25, 2016

I would love to get some consensus on whether arbitrary timers is (on the one hand) a valid way to provide a helpful error message to people running tests without the wrapper or (on the other hand) an anti-pattern that introduces maddening flakiness into our tests. (Spoiler alert: I think it's an anti-pattern.)

@santigimeno
Copy link
Member Author

Closing in favor of #27

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants