Skip to content
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

Jump to bottom

test_runner: allow nesting test within describe #46544

Merged
merged 1 commit into from Feb 17, 2023
Merged

test_runner: allow nesting test within describe #46544

merged 1 commit into from Feb 17, 2023
+76 −62

Conversation

MoLow
Copy link
Member

@MoLow MoLow commented Feb 7, 2023 •
edited

Fixes: #46478

although initially test and describe were not meant to be used together I see no reason why not to allow this,
as the use case demonstrated in #46478 (comment) is valid.
additionally, other test runners such as playwright and mocha also support this.

another outcome of this change is that running a nested test within test will now work the same way t.test did previously, wich sounds reasonable to me as well

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@MoLow MoLow mentioned this pull request Feb 7, 2023
Closed
@piranna
Copy link
Contributor

piranna commented Feb 7, 2023

another outcome of this change is that running a nested test within test will now work the same way t.test did previously, wich sounds reasonable to me as well

Maybe describe() should be merged here too? Or at least in another PR... signature and parameters are almost the same than test(), just only returning undefined instead of a Promise<undefined>, and I find this diference a bit arbitrary, specially since describe()s can be nested (like test()s can be now) and that they can have asynchronous tests, so they should wait too...

@piranna
Copy link
Contributor

piranna commented Feb 7, 2023

specially since describe()s can be nested (like test()s can be now) and that they can have asynchronous tests, so they should wait too...

In fact... for tap output, everything are sub-tests, there's no describes :-P

@MoLow
Copy link
Member Author

MoLow commented Feb 7, 2023

Maybe describe() should be merged here too? Or at least in another PR... signature and parameters are almost the same than test(), just only returning undefined instead of a Promise<undefined>, and I find this diference a bit arbitrary, specially since describe()s can be nested (like test()s can be now) and that they can have asynchronous tests, so they should wait too...

describe builds the test tree before running it, to aviod the need of adding an await before child tests

@piranna
Copy link
Contributor

piranna commented Feb 7, 2023

describe builds the test tree before running it

That... it's a reason, maybe that detail should be clarified in the docs.

cjihrig
cjihrig approved these changes Feb 8, 2023
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM if the CI passes.

@MoLow MoLow added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 8, 2023
benjamingr
benjamingr reviewed Feb 9, 2023
View reviewed changes
lib/internal/test_runner/harness.js
@@ -167,7 +167,8 @@ function getGlobalRoot() {
}

function test(name, options, fn) {
const subtest = getGlobalRoot().createSubtest(Test, name, options, fn);
const parent = testResources.get(executionAsyncId()) || getGlobalRoot();
Copy link
Member

Choose a reason for hiding this comment

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

nit: can't we use ?? here?

benjamingr
benjamingr reviewed Feb 9, 2023
View reviewed changes
test/message/test_runner_describe_it.js
@@ -309,12 +312,12 @@ describe('describe async throw fails', async () => {
describe('timeouts', () => {
it('timed out async test', { timeout: 5 }, async () => {
return new Promise((resolve) => {
setTimeout(resolve, 1000);
setTimeout(resolve, 100);
Copy link
Member

Choose a reason for hiding this comment

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

Should these setTimeouts use common platform timeout machinery or are you convinced there is no/little flakiness potential here?

Copy link
Member Author

Choose a reason for hiding this comment

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

we have already changed these values in the equivalent test recipe test

node/test/message/test_runner_output.js

Line 116 in 5092346

}, 100);

@richiemccoll richiemccoll mentioned this pull request Feb 14, 2023
Merged
@MoLow MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 16, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/49570/

@MoLow MoLow removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 16, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/49609/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/49618/

@MoLow MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 17, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 17, 2023
@nodejs-github-bot nodejs-github-bot merged commit 2787e2d into nodejs:main Feb 17, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 2787e2d

@MoLow MoLow deleted the allow-running-test-in-describe branch February 17, 2023 13:42
@github-actions github-actions bot mentioned this pull request Feb 18, 2023
Open
32 tasks
MoLow added a commit to MoLow/node that referenced this pull request Feb 18, 2023
@MoLow
test_runner: allow nesting test within describe
ed06bf6 
PR-URL: nodejs#46544
Fixes: nodejs#46478
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2023
@MoLow @MylesBorins
test_runner: allow nesting test within describe
e97e796 
PR-URL: #46544
Fixes: #46478
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 19, 2023
Merged
MylesBorins pushed a commit that referenced this pull request Feb 20, 2023
@MoLow @MylesBorins
test_runner: allow nesting test within describe
5b74811 
PR-URL: #46544
Fixes: #46478
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MoLow added a commit to MoLow/node that referenced this pull request Feb 25, 2023
@MoLow
test_runner: allow nesting test within describe
86e847c 
PR-URL: nodejs#46544
Fixes: nodejs#46478
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MoLow added a commit to MoLow/node that referenced this pull request Feb 25, 2023
@MoLow
test_runner: allow nesting test within describe
c13f16a 
PR-URL: nodejs#46544
Fixes: nodejs#46478
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MoLow MoLow added the backport-open-v18.x Indicate that the PR has an open backport. label Feb 25, 2023
MoLow added a commit to MoLow/node that referenced this pull request Feb 25, 2023
@MoLow
test_runner: allow nesting test within describe
1502aca 
PR-URL: nodejs#46544
Fixes: nodejs#46478
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MoLow MoLow mentioned this pull request Feb 28, 2023
Closed
1 task
juanarbol pushed a commit that referenced this pull request Mar 3, 2023
@MoLow @juanarbol
test_runner: allow nesting test within describe
2cf1e7c 
PR-URL: #46544
Backport-PR-URL: #46839
Fixes: #46478
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@juanarbol juanarbol mentioned this pull request Mar 3, 2023
Merged
juanarbol pushed a commit that referenced this pull request Mar 5, 2023
@MoLow @juanarbol
test_runner: allow nesting test within describe
391ff0d 
PR-URL: #46544
Backport-PR-URL: #46839
Fixes: #46478
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
juliangruber pushed a commit to nodejs/node-core-test that referenced this pull request Mar 16, 2023
@MoLow @juliangruber
test_runner: allow nesting test within describe
5cebe91 
PR-URL: nodejs/node#46544
Fixes: nodejs/node#46478
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
(cherry picked from commit 2787e2dfc2e10c584259ff34d7aad565447a84d9)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@benjamingr benjamingr benjamingr approved these changes

@cjihrig cjihrig cjihrig approved these changes

@aduh95 aduh95 aduh95 approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-open-v18.x Indicate that the PR has an open backport. needs-ci PRs that need a full CI run. test_runner
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Built-in test module only report ouput of the first top level describe
7 participants
@MoLow @nodejs-github-bot @piranna @jasnell @benjamingr @cjihrig @aduh95