tests: add test for non-integer delay timers. #8267
Conversation
|
I don't know if we want to merge this into 0.10 since it might not change much in the future. However I think it would be useful to merge it into v0.12 and at least into master. There are other ways to implement this test that wouldn't make the tests suite timeout after 1 minute in case the issue is reproduced. One possible way would be to make a child process set the timers with non-integers delays and making the test fail if the child didn't send an event to the parent after some time. However, these methods depend a lot on when the child process is scheduled for execution, and waiting for less than 1 second for it to terminate would give a lot of false positives on a loaded system (like our Jenkins nodes). Moreover, having the test timeout after one minute when the issue reappears is not too much of a problem compared to the potential impact of not detecting it. |
| process.exit(0); | ||
| } | ||
| }, TIMEOUT_DELAY); | ||
|
|
|
Thank you! Let's land it in v0.10, but first - please check the style nits ;) |
PR nodejs#8034 came with a test to make sure that timers expiry is based on monotonic time and not on wall-clock time. However, a bug in the implementation broke timers with non-integer delays. A fix for this issue was provided with PR nodejs#8073, but it didn't come with a test. Because nodejs#8073 fixed a subtle issue that could reappear in the future, and because the impact of such an issue would be significant, I suggest adding this test. The test would timeout after 1 minute if the issue was reproduced. Otherwise it will run very quickly.
misterdjules
force-pushed
the
add-non-integer-timers-delay-test
branch
from
54cbd0a to
d232a03
Compare
|
@indutny Thank you, I made the requested changes. |
|
Landed in 8e60b45, thank you! |
PR #8034 came with a test to make sure that timers expiry is based on
monotonic time and not on wall-clock time. However, a bug in the
implementation broke timers with non-integer delays. A fix for this
issue was provided with PR #8073, but it didn't come with a test.
Because #8073 fixed a subtle issue that could reappear in the future,
and because the impact of such an issue would be significant, I suggest
adding this test.
The test would timeout after 1 minute if the issue was reproduced.
Otherwise it will run very quickly.