-
Notifications
You must be signed in to change notification settings - Fork 29.1k
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
doc: define use case of common.platformTimeout
#11405
Conversation
Define use case of `common.platformTimeout` method in general recommendations on timers. As proposed in nodejs#11368.
@nodejs/testing |
@@ -120,7 +120,7 @@ Avoid timers unless the test is specifically testing timers. There are multiple | |||
reasons for this. Mainly, they are a source of flakiness. For a thorough | |||
explanation go [here](https://github.com/nodejs/testing/issues/27). | |||
|
|||
In the event a test needs a timer, consider using the | |||
In the event of a ['flaky tests' situation](https://github.com/nodejs/node/wiki/Flaky-tests), consider using the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this make sense to me. I can see guidance that says avoid timers but if you have to use one then consider common.platformTimeout. But I don't think its only if the test fails intermittently. We may have tests that just need more time on some platforms (ex arm) and if there is no other way than using a timer then even if the test fails 100% common.platformTimeout() may be the right advice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mhdawson, thanks for reply! Probably i haven't used term 'flaky tests' correctly here (i'm not native speaker :( ). I meant the same thing as you: we should use common.platformTimeout() only in situations when we should use timers and tests are fails on slow platforms (like you said: arm).
You can help me, if you suggest more correct term for this sentence :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late reply, I think the original wording is not too far off, possibly just update to stress only to use timers when necessary:
In the event that there is no reasonable alternative to using a timer, consider using the
Related to this is that ARM tests also get a lot more time to complete. |
This issue has been inactive for sufficiently long that it seems like perhaps it should be closed. Feel free to re-open (or leave a comment requesting that it be re-opened) if you disagree. I'm just tidying up and not acting on a super-strong opinion or anything like that. |
Define use case of
common.platformTimeout
method in general recommendations on timers. As proposed in #11368.Feel free to offer grammar fixes :)
Checklist
Affected core subsystem(s)
doc