test: use test runner in eventtarget-once-twice test by alexweej · Pull Request #55752 · nodejs/node

alexweej · 2024-11-06T16:09:12Z

No description provided.

test/parallel/test-eventtarget-once-twice.js

avivkeller · 2024-11-06T16:56:08Z

test/parallel/test-eventtarget-once-twice.js

-  await once(et, 'foo');
-  await once(et, 'foo');
-})().then(common.mustCall());
+test('should resolve `once` twice', (t, done) => {


This can be simplified as an asynchronous function, right? Calling "done" in the middle might lead to a race condition that generates extraneous asynchronous activity

That was my original idea but @tlhunter suggested we try with just done. Personally I'm 65/35 on doing it with async the way it currently is now. What do you both think?

(Now, after I updated the PR to use async (again), albeit slightly simpler)

I'm inclined on the async way, it's much cleaner, and there's IMO less of a chance for a race condition

test/parallel/test-eventtarget-once-twice.js

codecov · 2024-11-08T17:37:14Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.54%. Comparing base (8e5c80d) to head (257e4f6).
⚠️ Report is 352 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #55752   +/-   ##
=======================================
  Coverage   88.54%   88.54%           
=======================================
  Files         704      704           
  Lines      208103   208103           
  Branches    40089    40084    -5     
=======================================
+ Hits       184256   184268   +12     
+ Misses      15876    15873    -3     
+ Partials     7971     7962    -9

see 28 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

pmarchini · 2024-11-08T18:12:34Z

hey @alexweej, there's a lint issue:

You can fix it running make lint-js-fix 😁

alexweej · 2024-11-11T15:51:35Z

hey @alexweej, there's a lint issue: You can fix it running make lint-js-fix 😁

make lint-js-fix was burning a codespaces CPU for 60s before I killed it. I'll add the trailing comma manually to get this merged...

nodejs-github-bot · 2024-11-12T13:44:43Z

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

nodejs-github-bot · 2024-11-15T15:01:35Z

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

nodejs-github-bot · 2024-11-15T22:45:45Z

Commit Queue failed

- Loading data for nodejs/node/pull/55752
✔  Done loading data for nodejs/node/pull/55752
----------------------------------- PR info ------------------------------------
Title      test: use test runner in eventtarget-once-twice test (#55752)
Author     Alexander “weej” Jones <alex@weej.com> (@alexweej, first-time contributor)
Branch     alexweej:sdghjfsjghafjhkfs -> nodejs:main
Labels     test, code-and-learn, needs-ci, commit-queue-squash
Commits    6
 - test: use test runner in eventtarget-once-twice test
 - Merge remote-tracking branch 'upstream/main' into sdghjfsjghafjhkfs
 - Add somewhat redundant `mustCall`s. Open to suggestions on how to app…
 - Async (again)
 - Tidy up imports
 - comma
Committers 1
 - Alexander “weej” Jones <alex@weej.com>
PR-URL: https://github.com/nodejs/node/pull/55752
Reviewed-By: Pietro Marchini <pietro.marchini94@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/55752
Reviewed-By: Pietro Marchini <pietro.marchini94@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Wed, 06 Nov 2024 16:09:12 GMT
   ✔  Approvals: 2
   ✔  - Pietro Marchini (@pmarchini): https://github.com/nodejs/node/pull/55752#pullrequestreview-2424149511
   ✔  - Stephen Belanger (@Qard): https://github.com/nodejs/node/pull/55752#pullrequestreview-2438826213
   ℹ  This PR is being fast-tracked because it is from a Code and Learn event
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-11-15T15:01:35Z: https://ci.nodejs.org/job/node-test-pull-request/63555/
- Querying data for job/node-test-pull-request/63555/
   ✔  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 55752
From https://github.com/nodejs/node
 * branch                  refs/pull/55752/merge -> FETCH_HEAD
✔  Fetched commits as def4c2870aa0..a1009cb0a71e
--------------------------------------------------------------------------------
error: commit 672dbbdf104f2af6093e187f0a1bf1bfae82aeda is a merge but no -m option was given.
fatal: cherry-pick failed
[main cb6b4fb92f] test: use test runner in eventtarget-once-twice test
 Author: Alexander “weej” Jones <alex@weej.com>
 Date: Wed Nov 6 16:02:43 2024 +0000
 1 file changed, 22 insertions(+), 12 deletions(-)
   ✘  Failed to apply patches

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

alexweej · 2024-11-18T16:55:41Z

@Qard hey man! From the commit queue log it seems like it's upset with merge commits being used as a way to sync this up. I'll try a rebase.

cjihrig · 2024-11-18T16:56:08Z

Merge remote-tracking branch 'upstream/main' into sdghjfsjghafjhkfs

Just FYI - I believe the merge commits here are the reason that the automation cannot land the PR.

alexweej · 2024-11-18T17:01:23Z

Merge remote-tracking branch 'upstream/main' into sdghjfsjghafjhkfs

Just FYI - I believe the merge commits here are the reason that the automation cannot land the PR.

Thanks. I finally read https://github.com/nodejs/node/blob/main/doc/contributing/pull-requests.md#step-5-rebase ...

JakobJingleheimer · 2024-11-20T08:02:58Z

Yes, merge is quite a pitfall on node. @targos has a clever git command to fix it; I'll see if I can dig it up.

nodejs-github-bot · 2025-04-01T14:05:26Z

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

JakobJingleheimer · 2025-04-01T14:51:39Z

If you still need it (from Targos):

My trick is: git reset upstream/main (soft reset), then git add and recreate the commit

Signed-off-by: Alexander “weej” Jones <alex@weej.com>

…ease the linter...

nodejs-github-bot · 2026-01-03T20:40:18Z

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

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

pmarchini reviewed Nov 6, 2024

View reviewed changes

test/parallel/test-eventtarget-once-twice.js Outdated Show resolved Hide resolved

pmarchini added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Nov 6, 2024

avivkeller reviewed Nov 6, 2024

View reviewed changes

avivkeller added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Nov 7, 2024

avivkeller reviewed Nov 7, 2024

View reviewed changes

test/parallel/test-eventtarget-once-twice.js Outdated Show resolved Hide resolved

pmarchini approved these changes Nov 8, 2024

View reviewed changes

pmarchini added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 12, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 12, 2024

Qard approved these changes Nov 15, 2024

View reviewed changes

Qard added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 15, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 15, 2024

cjihrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 15, 2024

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 Nov 15, 2024

alexweej force-pushed the sdghjfsjghafjhkfs branch from c31e7e5 to e70ec63 Compare November 18, 2024 16:57

Qard added request-ci Add this label to start a Jenkins CI on a PR. and removed needs-ci PRs that need a full CI run. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Mar 26, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 1, 2025

alexweej added 5 commits November 8, 2025 15:24

test: use test runner in eventtarget-once-twice test

6cda862

Signed-off-by: Alexander “weej” Jones <alex@weej.com>

Add somewhat redundant mustCalls. Open to suggestions on how to app…

037f876

…ease the linter...

Async (again)

b549e10

Tidy up imports

531a641

comma

257e4f6

aduh95 force-pushed the sdghjfsjghafjhkfs branch from e70ec63 to 257e4f6 Compare November 8, 2025 14:25

aduh95 added the needs-ci PRs that need a full CI run. label Nov 8, 2025

avivkeller 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 Jan 3, 2026

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 3, 2026

Uh oh!

Conversation

alexweej commented Nov 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

avivkeller Nov 6, 2024

Choose a reason for hiding this comment

Uh oh!

alexweej Nov 7, 2024

Choose a reason for hiding this comment

Uh oh!

alexweej Nov 7, 2024

Choose a reason for hiding this comment

Uh oh!

avivkeller Nov 7, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pmarchini commented Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexweej commented Nov 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Nov 12, 2024

Uh oh!

nodejs-github-bot commented Nov 15, 2024

Uh oh!

nodejs-github-bot commented Nov 15, 2024

Uh oh!

alexweej commented Nov 18, 2024

Uh oh!

cjihrig commented Nov 18, 2024

Uh oh!

alexweej commented Nov 18, 2024

Uh oh!

JakobJingleheimer commented Nov 20, 2024

Uh oh!

nodejs-github-bot commented Apr 1, 2025

Uh oh!

JakobJingleheimer commented Apr 1, 2025

Uh oh!

nodejs-github-bot commented Jan 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

alexweej commented Nov 6, 2024 •

edited

Loading

codecov bot commented Nov 8, 2024 •

edited

Loading

pmarchini commented Nov 8, 2024 •

edited

Loading

alexweej commented Nov 11, 2024 •

edited

Loading