test_runner: preserve original property descriptor when mutating objects #49433

ErickWendel · 2023-09-01T17:13:36Z

In this comment, #49397 (comment) @ljharb suggested preserving the behavior and attributes of properties as they were before the modification

cc @nodejs/test_runner

nodejs-github-bot · 2023-09-01T17:13:41Z

Review requested:

@nodejs/test_runner

ljharb

would be good to add a test or two checking enumerability, since that's likely the only thing different with this change

lib/internal/test_runner/mock/mock_timers.js

ErickWendel · 2023-09-01T17:19:11Z

would be good to add a test or two checking enumerability, since that's likely the only thing different with this change

Uh, good catch! Gonna do it in a few

Signed-off-by: Erick Wendel <[email protected]>

ErickWendel · 2023-09-01T19:03:10Z

would be good to add a test or two checking enumerability, since that's likely the only thing different with this change

done!

ljharb

nice, this is way cleaner

nodejs-github-bot · 2023-09-02T12:30:48Z

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

nodejs-github-bot · 2023-09-02T19:22:27Z

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

nodejs-github-bot · 2023-09-03T17:20:32Z

Commit Queue failed

- Loading data for nodejs/node/pull/49433 ✔ Done loading data for nodejs/node/pull/49433 ----------------------------------- PR info ------------------------------------ Title test_runner: preserve original property descriptor when mutating objects (#49433) Author Erick Wendel (@ErickWendel) Branch ErickWendel:test_runner/mock-preserve-original-property-descriptor -> nodejs:main Labels author ready, needs-ci, test_runner Commits 3 - test_runner: preserve original property descriptor - test_runner: add test for checking the property descriptor state - test_runner: refactoring functions Committers 1 - Erick Wendel PR-URL: https://github.com/nodejs/node/pull/49433 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Raz Luvaton Reviewed-By: Chemi Atlow Reviewed-By: Moshe Atlow ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/49433 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Raz Luvaton Reviewed-By: Chemi Atlow Reviewed-By: Moshe Atlow -------------------------------------------------------------------------------- ℹ This PR was created on Fri, 01 Sep 2023 17:13:36 GMT ✔ Approvals: 4 ✔ - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/49433#pullrequestreview-1608049890 ✔ - Raz Luvaton (@rluvaton): https://github.com/nodejs/node/pull/49433#pullrequestreview-1608346449 ✔ - Chemi Atlow (@atlowChemi): https://github.com/nodejs/node/pull/49433#pullrequestreview-1608366919 ✔ - Moshe Atlow (@MoLow) (TSC): https://github.com/nodejs/node/pull/49433#pullrequestreview-1608411583 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2023-09-02T19:22:27Z: https://ci.nodejs.org/job/node-test-pull-request/53694/ - Querying data for job/node-test-pull-request/53694/ ✔ 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 ✔ origin/main is now up-to-date - Downloading patch for 49433 From https://github.com/nodejs/node * branch refs/pull/49433/merge -> FETCH_HEAD ✔ Fetched commits as 0add7a8f0c92..64f872a28f86 -------------------------------------------------------------------------------- [main 9ce045d826] test_runner: preserve original property descriptor Author: Erick Wendel Date: Fri Sep 1 14:07:12 2023 -0300 1 file changed, 137 insertions(+), 35 deletions(-) [main 9a7621b25c] test_runner: add test for checking the property descriptor state Author: Erick Wendel Date: Fri Sep 1 15:55:26 2023 -0300 1 file changed, 49 insertions(+) [main 2de5750354] test_runner: refactoring functions Author: Erick Wendel Date: Fri Sep 1 15:59:39 2023 -0300 2 files changed, 162 insertions(+), 141 deletions(-) ✔ Patches applied There are 3 commits in the PR. Attempting autorebase. Rebasing (2/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test_runner: preserve original property descriptor

PR-URL: #49433
Reviewed-By: Benjamin Gruenbaum [email protected]
Reviewed-By: Raz Luvaton [email protected]
Reviewed-By: Chemi Atlow [email protected]
Reviewed-By: Moshe Atlow [email protected]

[detached HEAD dc9c314b0f] test_runner: preserve original property descriptor
Author: Erick Wendel [email protected]
Date: Fri Sep 1 14:07:12 2023 -0300
1 file changed, 137 insertions(+), 35 deletions(-)
Rebasing (3/6)
Rebasing (4/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test_runner: add test for checking the property descriptor state

Signed-off-by: Erick Wendel [email protected]
PR-URL: #49433
Reviewed-By: Benjamin Gruenbaum [email protected]
Reviewed-By: Raz Luvaton [email protected]
Reviewed-By: Chemi Atlow [email protected]
Reviewed-By: Moshe Atlow [email protected]

[detached HEAD 496538d949] test_runner: add test for checking the property descriptor state
Author: Erick Wendel [email protected]
Date: Fri Sep 1 15:55:26 2023 -0300
1 file changed, 49 insertions(+)
Rebasing (5/6)
Rebasing (6/6)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test_runner: refactoring functions

Signed-off-by: Erick Wendel [email protected]
PR-URL: #49433
Reviewed-By: Benjamin Gruenbaum [email protected]
Reviewed-By: Raz Luvaton [email protected]
Reviewed-By: Chemi Atlow [email protected]
Reviewed-By: Moshe Atlow [email protected]

[detached HEAD 1dc3702999] test_runner: refactoring functions
Author: Erick Wendel [email protected]
Date: Fri Sep 1 15:59:39 2023 -0300
2 files changed, 162 insertions(+), 141 deletions(-)

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/6065810018

nodejs-github-bot · 2023-09-03T19:08:27Z

Landed in 8e82cfc

PR-URL: #49433 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Raz Luvaton <[email protected]> Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

PR-URL: nodejs#49433 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Raz Luvaton <[email protected]> Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

PR-URL: #49433 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Raz Luvaton <[email protected]> Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

PR-URL: nodejs/node#49433 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Raz Luvaton <[email protected]> Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

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 Sep 1, 2023

test_runner: preserve original property descriptor

3fda352

ErickWendel force-pushed the test_runner/mock-preserve-original-property-descriptor branch from 768af5c to 3fda352 Compare September 1, 2023 17:15

ljharb approved these changes Sep 1, 2023

View reviewed changes

lib/internal/test_runner/mock/mock_timers.js Outdated Show resolved Hide resolved

ErickWendel self-assigned this Sep 1, 2023

ErickWendel added 2 commits September 1, 2023 15:55

test_runner: add test for checking the property descriptor state

77caf13

Signed-off-by: Erick Wendel <[email protected]>

test_runner: refactoring functions

64f872a

Signed-off-by: Erick Wendel <[email protected]>

ErickWendel force-pushed the test_runner/mock-preserve-original-property-descriptor branch from 93a9e75 to 64f872a Compare September 1, 2023 19:02

ljharb approved these changes Sep 1, 2023

View reviewed changes

benjamingr approved these changes Sep 2, 2023

View reviewed changes

ErickWendel added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 2, 2023

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

rluvaton approved these changes Sep 2, 2023

View reviewed changes

atlowChemi approved these changes Sep 2, 2023

View reviewed changes

atlowChemi added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 2, 2023

MoLow approved these changes Sep 3, 2023

View reviewed changes

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

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 Sep 3, 2023

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

nodejs-github-bot merged commit 8e82cfc into nodejs:main Sep 3, 2023
39 checks passed

UlisesGascon mentioned this pull request Sep 10, 2023

v20.7.0 proposal #49592

Merged

targos mentioned this pull request Nov 28, 2023

v18.19.0 release proposal #50953

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test_runner: preserve original property descriptor when mutating objects #49433

test_runner: preserve original property descriptor when mutating objects #49433

ErickWendel commented Sep 1, 2023

nodejs-github-bot commented Sep 1, 2023 •

edited by ErickWendel

Loading

ljharb left a comment

ErickWendel commented Sep 1, 2023

ErickWendel commented Sep 1, 2023

ljharb left a comment •

edited

Loading

nodejs-github-bot commented Sep 2, 2023

nodejs-github-bot commented Sep 2, 2023

nodejs-github-bot commented Sep 3, 2023

PR-URL: #49433
Reviewed-By: Benjamin Gruenbaum [email protected]
Reviewed-By: Raz Luvaton [email protected]
Reviewed-By: Chemi Atlow [email protected]
Reviewed-By: Moshe Atlow [email protected]

Signed-off-by: Erick Wendel [email protected]
PR-URL: #49433
Reviewed-By: Benjamin Gruenbaum [email protected]
Reviewed-By: Raz Luvaton [email protected]
Reviewed-By: Chemi Atlow [email protected]
Reviewed-By: Moshe Atlow [email protected]

Signed-off-by: Erick Wendel [email protected]
PR-URL: #49433
Reviewed-By: Benjamin Gruenbaum [email protected]
Reviewed-By: Raz Luvaton [email protected]
Reviewed-By: Chemi Atlow [email protected]
Reviewed-By: Moshe Atlow [email protected]

Successfully rebased and updated refs/heads/main.

nodejs-github-bot commented Sep 3, 2023

test_runner: preserve original property descriptor when mutating objects #49433

test_runner: preserve original property descriptor when mutating objects #49433

Conversation

ErickWendel commented Sep 1, 2023

nodejs-github-bot commented Sep 1, 2023 • edited by ErickWendel Loading

ljharb left a comment

Choose a reason for hiding this comment

ErickWendel commented Sep 1, 2023

ErickWendel commented Sep 1, 2023

ljharb left a comment • edited Loading

Choose a reason for hiding this comment

nodejs-github-bot commented Sep 2, 2023

nodejs-github-bot commented Sep 2, 2023

nodejs-github-bot commented Sep 3, 2023

PR-URL: #49433 Reviewed-By: Benjamin Gruenbaum [email protected] Reviewed-By: Raz Luvaton [email protected] Reviewed-By: Chemi Atlow [email protected] Reviewed-By: Moshe Atlow [email protected]

Signed-off-by: Erick Wendel [email protected] PR-URL: #49433 Reviewed-By: Benjamin Gruenbaum [email protected] Reviewed-By: Raz Luvaton [email protected] Reviewed-By: Chemi Atlow [email protected] Reviewed-By: Moshe Atlow [email protected]

Signed-off-by: Erick Wendel [email protected] PR-URL: #49433 Reviewed-By: Benjamin Gruenbaum [email protected] Reviewed-By: Raz Luvaton [email protected] Reviewed-By: Chemi Atlow [email protected] Reviewed-By: Moshe Atlow [email protected]

Successfully rebased and updated refs/heads/main.

nodejs-github-bot commented Sep 3, 2023

nodejs-github-bot commented Sep 1, 2023 •

edited by ErickWendel

Loading

ljharb left a comment •

edited

Loading

PR-URL: #49433
Reviewed-By: Benjamin Gruenbaum [email protected]
Reviewed-By: Raz Luvaton [email protected]
Reviewed-By: Chemi Atlow [email protected]
Reviewed-By: Moshe Atlow [email protected]

Signed-off-by: Erick Wendel [email protected]
PR-URL: #49433
Reviewed-By: Benjamin Gruenbaum [email protected]
Reviewed-By: Raz Luvaton [email protected]
Reviewed-By: Chemi Atlow [email protected]
Reviewed-By: Moshe Atlow [email protected]

Signed-off-by: Erick Wendel [email protected]
PR-URL: #49433
Reviewed-By: Benjamin Gruenbaum [email protected]
Reviewed-By: Raz Luvaton [email protected]
Reviewed-By: Chemi Atlow [email protected]
Reviewed-By: Moshe Atlow [email protected]