Usage of common.platformTimeout in tests guide
Hello! In 'writing tests' guide there is recommendation of usage common.platformTimeout
in setTimeout
. Today I've asked @Trott
in IRC PM about usage of this method in some irrelevant to this issue test:
Chat log (gist)
notarseniy
Thank you for idea for first PR. I've wrote a day ago to #node-dev about one thing that I found: http://logs.libuv.org/node-dev/2017-02-12#14:24:04.509
Trott
Not all callbacks should have `common.mustCall()`.
Trott
Sometimes you can not say for certain how many times a callback will be called or even whether it will be called.
notarseniy
umm.. sorry, i misunderstood you :) Where is common.mustCall?
Trott
Oh, sorry, you're asking about platformTimeout() and not mustCall().
Trott
Let me try again...
Trott
platformTimeout() is not always required.
Trott
And in fact to the extent it can be avoided, all the better.
Trott
platformTimeout() is a sign of a problem in the test, really. To the extent we need it, it should be used. But to the extent that we can avoid it, it is better for the tests IMO.
Trott
So, basically, I wouldn't add it to a test unless you are certain there's a problem that it helps with.
notarseniy
wow, okay! probably got it. really good explanation! maybe we should write about this in https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md?
Trott
(I would love to be able to get rid of platformTimeout() some day, but that would involve fixing all the tests that use it, and a small number of them may not even be fixable.)
Trott
Oh, yeah, that needs more detail for sure.
tl;dr: common.platformTimeout
is for specific cases and probably we should avoid using it.
I think there's need of redefining recommendation on timeouts for accuracy of common.platformTimeout
usage.
(sorry for spelling errors, trying to be better :)
/cc @Trott