Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

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

wants to merge 5 commits into from
+22 −12

Conversation

alexweej
Copy link

@alexweej alexweej commented Nov 6, 2024

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

@nodejs-github-bot 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 pmarchini added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Nov 6, 2024
RedYetiDev
RedYetiDev reviewed Nov 6, 2024
View reviewed changes
test/parallel/test-eventtarget-once-twice.js Outdated
await once(et, 'foo');
await once(et, 'foo');
})().then(common.mustCall());
test('should resolve `once` twice', (t, done) => {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

@RedYetiDev RedYetiDev 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
@codecov Codecov
Copy link

codecov bot commented Nov 8, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.40%. Comparing base (1aa7135) to head (a1009cb).
Report is 48 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #55752   +/-   ##
=======================================
  Coverage   88.40%   88.40%           
=======================================
  Files         654      654           
  Lines      187747   187815   +68     
  Branches    36127    36137   +10     
=======================================
+ Hits       165972   166039   +67     
+ Misses      15009    15008    -1     
- Partials     6766     6768    +2

see 46 files with indirect coverage changes

@pmarchini
Copy link
Member

pmarchini commented Nov 8, 2024 •
edited
Loading

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

@alexweej
Copy link
Author

alexweej commented Nov 11, 2024 •
edited
Loading

hey @alexweej, there's a lint issue: image 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...

@pmarchini pmarchini added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 12, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 12, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@Qard Qard added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 15, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 15, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@cjihrig cjihrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 15, 2024
@nodejs-github-bot 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
@nodejs-github-bot
Copy link
Collaborator

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
Copy link
Author

@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
Copy link
Contributor

cjihrig commented Nov 18, 2024

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
Copy link
Author

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 ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RedYetiDev RedYetiDev RedYetiDev left review comments

@pmarchini pmarchini pmarchini approved these changes

@Qard Qard Qard approved these changes

Assignees
No one assigned
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. commit-queue-failed An error occurred while landing this 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. 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.
6 participants
@alexweej @pmarchini @nodejs-github-bot @cjihrig @Qard @RedYetiDev