FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test_runner: replace forEach() with for ... of by tomhaddad · Pull Request #50595 · nodejs/node · GitHub

/ node Public

test_runner: replace forEach() with for ... of - #50595

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
tomhaddad:test-runner-mock-timers-replace-foreach
Nov 8, 2023
ConversationCommits (1)ChecksFiles changed
Merged

test_runner: replace forEach() with for ... of#50595
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
tomhaddad:test-runner-mock-timers-replace-foreach

Conversation

tomhaddad commented Nov 7, 2023 •
edited by marco-ippolito
Loading

Copy link
Copy Markdown
Contributor

Replaces usage of forEach() with for ... of in test-runner-mock-timers.js
#NodeConf

nodejs-github-bot commented Nov 7, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner

nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Nov 7, 2023

ljharb commented Nov 7, 2023

Copy link
Copy Markdown
Member

why? assert.forEach isn’t the same as a for..of loop.

marco-ippolito added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Nov 7, 2023
tomhaddad force-pushed the test-runner-mock-timers-replace-foreach branch from 853f68e to 18bd8aa Compare November 7, 2023 15:23
tomhaddad force-pushed the test-runner-mock-timers-replace-foreach branch from 18bd8aa to d907f14 Compare November 7, 2023 15:28
atlowChemi reviewed Nov 7, 2023
View reviewed changes

atlowChemi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Hi @tomhaddad, what is the reason for this change?
This is not a part of core in which we avoid prototype calls like forEach()

lpinca commented Nov 7, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

@atlowChemi it's a code-and-learn exercise.

lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 7, 2023
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 7, 2023

nodejs-github-bot commented Nov 7, 2023

Copy link
Copy Markdown
Collaborator

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

ljharb commented Nov 7, 2023

Copy link
Copy Markdown
Member

That doesn’t mean the change should just land tho, right? Or does it mean the PR isn’t going to actually land, it’s just to demonstrate how to do one?

github-actions Bot mentioned this pull request Nov 8, 2023
Open
16 tasks
aduh95 approved these changes Nov 8, 2023
View reviewed changes

aduh95 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Note to reviewers: this is a test file, we do not have any requirements for how to iterate over arrays in test files, both methods are fine by me.

nodejs-github-bot commented Nov 8, 2023

Copy link
Copy Markdown
Collaborator

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

aduh95 commented Nov 8, 2023

Copy link
Copy Markdown
Contributor

That doesn’t mean the change should just land tho, right? Or does it mean the PR isn’t going to actually land, it’s just to demonstrate how to do one?

As any PR, this can land after if it has approval and green CI, after the waiting time. Code and Learn consists on making low hanging fruit PRs to get a first contribution to the repo, with the hope that it will result in more contributions in the future.

Thanks for the PR @tomhaddad :)

aduh95 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Nov 8, 2023

tomhaddad commented Nov 8, 2023

Copy link
Copy Markdown
Contributor Author

Thanks for the PR @tomhaddad :)

You are most welcome, happy to have made my first contribution :)

aduh95 added the fast-track PRs that do not need to wait for 48 hours to land. label Nov 8, 2023

github-actions Bot commented Nov 8, 2023

Copy link
Copy Markdown
Contributor

Fast-track has been requested by @aduh95. Please 👍 to approve.

MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 8, 2023
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 8, 2023
nodejs-github-bot merged commit 8588ee1 into nodejs:main Nov 8, 2023

nodejs-github-bot commented Nov 8, 2023

Copy link
Copy Markdown
Collaborator

Landed in 8588ee1

github-actions Bot mentioned this pull request Nov 9, 2023
Open
20 tasks
This was referenced Nov 10, 2023
Open
Open
targos pushed a commit that referenced this pull request Nov 11, 2023
test_runner: replace forEach with for of
992ad0d 
PR-URL: #50595
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
github-actions Bot mentioned this pull request Nov 12, 2023
Open
18 tasks
targos mentioned this pull request Nov 12, 2023
Merged
targos pushed a commit that referenced this pull request Nov 14, 2023
test_runner: replace forEach with for of
ae4a7ba 
PR-URL: #50595
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
UlisesGascon pushed a commit that referenced this pull request Dec 11, 2023
test_runner: replace forEach with for of
c33a84a 
PR-URL: #50595
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
UlisesGascon mentioned this pull request Dec 12, 2023
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

atlowChemi atlowChemi left review comments
benjamingr benjamingr approved these changes
lpinca lpinca approved these changes
MoLow MoLow approved these changes
aduh95 aduh95 approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. fast-track PRs that do not need to wait for 48 hours to land. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

Back | FazBrowse Home | New Git URL