GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test_runner: support test plans by marco-ippolito · Pull Request #52860 · nodejs/node

Conversation

@marco-ippolito

@marco-ippolito marco-ippolito commented May 6, 2024

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

simoneb, hackpirodev, hiimjako, styfle, and treblasft reacted with thumbs up emoji MoLow and tonivj5 reacted with confused emoji
All reactions
  • 👍 5 reactions
  • 😕 2 reactions
@nodejs-github-bot

nodejs-github-bot commented May 6, 2024

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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

avivkeller commented May 6, 2024

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@marco-ippolito
marco-ippolito force-pushed the feat/test-plan branch from 61be2da to 8bd4c30 Compare May 6, 2024 16:19
climba03003
climba03003 reviewed May 6, 2024
View reviewed changes
Comment thread test/fixtures/test-runner/output/test-runner-plan.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

VoltrexKeyva
VoltrexKeyva reviewed May 6, 2024
View reviewed changes
Comment thread doc/api/test.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

avivkeller
avivkeller reviewed May 6, 2024
View reviewed changes
Comment thread doc/api/test.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

@cjihrig @marco-ippolito
test_runner: support test plans
01decff
Co-Authored-By: Marco Ippolito <[email protected]>
@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
MoLow
MoLow reviewed May 7, 2024
View reviewed changes
Comment thread lib/internal/test_runner/test.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

@marco-ippolito
marco-ippolito force-pushed the feat/test-plan branch from 80e4079 to c875b18 Compare May 7, 2024 09:03
MoLow
MoLow reviewed May 7, 2024
View reviewed changes

@MoLow MoLow left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@marco-ippolito

marco-ippolito commented May 7, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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

  • tap https://node-tap.org/basics/#planned-assertion-counts
  • jest https://jestjs.io/docs/expect#expectassertionsnumber
  • vitest https://vitest.dev/api/expect.html#expect-assertions
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@MoLow

MoLow commented May 7, 2024

Copy link
Copy Markdown
Member

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

tonivj5 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@simoneb

simoneb commented May 7, 2024

Copy link
Copy Markdown
Contributor

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

mcollina commented May 7, 2024

Copy link
Copy Markdown
Member

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mcollina
mcollina reviewed May 7, 2024
View reviewed changes
Comment thread doc/api/test.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

@marco-ippolito
marco-ippolito force-pushed the feat/test-plan branch from df0e137 to 54dcadf Compare May 7, 2024 10:23
mcollina
mcollina reviewed May 7, 2024
View reviewed changes
Comment thread test/fixtures/test-runner/output/test-runner-plan.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

@mcollina

mcollina commented May 7, 2024

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@marco-ippolito
marco-ippolito force-pushed the feat/test-plan branch from 54dcadf to 7a6f376 Compare May 7, 2024 11:25
mcollina
mcollina reviewed May 7, 2024
View reviewed changes
Comment thread doc/api/test.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

mcollina
mcollina reviewed May 7, 2024
View reviewed changes
Comment thread test/fixtures/test-runner/output/test-runner-plan.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

@cjihrig cjihrig mentioned this pull request May 18, 2024
test_runner: fix t.assert methods #53049
Merged
nodejs-github-bot pushed a commit that referenced this pull request May 20, 2024
@cjihrig
test_runner: fix t.assert methods
f9a0913
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 <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request May 21, 2024
@cjihrig @targos
test_runner: fix t.assert methods
3ff174f
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 <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
deokjinkim added a commit to deokjinkim/node that referenced this pull request May 24, 2024
@deokjinkim
doc: fix wrong function name in example of context.plan()
8975fef
t.subtest -> t.test

Refs: nodejs#52860
@deokjinkim deokjinkim mentioned this pull request May 24, 2024
doc: fix wrong function name in example of context.plan() #53140
Merged
nodejs-github-bot pushed a commit that referenced this pull request May 26, 2024
@deokjinkim
doc: fix wrong function name in example of context.plan()
892d360
t.subtest -> t.test

Refs: #52860
PR-URL: #53140
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
@cjihrig cjihrig mentioned this pull request May 27, 2024
test_runner: add snapshot testing #53169
Merged
cjihrig added a commit to cjihrig/node that referenced this pull request May 30, 2024
@cjihrig
doc: add context.assert docs
851dcdd
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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
targos pushed a commit that referenced this pull request Jun 1, 2024
@deokjinkim @targos
doc: fix wrong function name in example of context.plan()
7147c1d
t.subtest -> t.test

Refs: #52860
PR-URL: #53140
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
targos pushed a commit that referenced this pull request Jun 1, 2024
@cjihrig @targos
doc: add context.assert docs
048478d
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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
@marco-ippolito marco-ippolito mentioned this pull request Jun 17, 2024
v20.15.0 proposal #53486
Merged
marco-ippolito added a commit that referenced this pull request Jun 17, 2024
@cjihrig @marco-ippolito
test_runner: support test plans
044bcce
Co-Authored-By: Marco Ippolito <[email protected]>
PR-URL: #52860
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
marco-ippolito added a commit that referenced this pull request Jun 17, 2024
@marco-ippolito
2024-06-20, Version 20.15.0 'Iron' (LTS)
7d9ad19
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 19, 2024
@cjihrig @marco-ippolito
test_runner: support test plans
d54aa47
Co-Authored-By: Marco Ippolito <[email protected]>
PR-URL: #52860
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
marco-ippolito added a commit that referenced this pull request Jun 19, 2024
@marco-ippolito
2024-06-20, Version 20.15.0 'Iron' (LTS)
e09cd5e
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
@marco-ippolito
2024-06-20, Version 20.15.0 'Iron' (LTS)
0bf200b
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
@marco-ippolito
2024-06-20, Version 20.15.0 'Iron' (LTS)
167ef1a
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
@cjihrig cjihrig mentioned this pull request Jul 13, 2024
test_runner: remove plan option from run() #53834
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MoLow MoLow MoLow left review comments
@avivkeller avivkeller avivkeller left review comments
@mcollina mcollina mcollina approved these changes
@ShogunPanda ShogunPanda ShogunPanda approved these changes
+3 more reviewers
@simoneb simoneb simoneb left review comments
@climba03003 climba03003 climba03003 left review comments
@VoltrexKeyva VoltrexKeyva VoltrexKeyva left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue-squash PRs the Commit Queue should land as one squashed commit. 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

11 participants

@marco-ippolito @nodejs-github-bot @avivkeller @MoLow @simoneb @mcollina @ShogunPanda @cjihrig @climba03003 @targos @VoltrexKeyva
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.