Looking for clarity on test runner reporting and skip/todo

[philnash]

Affected URL(s)

https://nodejs.org/api/test.html#event-testfail

Description of the problem

I am writing a custom reporter for the Node.js test runner and I am confused over the types for the events that can be emitted and how they fit with skip/todo flags. Consequently, I'd also appreciate guidance on how skip/todo is supposed to act.

I've added this as a documentation bug, as I believe at the very least the docs could be more specific on the behaviour here. But I rubber-ducked myself while writing all this out, and came up with my own opinions about how this should work and that maybe there should be a behaviour update too. Read to the end to find out what I think!

From what I understand at the moment:

skips

If you skip a test using:

test("this is skipped", { skip: true }, () => {
  // test code
});

Then the actual body of the test is replaced with a noop and the test passes and is marked as skipped. That is, the runner emits a test:pass event and the data of the event includes skip: true.

If you skip a test using the context object instead, like this:

test("this is skipped", (t) => {
  t.skip();
  // test code
});

Then the test is run and the result could either be a pass or a fail, but will have skip: true set. That is, the test could emit either a test:pass or a test:fail but either way the data will include skip: true.

Also, if a skipped test fails with this method, all its parent tests fail.

todos

However, for todos, if you mark a test as todo using either pattern the test will run and the result may be a test:pass or a test:fail with todo: true in the data.

Further confusing, because the options can both be passed there is further behaviour from using them.

If you pass both todo and skip as options, like this:

test("this is skipped", { skip: true, todo: true }, () => {
  // test code
});

Then the test will not be run, it will be reported through test:pass and skip: true will be part of the data, but todo: true will not.

Then if you pass todo: true as an option, but call skip on the context.

test("this is skipped", { todo: true }, (t) => {
  t.skip();
  // test code
});

If the test fails it will trigger test:fail with skip: true set in the data and if it passes it will trigger test:pass with skip: true in the data. It will never report as todo: true. This is also the case if you call both t.skip() and t.todo() in the same test body.

Questions

So, skipping a test only really works if you do so in the options passed to the test, otherwise the test will be run and the result reported. Should that be the case, or should calling context.skip() anywhere within a test cease it's operation and report as a test:pass with skip: true?

When using skip and todo together, the todo is always overridden. This is also the case if you set messages for skip and todo. Should todo also be reported if it is used alongside skip?

When using todo on its own the test is run regardless and passes or fails are reported. Is that the expected behaviour? From research, that's how tape acts, but jest will not run a todo test (in fact, it throws an error if you try to pass a function to test.todo) and mocha hasn't implemented todo at all.

My opinions

A skipped test should never report as a failure. I can handle this in the reporter I'm writing, but it's weird to need to handle skips in both the pass and fail event. Plus, if a skipped test fails then all the parent tests fail and they should not be reported as having failed if all their child tests either passed or were skipped. I'd prefer to see a test aborted if context.skip is called part way through and the result deemed a pass, but marked as skipped. I suspect this makes coverage tough to get right though.

A test marked as "todo" seems to be more of an annotation than any other behaviour. So I'm happy that tests run regardless. But I think that annotation should be present even if the test is also skipped. So if someone uses skip and todo on the test, then both flags/messages should appear in the test:pass event data.

@cjihrig @MoLow I'd love to understand if I've summarised all of this correctly, and if perhaps there are changes to be made here, either in the docs or the behaviour itself?

[cjihrig]

Thanks for the issue. I think you have identified a few edge cases that we should address in the test runner. Here are my opinions on your questions. I think they align with your opinions as well:

Skip tests:

I think a skip test should never fail. That is the case if you skip the test before it starts. If the test has already started we should probably cancel it and mark it and any subtests as passing.

Todo tests:

I think this behavior is correct based on the TAP spec:

These tests represent a feature to be implemented or a bug to be fixed and act as something of an executable “things to do” list. They are not expected to succeed. Should a todo test point begin succeeding, the harness should report it as a bonus. This indicates that whatever you were supposed to do has been done and you should promote this to a normal test point.

Skip + todo tests:

This seems like something that should not normally be done. I don't think TAP can even report multiple directives. If it is done, we either need to report an error or pick the semantics of one. I think adopting skip semantics is the simplest here.

[MoLow]

A test marked as "todo" seems to be more of an annotation than any other behaviour. So I'm happy that tests run regardless. But I think that annotation should be present even if the test is also skipped. So if someone uses skip and todo on the test, then both flags/messages should appear in the test:pass event data.

a todo test allows it to fail without affecting the exit code. I think this is not documented and it should be.
skip + todo is indeed something that is not really documented/thought through but should easily be handled once we decide on a behavior

[philnash]

A test marked as "todo" seems to be more of an annotation than any other behaviour. So I'm happy that tests run regardless. But I think that annotation should be present even if the test is also skipped. So if someone uses skip and todo on the test, then both flags/messages should appear in the test:pass event data.

a todo test allows it to fail without affecting the exit code. I think this is not documented and it should be. skip + todo is indeed something that is not really documented/thought through but should easily be handled once we decide on a behavior

This would be really useful. However, a quick test with v20.5.0 shows me that a failing todo test will lead to an exit code of 1. This is the file I ran and the output I got: https://gist.github.com/philnash/f56cf86f7013b2be32e91a3f715bc9ee

[philnash]

Thanks @cjihrig, I'll try to find some time to fix this up. I have some questions, just to confirm:

Skip tests:

I think a skip test should never fail. That is the case if you skip the test before it starts. If the test has already started we should probably cancel it and mark it and any subtests as passing.

To implement this, the skip method on a test should act like the cancel method, instead setting the test as a pass and then calling the abort controller.

Then in the postRun method all subtests should also be set to skipped and passed before they report.

Todo tests:

I think this behavior is correct based on the TAP spec:

These tests represent a feature to be implemented or a bug to be fixed and act as something of an executable “things to do” list. They are not expected to succeed. Should a todo test point begin succeeding, the harness should report it as a bonus. This indicates that whatever you were supposed to do has been done and you should promote this to a normal test point.

👍

Skip + todo tests:

This seems like something that should not normally be done. I don't think TAP can even report multiple directives. If it is done, we either need to report an error or pick the semantics of one. I think adopting skip semantics is the simplest here.

I agree, if a test is marked as skip it should be skipped regardless of anything else.

[MoLow]

This would be really useful. However, a quick test with v20.5.0 shows me that a failing todo test will lead to an exit code of 1. This is the file I ran and the output I got: gist.github.com/philnash/f56cf86f7013b2be32e91a3f715bc9ee

it was lately fixed: #48929

[philnash]

Ah, brilliant, thanks!

[pulkit-30]

Question: Why do we run test fn when it is marked as todo?

What is expected is if a test is marked as todo then it should not run test fn whether it passes or fails. or another possibility, we can mention in docs that do not pass function if a test is a todo(also make relevant changes in code too), something similar to what jest does.

Context

code:

import test from 'node:test';
import assert from 'node:assert';

test.todo('some test', (t) => {
    throw new Error('some error') //error statement
    // assert.strictEqual(1,1).   //assert statement
})

the above code, if we uncomment error statement then it fail with fail count as 0 and todo count as 1 and the output is:

✖ some test (0.769708ms) # TODO
  Error: some error
      at TestContext.<anonymous> (file:///Users/pulkitgupta/Desktop/node/index.mjs:5:11)
      at Test.runInAsyncScope (node:async_hooks:206:9)
      at Test.run (node:internal/test_runner/test:631:25)
      at Test.start (node:internal/test_runner/test:542:17)
      at startSubtest (node:internal/test_runner/harness:218:17)

ℹ tests 1
ℹ suites 0
ℹ pass 0
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 1
ℹ duration_ms 78.867125

✖ failing tests:

test at file:/Users/pulkitgupta/Desktop/node/index.mjs:4:6
✖ some test (0.769708ms) # TODO
  Error: some error
      at TestContext.<anonymous> (file:///Users/pulkitgupta/Desktop/node/index.mjs:5:11)
      at Test.runInAsyncScope (node:async_hooks:206:9)
      at Test.run (node:internal/test_runner/test:631:25)
      at Test.start (node:internal/test_runner/test:542:17)
      at startSubtest (node:internal/test_runner/harness:218:17)

and if i comment error statement and uncomment assert statement, it pass with output

✔ some test (0.715125ms) # TODO
ℹ tests 1
ℹ suites 0
ℹ pass 0
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 1
ℹ duration_ms 81.904125

but overall it runs the test fn if it is marked as todo, which it should not be.

Edit: I'm looking for clarity on this, not pointing to a bug 😅

[philnash]

@pulkit-30 I think that is due to the TAP spec as explained by cjihrig above.

[MoLow]

Closing as completed by #52204