Conversation
|
|
||
| ## Outcomes | ||
|
|
||
| * General agreement that `t.skip()`'s behaviour is wrong (likely an accident/bug/mistake, which the docs seem to support). |
There was a problem hiding this comment.
I've just checked the rest of the documentation, and the behavior is definitely intended:
https://nodejs.org/docs/v24.13.0/api/test.html#contextskipmessage.
When checking other test runners, like Mocha, this behavior also exists (e.g. https://mochajs.org/declaring/inclusive-tests/#_top).
Given that this behavior is "by design" and aligned with other test runners,IMHO , it's just a matter of personal preference...and I don't think it's enough to justify a major breaking change
P.S.: the same is true for Vitest as well (https://vitest.dev/guide/test-context.html#test-context)
There was a problem hiding this comment.
Ah, interesting. Thanks for checking!
If it's actually fairly common, then sure, it's already there. Kinda weird, but some people like liquorice too 🤷♂️
There was a problem hiding this comment.
We might want to understand how common it is, but I'm not sure that, without a standard, it makes sense to compare different tools...
cc @ljharb
There was a problem hiding this comment.
If these runners all have the API, then that’s a very different story, but it still seems weird to me given the tap-like API the test runner has - which leads into the design philosophy discussion.
|
|
||
| ## Outcomes | ||
|
|
||
| * General agreement that `t.skip()`'s behaviour is wrong (likely an accident/bug/mistake, which the docs seem to support). |
There was a problem hiding this comment.
| * General agreement that `t.skip()`'s behaviour is wrong (likely an accident/bug/mistake, which the docs seem to support). | |
| * ~Initially general agreement that `t.skip()`'s behaviour is wrong (likely an accident/bug/mistake, which the docs seem to support).~ | |
| * Further investigation shows this behaviour | |
| * exists in others (including [mocha](https://mochajs.org/declaring/inclusive-tests/#_top)) | |
| * was intended: https://nodejs.org/docs/v24.13.0/api/test.html#contextskipmessage |
There was a problem hiding this comment.
I can't find it written anywhere what exactly was deemed wrong about t.skip's behavior.
There was a problem hiding this comment.
testCtx.skip skips the test from which it is invoked dynamically, while test.skip creates a skipped test (test with option skip: true)
There was a problem hiding this comment.
Since there actually wasn't consensus, let's update the minutes to just describe the problem (and merge the minutes), and then we can continue the discussion (either here or in a separate issue/discussion)?
There was a problem hiding this comment.
yep - one mental model is "t.skip skips t" and another is "t.skip is a way to generate a skipped assertion underneath t". The latter is how tap and tape work.
There was a problem hiding this comment.
Doesn't t.test.skip create a skipped test underneath t?
There was a problem hiding this comment.
Currently, skip never generates a test anywhere in the API. It changes the state of the test to which it is applied1. If applied to a test context, it changes the state of that test. Consistent, yeah?
Footnotes
-
Conceptually speaking. Technically
skipis a function in thetestnamespace that creates a test, but if you consider the following I think you will get what I mean:
↩test('yada', fn) test('yada', {skip: true}, fn) test.skip('yada', fn)
There was a problem hiding this comment.
Doesn't
t.test.skipcreate a skipped test underneatht?
Nope: TypeError: t.test.skip is not a function
But it should be. IMHO, that would result in a consistent API, with t.skip staying as it is.
Closes #14