test: use test runner in eventtarget-once-twice test #55752
+22
−12
Conversation
This file contains 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
Signed-off-by: Alexander “weej” Jones <[email protected]>
pmarchini
reviewed
Nov 6, 2024
…ease the linter...
RedYetiDev
reviewed
Nov 6, 2024
| 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
reviewed
Nov 7, 2024
pmarchini
approved these changes
Nov 8, 2024
Codecov ReportAll modified and coverable lines are covered by tests ✅
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 |
|
hey @alexweej, there's a lint issue: |
|
Qard
approved these changes
Nov 15, 2024
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 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Alexander “weej” Jones [email protected]