test_runner: don't await the same promise for each test #52185
Closed
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
This commit updates a test runner fixture that includes a failing child test. However, the nested test is created using the top level test() function instead t.test(). This commit updates the fixture to use t.test(), while preserving the expected failure. Refs: nodejs#47164
Prior to this commit, each top level test awaited the same global promise for setting up test reporters. This commit updates the logic to only await the promise the first time it is encountered.
|
Review requested:
|
lpinca
approved these changes
Mar 22, 2024
github-actions
bot
removed
the
request-ci
cjihrig
added
author ready
atlowChemi
approved these changes
Mar 24, 2024
Comment on lines
+228
to
+232
| if (reportersSetup) { | ||
| // Only incur the overhead of awaiting the Promise once. | ||
| await reportersSetup; | ||
| reportersSetup = undefined; | ||
| } |
There was a problem hiding this comment.
Do you happen to have any benchmarking for this change?
Did anything specific raise this change?
There was a problem hiding this comment.
@cjihrig I am curious as well what impact this has. is it that expensive to await the same promise multiple times?
There was a problem hiding this comment.
@MoLow see my comment in #52185 (comment). The issue is just that even after the promise is settled, awaiting it costs another tick. Quoting MDN:
Even when the used promise is already fulfilled, the async function's execution still pauses until the next tick.
It just seemed like a simple way to eliminate some overhead.
atlowChemi
added
the
commit-queue-rebase
cjihrig
added
the
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in bae14b7...4648c83 |
nodejs-github-bot
pushed a commit
that referenced
this pull request
Mar 25, 2024
This commit updates a test runner fixture that includes a failing child test. However, the nested test is created using the top level test() function instead t.test(). This commit updates the fixture to use t.test(), while preserving the expected failure. Refs: #47164 PR-URL: #52185 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
nodejs-github-bot
pushed a commit
that referenced
this pull request
Mar 25, 2024
Prior to this commit, each top level test awaited the same global promise for setting up test reporters. This commit updates the logic to only await the promise the first time it is encountered. PR-URL: #52185 Refs: #47164 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
anonrig
pushed a commit
to anonrig/node
that referenced
this pull request
Mar 25, 2024
This commit updates a test runner fixture that includes a failing child test. However, the nested test is created using the top level test() function instead t.test(). This commit updates the fixture to use t.test(), while preserving the expected failure. Refs: nodejs#47164 PR-URL: nodejs#52185 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
anonrig
pushed a commit
to anonrig/node
that referenced
this pull request
Mar 25, 2024
Prior to this commit, each top level test awaited the same global promise for setting up test reporters. This commit updates the logic to only await the promise the first time it is encountered. PR-URL: nodejs#52185 Refs: nodejs#47164 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
rdw-msft
pushed a commit
to rdw-msft/node
that referenced
this pull request
Mar 26, 2024
This commit updates a test runner fixture that includes a failing child test. However, the nested test is created using the top level test() function instead t.test(). This commit updates the fixture to use t.test(), while preserving the expected failure. Refs: nodejs#47164 PR-URL: nodejs#52185 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
rdw-msft
pushed a commit
to rdw-msft/node
that referenced
this pull request
Mar 26, 2024
Prior to this commit, each top level test awaited the same global promise for setting up test reporters. This commit updates the logic to only await the promise the first time it is encountered. PR-URL: nodejs#52185 Refs: nodejs#47164 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
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.
test: fix incorrect test fixture
This commit updates a test runner fixture that includes a failing
child test. However, the nested test is created using the top
level
test()function insteadt.test(). This commit updates thefixture to use
t.test(), while preserving the expected failure.Refs: #47164
test_runner: don't await the same promise for each test
Prior to this commit, each top level test awaited the same
global promise for setting up test reporters. This commit
updates the logic to only await the promise the first time
it is encountered.