test_runner: Fix mock timer issues #50384
Conversation
An incorrect early return caused processing of due timers to stop prematurely. Fixes: nodejs#50382
mocked setInterval used the wrong increment when scheduling the new timer. Fixes: nodejs#50382
Removal of mocket timers from the priority queue was broken. It used the timerId instead of the position in the queue as index. This lead to removal of incorrect timers from the queue causing timers not to be scheduled at all. Also, aborts caused removal from the queue even when the timer was already triggered, and thus no longer present in the queue. Fixes: nodejs#50365
|
Review requested:
|
nodejs-github-bot
added
needs-ci
mika-fischer
changed the title
atlowChemi
added
author ready
github-actions
bot
removed
the
request-ci
Co-authored-by: Raz Luvaton <[email protected]>
github-actions
bot
removed
the
request-ci
|
Anything I can do to move this along? |
jasnell
added
the
commit-queue
nodejs-github-bot
removed
the
commit-queue
nodejs-github-bot
added
the
commit-queue-failed
Commit Queue failed- Loading data for nodejs/node/pull/50384 ✔ Done loading data for nodejs/node/pull/50384 ----------------------------------- PR info ------------------------------------ Title test_runner: Fix mock timer issues (#50384) Author Mika Fischer (@mika-fischer) Branch mika-fischer:fix-mock-timer-issues -> nodejs:main Labels author ready, needs-ci, test_runner Commits 5 - test_runner: add tests for various mock timer issues - test_runner: fix tick processing terminating prematurely - test_runner: fix mocked setInterval using the wrong interval - test_runner: fix incorrect cleanup of timers - Update test/parallel/test-runner-mock-timers.js Committers 2 - Mika Fischer - GitHub PR-URL: https://github.com/nodejs/node/pull/50384 Fixes: https://github.com/nodejs/node/issues/50365 Fixes: https://github.com/nodejs/node/issues/50381 Fixes: https://github.com/nodejs/node/issues/50382 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Chemi Atlow Reviewed-By: James M Snell ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/50384 Fixes: https://github.com/nodejs/node/issues/50365 Fixes: https://github.com/nodejs/node/issues/50381 Fixes: https://github.com/nodejs/node/issues/50382 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Chemi Atlow Reviewed-By: James M Snell -------------------------------------------------------------------------------- ℹ This PR was created on Wed, 25 Oct 2023 10:13:19 GMT ✔ Approvals: 3 ✔ - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/50384#pullrequestreview-1698099014 ✔ - Chemi Atlow (@atlowChemi): https://github.com/nodejs/node/pull/50384#pullrequestreview-1698665528 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/50384#pullrequestreview-1702811208 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2023-11-02T13:56:29Z: https://ci.nodejs.org/job/node-test-pull-request/55423/ - Querying data for job/node-test-pull-request/55423/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD 6b27f5b09e..83e6350b82 main -> origin/main ✔ origin/main is now up-to-date main is out of sync with origin/main. Mismatched commits: - 73aab9193b errors: improve hideStackFrames - 83e6350b82 errors: improve hideStackFrames -------------------------------------------------------------------------------- HEAD is now at 83e6350b82 errors: improve hideStackFrames ✔ Reset to origin/main - Downloading patch for 50384 From https://github.com/nodejs/node * branch refs/pull/50384/merge -> FETCH_HEAD ✔ Fetched commits as 83e6350b826a..1f91c1fed83b -------------------------------------------------------------------------------- Auto-merging test/parallel/test-runner-mock-timers.js [main fe61727cef] test_runner: add tests for various mock timer issues Author: Mika Fischer Date: Wed Oct 25 10:50:11 2023 +0200 1 file changed, 85 insertions(+) [main 3fe95d485e] test_runner: fix tick processing terminating prematurely Author: Mika Fischer Date: Tue Oct 24 19:46:03 2023 +0200 1 file changed, 1 deletion(-) [main 3ecafa459c] test_runner: fix mocked setInterval using the wrong interval Author: Mika Fischer Date: Tue Oct 24 19:47:47 2023 +0200 1 file changed, 2 insertions(+), 2 deletions(-) [main 5c4a7c7d38] test_runner: fix incorrect cleanup of timers Author: Mika Fischer Date: Wed Oct 25 09:52:39 2023 +0200 1 file changed, 14 insertions(+), 10 deletions(-) Auto-merging test/parallel/test-runner-mock-timers.js [main a95c9d8fef] Update test/parallel/test-runner-mock-timers.js Author: Mika Fischer Date: Fri Oct 27 10:15:21 2023 +0200 1 file changed, 1 insertion(+), 1 deletion(-) ✔ Patches applied There are 5 commits in the PR. Attempting autorebase. Rebasing (2/10)https://github.com/nodejs/node/actions/runs/6835327454 |
atlowChemi
added
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in 314c8f9 |
PR-URL: #50384 Fixes: #50365 Fixes: #50381 Fixes: #50382 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #50384 Fixes: #50365 Fixes: #50381 Fixes: #50382 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: James M Snell <[email protected]>
There were various issues in the mock timer implementation causing deviations from expected behavior. This adds tests and fixes these issues. See the issues below for details:
Fixes: #50365
Fixes: #50381
Fixes: #50382