test: use test runner in eventtarget-once-twice test #55752
Conversation
nodejs-github-bot added needs-ci 
pmarchini added the code-and-learn 
| await once(et, 'foo'); | ||
| await once(et, 'foo'); | ||
| })().then(common.mustCall()); | ||
| test('should resolve `once` twice', (t, done) => { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
(Now, after I updated the PR to use async (again), albeit slightly simpler)
There was a problem hiding this comment.
I'm inclined on the async way, it's much cleaner, and there's IMO less of a chance for a race condition
avivkeller added the commit-queue-squash 
Codecov Report✅ All modified and coverable lines are covered by tests. 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 🚀 New features to boost your workflow:
|
| hey @alexweej, there's a lint issue: |
|
github-actions bot removed the request-ci 
github-actions bot removed the request-ci 
cjihrig added the commit-queue nodejs-github-bot added commit-queue-failed 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 patcheshttps://github.com/nodejs/node/actions/runs/11864552572 |
| @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. |
Just FYI - I believe the merge commits here are the reason that the automation cannot land the PR. |
alexweej force-pushed the sdghjfsjghafjhkfs branch from c31e7e5 to e70ec63 Compare
Thanks. I finally read https://github.com/nodejs/node/blob/main/doc/contributing/pull-requests.md#step-5-rebase ... |
| 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. |
Qard added request-ci github-actions bot removed the request-ci | If you still need it (from Targos):
|
Signed-off-by: Alexander “weej” Jones <alex@weej.com>
…ease the linter...
aduh95 force-pushed the sdghjfsjghafjhkfs branch from e70ec63 to 257e4f6 Compare 8 participants
No description provided.