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: support test plans #52860

Merged
merged 2 commits into from
May 9, 2024

Conversation

marco-ippolito
Copy link
Member

I picked up the work from this comment #44125 (comment) by @cjihrig to add support for test planning

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels May 6, 2024
@avivkeller
Copy link
Member

IMO the t.plan() should also be settable via the test options

doc/api/test.md Outdated Show resolved Hide resolved
doc/api/test.md Outdated Show resolved Hide resolved
Co-Authored-By: Marco Ippolito <marcoippolito54@gmail.com>
@marco-ippolito marco-ippolito force-pushed the feat/test-plan branch 6 times, most recently from 607b2fb to 80e4079 Compare May 6, 2024 20:15
Copy link
Member

@MoLow MoLow left a comment

Choose a reason for hiding this comment

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

Can you please elaborate on the use cases of this feature?
We already error if a test never ends and the event loop is drained - in what scenario should I worry not all assertions will be checked?
and what makes assertions different than any other piece of code inside the test?

@marco-ippolito
Copy link
Member Author

marco-ippolito commented May 7, 2024

Can you please elaborate on the use cases of this feature? We already error if a test never ends and the event loop is drained - in what scenario should I worry not all assertions will be checked? and what makes assertions different than any other piece of code inside the test?

It makes sure that an assertion is called inside a callback, when the callback might not be invoked.
This feature is actually common in a lot of testing libraries such as

test('make sure callback is invoked', (t) => {
 t.plan(1)
  server.listen(0, (r) => {
   // assert something in here
   t.ok(true)
  });
});

With plan you are sure than the callback was invoked and the assertion was performed, otherwise the test would pass anyways.

@MoLow
Copy link
Member

MoLow commented May 7, 2024

I am -0.5 on adding this.
if one wants to assert his code run they can use mock.fn/mock.method and assert it was actually called. I don't think assertions are very different in that aspect

@simoneb
Copy link
Contributor

simoneb commented May 7, 2024

Hey folks, Marco is working with me and I suggested that he works on this feature that is missing from the Node.js test runner and it's necessary in certain specific cases.

The use case is fairly specific and it pertains callback-based APIs. When the callback is called and you need to make assertions there, there is no easy way to do that except telling the test that it should not end before X number of assertions have been made (bar a test timeout).

Let's take this example from the Fastify docs:

const tap = require('tap')
const buildFastify = require('./app')

tap.test('GET `/` route', t => {
  t.plan(4)

  const fastify = buildFastify()

  // At the end of your tests it is highly recommended to call `.close()`
  // to ensure that all connections to external services get closed.
  t.teardown(() => fastify.close())

  fastify.inject({
    method: 'GET',
    url: '/'
  }, (err, response) => {
    t.error(err)
    t.equal(response.statusCode, 200)
    t.equal(response.headers['content-type'], 'application/json; charset=utf-8')
    t.same(response.json(), { hello: 'world' })
  })
})

Let's forget for a second that fastify.inject also has a promise API. Without t.plan(4) you have no easy way to make sure that the assertions in the callback have been invoked. E.g. if inject due to a bug didn't invoke the callback, this test would pass just fine if you didn't have a way to tell the test that you're expecting 4 assertions.

I hope this clarifies why this feature is necessary.

@marco-ippolito marco-ippolito requested a review from mcollina May 7, 2024 09:29
@mcollina
Copy link
Member

mcollina commented May 7, 2024

I think this feature is needed.

@marco-ippolito can you add a test that verifies what happens if there were more assertion than the plan?

@mcollina
Copy link
Member

mcollina commented May 7, 2024

This is missing a way for other assertion frameworks to tap into the test planning. If I recall correctly, the need for this was what stopped @cjihrig working on this feature.

doc/api/test.md Outdated Show resolved Hide resolved
marco-ippolito added a commit that referenced this pull request Jun 19, 2024
Notable changes:

doc:
  * add pimterry to collaborators (Tim Perry) #52874
inspector:
  * (SEMVER-MINOR) introduce the `--inspect-wait` flag (Kohei Ueno) #52734
test_runner:
  * (SEMVER-MINOR) support test plans (Colin Ihrig) #52860
tools:
  * (SEMVER-MINOR) fix get_asan_state() in tools/test.py (Joyee Cheung) #52766
  * (SEMVER-MINOR) support max_virtual_memory test configuration (Joyee Cheung) #52766
  * (SEMVER-MINOR) support != in test status files (Joyee Cheung) #52766
zlib:
  * (SEMVER-MINOR) expose zlib.crc32() (Joyee Cheung) #52692

PR-URL: #53486
marco-ippolito added a commit that referenced this pull request Jun 20, 2024
Notable changes:

doc:
  * add pimterry to collaborators (Tim Perry) #52874
inspector:
  * (SEMVER-MINOR) introduce the `--inspect-wait` flag (Kohei Ueno) #52734
test_runner:
  * (SEMVER-MINOR) support test plans (Colin Ihrig) #52860
tools:
  * (SEMVER-MINOR) fix get_asan_state() in tools/test.py (Joyee Cheung) #52766
  * (SEMVER-MINOR) support max_virtual_memory test configuration (Joyee Cheung) #52766
  * (SEMVER-MINOR) support != in test status files (Joyee Cheung) #52766
zlib:
  * (SEMVER-MINOR) expose zlib.crc32() (Joyee Cheung) #52692

PR-URL: #53486
marco-ippolito added a commit that referenced this pull request Jun 20, 2024
Notable changes:

doc:
  * add pimterry to collaborators (Tim Perry) #52874
inspector:
  * (SEMVER-MINOR) introduce the `--inspect-wait` flag (Kohei Ueno) #52734
test_runner:
  * (SEMVER-MINOR) support test plans (Colin Ihrig) #52860
tools:
  * (SEMVER-MINOR) fix get_asan_state() in tools/test.py (Joyee Cheung) #52766
  * (SEMVER-MINOR) support max_virtual_memory test configuration (Joyee Cheung) #52766
  * (SEMVER-MINOR) support != in test status files (Joyee Cheung) #52766
zlib:
  * (SEMVER-MINOR) expose zlib.crc32() (Joyee Cheung) #52692

PR-URL: #53486
sophoniie pushed a commit to sophoniie/node that referenced this pull request Jun 20, 2024
Co-Authored-By: Marco Ippolito <marcoippolito54@gmail.com>
PR-URL: nodejs#52860
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
sophoniie pushed a commit to sophoniie/node that referenced this pull request Jun 20, 2024
Notable changes:

cli:
  * (SEMVER-MINOR) allow running wasm in limited vmem with
    --disable-wasm-trap-handler (Joyee Cheung)
    nodejs#52766
doc:
  * add pimterry to collaborators (Tim Perry)
    nodejs#52874
fs:
  * (SEMVER-MINOR) allow 'withFileTypes' to be used with globs
    (Aviv Keller) nodejs#52837
inspector:
  * (SEMVER-MINOR) introduce the `--inspect-wait` flag (Kohei Ueno)
    nodejs#52734
lib,src:
  * remove --experimental-policy (Rafael Gonzaga)
    nodejs#52583
perf_hooks:
  * (SEMVER-MINOR) add `deliveryType` and `responseStatus` fields
    (Matthew Aitken) nodejs#51589
test_runner:
  * (SEMVER-MINOR) support test plans (Colin Ihrig)
    nodejs#52860
zlib:
  * (SEMVER-MINOR) expose zlib.crc32() (Joyee Cheung)
    nodejs#52692

PR-URL: nodejs#52971
sophoniie pushed a commit to sophoniie/node that referenced this pull request Jun 20, 2024
The node:assert module contains several top level APIs that do
not make sense to expose as methods on t.assert. Examples include
AssertionError and CallTracker. This commit removes such APIs from
t.assert.

Refs: nodejs#52860
PR-URL: nodejs#53049
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
sophoniie pushed a commit to sophoniie/node that referenced this pull request Jun 20, 2024
t.subtest -> t.test

Refs: nodejs#52860
PR-URL: nodejs#53140
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
sophoniie pushed a commit to sophoniie/node that referenced this pull request Jun 20, 2024
When context.assert was added, no docs were added. This commit
adds initial documentation for context.assert because the
snapshot() function requires them.

PR-URL: nodejs#53169
Refs: nodejs#52860
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
sophoniie pushed a commit to sophoniie/node that referenced this pull request Jun 20, 2024
Notable changes:

doc:
  * add pimterry to collaborators (Tim Perry) nodejs#52874
inspector:
  * (SEMVER-MINOR) introduce the `--inspect-wait` flag (Kohei Ueno) nodejs#52734
test_runner:
  * (SEMVER-MINOR) support test plans (Colin Ihrig) nodejs#52860
tools:
  * (SEMVER-MINOR) fix get_asan_state() in tools/test.py (Joyee Cheung) nodejs#52766
  * (SEMVER-MINOR) support max_virtual_memory test configuration (Joyee Cheung) nodejs#52766
  * (SEMVER-MINOR) support != in test status files (Joyee Cheung) nodejs#52766
zlib:
  * (SEMVER-MINOR) expose zlib.crc32() (Joyee Cheung) nodejs#52692

PR-URL: nodejs#53486
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
Co-Authored-By: Marco Ippolito <marcoippolito54@gmail.com>
PR-URL: nodejs#52860
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
Notable changes:

cli:
  * (SEMVER-MINOR) allow running wasm in limited vmem with
    --disable-wasm-trap-handler (Joyee Cheung)
    nodejs#52766
doc:
  * add pimterry to collaborators (Tim Perry)
    nodejs#52874
fs:
  * (SEMVER-MINOR) allow 'withFileTypes' to be used with globs
    (Aviv Keller) nodejs#52837
inspector:
  * (SEMVER-MINOR) introduce the `--inspect-wait` flag (Kohei Ueno)
    nodejs#52734
lib,src:
  * remove --experimental-policy (Rafael Gonzaga)
    nodejs#52583
perf_hooks:
  * (SEMVER-MINOR) add `deliveryType` and `responseStatus` fields
    (Matthew Aitken) nodejs#51589
test_runner:
  * (SEMVER-MINOR) support test plans (Colin Ihrig)
    nodejs#52860
zlib:
  * (SEMVER-MINOR) expose zlib.crc32() (Joyee Cheung)
    nodejs#52692

PR-URL: nodejs#52971
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
The node:assert module contains several top level APIs that do
not make sense to expose as methods on t.assert. Examples include
AssertionError and CallTracker. This commit removes such APIs from
t.assert.

Refs: nodejs#52860
PR-URL: nodejs#53049
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
t.subtest -> t.test

Refs: nodejs#52860
PR-URL: nodejs#53140
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
When context.assert was added, no docs were added. This commit
adds initial documentation for context.assert because the
snapshot() function requires them.

PR-URL: nodejs#53169
Refs: nodejs#52860
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
Notable changes:

doc:
  * add pimterry to collaborators (Tim Perry) nodejs#52874
inspector:
  * (SEMVER-MINOR) introduce the `--inspect-wait` flag (Kohei Ueno) nodejs#52734
test_runner:
  * (SEMVER-MINOR) support test plans (Colin Ihrig) nodejs#52860
tools:
  * (SEMVER-MINOR) fix get_asan_state() in tools/test.py (Joyee Cheung) nodejs#52766
  * (SEMVER-MINOR) support max_virtual_memory test configuration (Joyee Cheung) nodejs#52766
  * (SEMVER-MINOR) support != in test status files (Joyee Cheung) nodejs#52766
zlib:
  * (SEMVER-MINOR) expose zlib.crc32() (Joyee Cheung) nodejs#52692

PR-URL: nodejs#53486
cjihrig added a commit to cjihrig/node that referenced this pull request Jul 13, 2024
This commit removes the plan option to run(). I believe it was
added by mistake. It is not documented, untested, and a test
plan does not make sense in the context of run().

This commit also fixes a minor formatting issue in a related
fixture.

Refs: nodejs#52860
nodejs-github-bot pushed a commit that referenced this pull request Jul 15, 2024
This commit removes the plan option to run(). I believe it was
added by mistake. It is not documented, untested, and a test
plan does not make sense in the context of run().

This commit also fixes a minor formatting issue in a related
fixture.

Refs: #52860
PR-URL: #53834
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
aduh95 pushed a commit that referenced this pull request Jul 16, 2024
This commit removes the plan option to run(). I believe it was
added by mistake. It is not documented, untested, and a test
plan does not make sense in the context of run().

This commit also fixes a minor formatting issue in a related
fixture.

Refs: #52860
PR-URL: #53834
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
aduh95 pushed a commit that referenced this pull request Jul 16, 2024
This commit removes the plan option to run(). I believe it was
added by mistake. It is not documented, untested, and a test
plan does not make sense in the context of run().

This commit also fixes a minor formatting issue in a related
fixture.

Refs: #52860
PR-URL: #53834
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
ehsankhfr pushed a commit to ehsankhfr/node that referenced this pull request Jul 18, 2024
This commit removes the plan option to run(). I believe it was
added by mistake. It is not documented, untested, and a test
plan does not make sense in the context of run().

This commit also fixes a minor formatting issue in a related
fixture.

Refs: nodejs#52860
PR-URL: nodejs#53834
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
marco-ippolito pushed a commit that referenced this pull request Jul 19, 2024
The node:assert module contains several top level APIs that do
not make sense to expose as methods on t.assert. Examples include
AssertionError and CallTracker. This commit removes such APIs from
t.assert.

Refs: #52860
PR-URL: #53049
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
marco-ippolito pushed a commit that referenced this pull request Jul 19, 2024
t.subtest -> t.test

Refs: #52860
PR-URL: #53140
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
marco-ippolito pushed a commit that referenced this pull request Jul 19, 2024
When context.assert was added, no docs were added. This commit
adds initial documentation for context.assert because the
snapshot() function requires them.

PR-URL: #53169
Refs: #52860
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
marco-ippolito pushed a commit that referenced this pull request Jul 19, 2024
The node:assert module contains several top level APIs that do
not make sense to expose as methods on t.assert. Examples include
AssertionError and CallTracker. This commit removes such APIs from
t.assert.

Refs: #52860
PR-URL: #53049
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
marco-ippolito pushed a commit that referenced this pull request Jul 19, 2024
t.subtest -> t.test

Refs: #52860
PR-URL: #53140
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
marco-ippolito pushed a commit that referenced this pull request Jul 19, 2024
When context.assert was added, no docs were added. This commit
adds initial documentation for context.assert because the
snapshot() function requires them.

PR-URL: #53169
Refs: #52860
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
marco-ippolito pushed a commit that referenced this pull request Aug 19, 2024
This commit removes the plan option to run(). I believe it was
added by mistake. It is not documented, untested, and a test
plan does not make sense in the context of run().

This commit also fixes a minor formatting issue in a related
fixture.

Refs: #52860
PR-URL: #53834
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
marco-ippolito pushed a commit that referenced this pull request Aug 19, 2024
This commit removes the plan option to run(). I believe it was
added by mistake. It is not documented, untested, and a test
plan does not make sense in the context of run().

This commit also fixes a minor formatting issue in a related
fixture.

Refs: #52860
PR-URL: #53834
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
marco-ippolito pushed a commit that referenced this pull request Aug 19, 2024
This commit removes the plan option to run(). I believe it was
added by mistake. It is not documented, untested, and a test
plan does not make sense in the context of run().

This commit also fixes a minor formatting issue in a related
fixture.

Refs: #52860
PR-URL: #53834
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
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. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.