Skip to content

test_runner: don't await the same promise for each test#52185

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

Closed
cjihrig wants to merge 2 commits into from
Closed

test_runner: don't await the same promise for each test #52185

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b7d0808
test: fix incorrect test fixture
cjihrig Mar 22, 2024
ed1821a
test_runner: don't await the same promise for each test
cjihrig Mar 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion lib/internal/test_runner/harness.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -225,7 +225,11 @@ function getGlobalRoot() {
}

async function startSubtest(subtest) {
await reportersSetup;
if (reportersSetup) {
// Only incur the overhead of awaiting the Promise once.
await reportersSetup;
reportersSetup = undefined;
}
Comment on lines +228 to +232

@atlowChemiatlowChemiMar 24, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you happen to have any benchmarking for this change?
Did anything specific raise this change?

@MoLowMoLowMar 26, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@cjihrig I am curious as well what impact this has. is it that expensive to await the same promise multiple times?

@cjihrigcjihrigMar 26, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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


const root = getGlobalRoot();
if (!root.harness.bootstrapComplete) {
Expand Down
8 changes: 5 additions & 3 deletions test/fixtures/test-runner/output/default_output.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -9,7 +9,9 @@ const test = require('node:test');
test('should pass', () => {});
test('should fail', () => { throw new Error('fail'); });
test('should skip', { skip: true }, () => {});
test('parent', () => {
test('should fail', () => { throw new Error('fail'); });
test('should pass but parent fail', () => {});
test('parent', (t) => {
t.test('should fail', () => { throw new Error('fail'); });
t.test('should pass but parent fail', (t, done) => {
Comment thread
atlowChemi marked this conversation as resolved.
setImmediate(done);
});
});
10 changes: 10 additions & 0 deletions test/fixtures/test-runner/output/default_output.snapshot
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -17,6 +17,11 @@
*[39m
*[39m
*[39m
*[39m
*[39m
*[39m
*[39m
*[39m
*[39m

[31m✖ should pass but parent fail [90m(*ms)[39m[39m
Expand DownExpand Up@@ -53,6 +58,11 @@
*[39m
*[39m
*[39m
*[39m
*[39m
*[39m
*[39m
*[39m
*[39m

*
Expand Down