test_runner: Fix mock timer promisified setInterval return value by mika-fischer · Pull Request #50331 · nodejs/node

mika-fischer · 2023-10-22T20:11:55Z

promisified setInterval can be passed a value that is returned by the async iterable. The mocked promisified setInterval used this value for its own purposes. This brings the mocked version in line with the original.

nodejs-github-bot · 2023-10-22T20:11:59Z

Review requested:

@nodejs/test_runner

nodejs-github-bot · 2023-10-23T07:45:21Z

CI: https://ci.nodejs.org/job/node-test-pull-request/55140/

nodejs-github-bot · 2023-10-23T10:46:40Z

CI: https://ci.nodejs.org/job/node-test-pull-request/55143/

ErickWendel

LGTM. @mika-fischer congrats on your first PR on the nodejs repo!! It's really helpful!

MoLow · 2023-10-23T11:25:50Z

@mika-fischer the PR needs a rebase

promisified setInterval can be passed a value that is returned by the async iterable. The mocked promisified setInterval used this value for its own purposes. This brings the mocked version in line with the original. Fixes: nodejs#50307

Some tests relied on the custom return value and were broken by the change to always return the value provided by the caller.

mika-fischer · 2023-10-23T12:29:26Z

LGTM. @mika-fischer congrats on your first PR on the nodejs repo!! It's really helpful!

Thanks @ErickWendel!

mika-fischer · 2023-10-23T12:29:44Z

@mika-fischer the PR needs a rebase

done @MoLow

nodejs-github-bot · 2023-10-23T12:58:54Z

CI: https://ci.nodejs.org/job/node-test-pull-request/55149/

mika-fischer · 2023-10-23T18:10:05Z

I'm not sure why some tests are failing, these seem unrelated to the changes in this PR. Let me know if there's anything I can do to move this along.

nodejs-github-bot · 2023-10-23T18:44:37Z

CI: https://ci.nodejs.org/job/node-test-pull-request/55158/

nodejs-github-bot · 2023-10-24T20:22:24Z

Commit Queue failed

- Loading data for nodejs/node/pull/50331
✔  Done loading data for nodejs/node/pull/50331
----------------------------------- PR info ------------------------------------
Title      test_runner: Fix mock timer promisified setInterval return value (#50331)
Author     Mika Fischer  (@mika-fischer, first-time contributor)
Branch     mika-fischer:fix-50307 -> nodejs:main
Labels     author ready, needs-ci, commit-queue-squash, test_runner
Commits    3
 - test_runner: test return value of mocked promisified timers
 - test_runner: fix mock timer promisified setInterval return value
 - test_runner: fix mock timer setInterval tests
Committers 1
 - Mika Fischer 
PR-URL: https://github.com/nodejs/node/pull/50331
Reviewed-By: Moshe Atlow 
Reviewed-By: Chemi Atlow 
Reviewed-By: Erick Wendel 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/50331
Reviewed-By: Moshe Atlow 
Reviewed-By: Chemi Atlow 
Reviewed-By: Erick Wendel 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - test_runner: test return value of mocked promisified timers
   ⚠  - test_runner: fix mock timer promisified setInterval return value
   ⚠  - test_runner: fix mock timer setInterval tests
   ℹ  This PR was created on Sun, 22 Oct 2023 20:11:55 GMT
   ✔  Approvals: 3
   ✔  - Moshe Atlow (@MoLow) (TSC): https://github.com/nodejs/node/pull/50331#pullrequestreview-1692229637
   ✔  - Chemi Atlow (@atlowChemi): https://github.com/nodejs/node/pull/50331#pullrequestreview-1691892475
   ✔  - Erick Wendel (@erickwendel): https://github.com/nodejs/node/pull/50331#pullrequestreview-1692276984
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-10-23T18:44:37Z: https://ci.nodejs.org/job/node-test-pull-request/55158/
- Querying data for job/node-test-pull-request/55158/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

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

nodejs-github-bot · 2023-10-25T07:15:57Z

Landed in 4f5db8b

mika-fischer · 2023-10-25T10:15:25Z

Did the closure of #50307 get dropped somehow? Should I have specified it differently? Should I close the issue now manually?

benjamingr · 2023-10-25T19:06:36Z

Typically it needs Fixes: or Closes: in the PR description for GH to pick it up.

Thank you for your contribution!

PR-URL: nodejs#50331 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Erick Wendel <erick.workspace@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

PR-URL: #50331 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Erick Wendel <erick.workspace@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

nodejs-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 22, 2023

mika-fischer mentioned this pull request Oct 22, 2023

node test mock timer promisified setTimeout & setInterval don't always return the specified value #50307

Closed

MoLow approved these changes Oct 22, 2023

View reviewed changes

MoLow requested review from ErickWendel and benjamingr October 22, 2023 20:43

mika-fischer force-pushed the fix-50307 branch from 78d949e to af38302 Compare October 23, 2023 06:30

atlowChemi approved these changes Oct 23, 2023

View reviewed changes

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 23, 2023

mika-fischer force-pushed the fix-50307 branch 2 times, most recently from 02d7e2e to e65beea Compare October 23, 2023 08:41

MoLow approved these changes Oct 23, 2023

View reviewed changes

MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 23, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 23, 2023

ErickWendel approved these changes Oct 23, 2023

View reviewed changes

mika-fischer added 3 commits October 23, 2023 14:24

test_runner: test return value of mocked promisified timers

ea2a31e

test_runner: fix mock timer setInterval tests

06d8b9d

Some tests relied on the custom return value and were broken by the change to always return the value provided by the caller.

mika-fischer force-pushed the fix-50307 branch from e65beea to 06d8b9d Compare October 23, 2023 12:29

MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 23, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 23, 2023

MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 23, 2023

github-actions bot mentioned this pull request Oct 24, 2023

CI Reliability 2023-10-24 nodejs/reliability#696

Open

27 tasks

mika-fischer mentioned this pull request Oct 24, 2023

test_runner: mock timer promisified setTimeout cleanup goes wrong losing some timers #50365

Closed

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Oct 24, 2023

benjamingr approved these changes Oct 24, 2023

View reviewed changes

github-actions bot mentioned this pull request Oct 25, 2023

CI Reliability 2023-10-25 nodejs/reliability#697

Open

16 tasks

MoLow added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Oct 25, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 25, 2023

nodejs-github-bot merged commit 4f5db8b into nodejs:main Oct 25, 2023

mika-fischer deleted the fix-50307 branch October 25, 2023 07:29

MoLow linked an issue Oct 25, 2023 that may be closed by this pull request

node test mock timer promisified setTimeout & setInterval don't always return the specified value #50307

Closed

github-actions bot mentioned this pull request Oct 26, 2023

CI Reliability 2023-10-26 nodejs/reliability#698

Open

20 tasks

targos mentioned this pull request Nov 12, 2023

v21.2.0 release proposal #50681

Merged

UlisesGascon mentioned this pull request Dec 12, 2023

v20.11.0 proposal #51124

Merged

Uh oh!

Conversation

mika-fischer commented Oct 22, 2023

Uh oh!

nodejs-github-bot commented Oct 22, 2023

Uh oh!

nodejs-github-bot commented Oct 23, 2023

Uh oh!

nodejs-github-bot commented Oct 23, 2023

Uh oh!

ErickWendel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MoLow commented Oct 23, 2023

Uh oh!

mika-fischer commented Oct 23, 2023

Uh oh!

mika-fischer commented Oct 23, 2023

Uh oh!

nodejs-github-bot commented Oct 23, 2023

Uh oh!

mika-fischer commented Oct 23, 2023

Uh oh!

nodejs-github-bot commented Oct 23, 2023

Uh oh!

nodejs-github-bot commented Oct 24, 2023

Uh oh!

nodejs-github-bot commented Oct 25, 2023

Uh oh!

mika-fischer commented Oct 25, 2023

Uh oh!

benjamingr commented Oct 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ErickWendel left a comment •

edited

Loading