-
Notifications
You must be signed in to change notification settings - Fork 30k
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: add describe.only
and it.only
shorthands
#46604
test_runner: add describe.only
and it.only
shorthands
#46604
Conversation
Review requested:
|
@@ -808,6 +808,11 @@ Shorthand for skipping a suite, same as [`describe([name], { skip: true }[, fn]) | |||
Shorthand for marking a suite as `TODO`, same as | |||
[`describe([name], { todo: true }[, fn])`][describe options]. | |||
|
|||
## `describe.only([name][, options][, fn])` |
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.
If we do this, we should also support test.only
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.
test
does not currently support test.skip()
or test.todo()
, and I would prefer to keep it that way.
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.
I think there is definitely value in having shorthands for the test
API - especially skip
and only
. However, for the scope of this commit, it's only about adding the extra shorthand to the describe/it
API. This keeps the options consistent.
@cjihrig are there any issues in particular you see with adding a shorthand API for the test interface in a future PR?
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.
There is no technical reason - just a desire to keep the API surface area smaller. The Test
class in particular is important because everything else builds on top of it. And, it already supports skip, todo, and only functionality, so I would prefer to not implement another way of doing the same thing.
9f9585a
to
e89d473
Compare
@richiemccoll is this ready for review? |
e89d473
to
e32726d
Compare
@MoLow Yes I was just checking the tests locally this morning before marking it for review. It should be ready now. |
@richiemccoll can you amend the commit message to adhere to our guidelines? The first word should be an imperative word, which node/doc/contributing/pull-requests.md Lines 168 to 169 in 5af2021
May I suggest |
e32726d
to
6c6a7dd
Compare
@aduh95 Thanks. Commit message updated and pushed the doc metadata changes. |
describe.only
and it.only
shorthands
duration_ms: * | ||
... | ||
# Subtest: subtest should run | ||
ok 14 - subtest should run # SKIP 'only' option not set |
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.
The test title says this should run, but it is skipped.
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.
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.
@richiemccoll #46544 has landed, can you please rebase this PR?
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.
I've rebased after your PR landed and updated these tests which now run. Commit here 6f069ef.
c7c895a
to
73b66f0
Compare
73b66f0
to
6f069ef
Compare
Landed in 0093fd3 |
PR-URL: nodejs#46604 Fixes: nodejs#46562 Reviewed-By: Moshe Atlow <[email protected]>
PR-URL: nodejs#46604 Fixes: nodejs#46562 Reviewed-By: Moshe Atlow <[email protected]>
PR-URL: nodejs#46604 Fixes: nodejs#46562 Reviewed-By: Moshe Atlow <[email protected]>
PR-URL: #46604 Backport-PR-URL: #46839 Fixes: #46562 Reviewed-By: Moshe Atlow <[email protected]>
PR-URL: #46604 Backport-PR-URL: #46839 Fixes: #46562 Reviewed-By: Moshe Atlow <[email protected]>
PR-URL: #46604 Fixes: #46562 Reviewed-By: Moshe Atlow <[email protected]>
Fixes: #46562
This PR adds support for
describe.only
andit.only
shorthands in the test_runner. It follows the existing semantics for howonly
works.