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.

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

test_runner: allow nesting test within describe #46544

Merged
merged 1 commit into from
Feb 17, 2023

Conversation

MoLow
Copy link
Member

@MoLow MoLow commented Feb 7, 2023

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

@nodejs-github-bot nodejs-github-bot added dont-land-on-v14.x needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Feb 7, 2023
@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.

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

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

@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

@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

@nodejs-github-bot
Copy link
Collaborator

@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
MoLow added a commit to MoLow/node that referenced this pull request Feb 18, 2023
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
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
MylesBorins pushed a commit that referenced this pull request Feb 20, 2023
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
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
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
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>
juanarbol pushed a commit that referenced this pull request Mar 3, 2023
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
juanarbol pushed a commit that referenced this pull request Mar 5, 2023
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
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
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 Issues and PRs related to the test runner subsystem.
Projects
None yet
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