Skip to content

Conversation

@mika-fischer
Copy link
Contributor

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

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
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 25, 2023

Review requested:

  • @nodejs/test_runner

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Oct 25, 2023
@mika-fischermika-fischer changed the title Fix mock timer issuestest_runner: Fix mock timer issuesOct 25, 2023
Copy link
Member

@benjamingrbenjamingr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

@atlowChemiatlowChemi added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 26, 2023
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 26, 2023
@nodejs-github-bot
Copy link
Collaborator

@anonriganonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 31, 2023
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 31, 2023
@nodejs-github-bot
Copy link
Collaborator

@mika-fischer
Copy link
ContributorAuthor

Anything I can do to move this along?

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 2, 2023

@jasnelljasnell added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 11, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 11, 2023
@nodejs-github-botnodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Nov 11, 2023
@nodejs-github-bot
Copy link
Collaborator

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) 

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test_runner: add tests for various mock timer issues

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]

[detached HEAD 83a9752d6b] test_runner: add tests for various mock timer issues
Author: Mika Fischer [email protected]
Date: Wed Oct 25 10:50:11 2023 +0200
1 file changed, 85 insertions(+)
Rebasing (3/10)
Rebasing (4/10)

Executing: git node land --amend --yes
⚠ Found Fixes: #50382, skipping..
--------------------------------- New Message ----------------------------------
test_runner: fix tick processing terminating prematurely

An incorrect early return caused processing of due timers to stop
prematurely.

Fixes: #50382
PR-URL: #50384
Fixes: #50365
Fixes: #50381
Reviewed-By: Benjamin Gruenbaum [email protected]
Reviewed-By: Chemi Atlow [email protected]
Reviewed-By: James M Snell [email protected]

[detached HEAD 01708bf8db] test_runner: fix tick processing terminating prematurely
Author: Mika Fischer [email protected]
Date: Tue Oct 24 19:46:03 2023 +0200
1 file changed, 1 deletion(-)
Rebasing (5/10)
Rebasing (6/10)

Executing: git node land --amend --yes
⚠ Found Fixes: #50382, skipping..
--------------------------------- New Message ----------------------------------
test_runner: fix mocked setInterval using the wrong interval

mocked setInterval used the wrong increment when scheduling the new
timer.

Fixes: #50382
PR-URL: #50384
Fixes: #50365
Fixes: #50381
Reviewed-By: Benjamin Gruenbaum [email protected]
Reviewed-By: Chemi Atlow [email protected]
Reviewed-By: James M Snell [email protected]

[detached HEAD 0518b2218e] test_runner: fix mocked setInterval using the wrong interval
Author: Mika Fischer [email protected]
Date: Tue Oct 24 19:47:47 2023 +0200
1 file changed, 2 insertions(+), 2 deletions(-)
Rebasing (7/10)
Rebasing (8/10)

Executing: git node land --amend --yes
⚠ Found Fixes: #50365, skipping..
--------------------------------- New Message ----------------------------------
test_runner: fix incorrect cleanup of timers

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: #50365
PR-URL: #50384
Fixes: #50381
Fixes: #50382
Reviewed-By: Benjamin Gruenbaum [email protected]
Reviewed-By: Chemi Atlow [email protected]
Reviewed-By: James M Snell [email protected]

[detached HEAD 5aff931d9b] test_runner: fix incorrect cleanup of timers
Author: Mika Fischer [email protected]
Date: Wed Oct 25 09:52:39 2023 +0200
1 file changed, 14 insertions(+), 10 deletions(-)
Rebasing (9/10)
Rebasing (10/10)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
Update test/parallel/test-runner-mock-timers.js

Co-authored-by: Raz Luvaton [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]

[detached HEAD 89e6a5b6d8] Update test/parallel/test-runner-mock-timers.js
Author: Mika Fischer [email protected]
Date: Fri Oct 27 10:15:21 2023 +0200
1 file changed, 1 insertion(+), 1 deletion(-)

Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/6835327454

@atlowChemiatlowChemi added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Nov 12, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 12, 2023
@nodejs-github-botnodejs-github-bot merged commit 314c8f9 into nodejs:mainNov 12, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 314c8f9

targos pushed a commit that referenced this pull request Nov 23, 2023
PR-URL: #50384Fixes: #50365Fixes: #50381Fixes: #50382 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: James M Snell <[email protected]>
@RafaelGSSRafaelGSS mentioned this pull request Nov 28, 2023
UlisesGascon pushed a commit that referenced this pull request Dec 11, 2023
PR-URL: #50384Fixes: #50365Fixes: #50381Fixes: #50382 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: James M Snell <[email protected]>
@UlisesGasconUlisesGascon mentioned this pull request Dec 12, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.needs-ciPRs that need a full CI run.test_runnerIssues and PRs related to the test runner subsystem.

Projects

None yet

7 participants

@mika-fischer@nodejs-github-bot@jasnell@benjamingr@anonrig@rluvaton@atlowChemi