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
RedYetiDev
added
the
commit-queue-squash
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #55752 +/- ##
=======================================
Coverage 88.49% 88.50%
=======================================
Files 653 653
Lines 187735 187735
Branches 36181 36180 -1
=======================================
+ Hits 166141 166147 +6
- Misses 14819 14822 +3
+ Partials 6775 6766 -9
|
|
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 <[email protected]> (@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 <[email protected]> PR-URL: https://github.com/nodejs/node/pull/55752 Reviewed-By: Pietro Marchini <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/55752 Reviewed-By: Pietro Marchini <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> -------------------------------------------------------------------------------- ℹ 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 <[email protected]> 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. |
Signed-off-by: Alexander “weej” Jones <[email protected]>
…ease the linter...
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. |
Signed-off-by: Alexander “weej” Jones [email protected]