Report skipped assertions

[davidmason]

This should address some of #90

• show skipped assertions like in the spec, e.g. ok 23 # skip Insufficient flogiston pressure.
• show skipped tests like in the spec, e.g. 1..0 # Skipped: WWW::Mechanize not installed
• include the count of skipped tests in the generated output, like the spec says:

I should be able to look at the last bit in the next week or two.

[Raynos]

This Lgtm.

Cc @substack

[ghost]

If this makes tape more spec compliant this looks good to me.

[Raynos]

Would appreciate a second opinion on spec compliance from @isaacs or @malandrew

[davidmason]

TLDR: needs a bit more work to match the spec. Should allow a string value for skip option, which is the skip reason.

I think I need to change a bit of the code back to fit the spec, and there are a few more things to add. I have copied what look like the relevant parts of the spec here for convenience (quoted sections are all from TAP 13 specification).

There is a possible point of ambiguity with skipped tests: should the test description be shown for skipped tests? None of the examples show both a description and a directive (# todo or # skip), but the test line summary seems to indicate that both can be there. None of the examples actually show both a description and a directive on a test line, but nothing says that having both would be invalid. It would be handy to see the descriptions of skipped tests in the output.

To summarize: - ok/not ok (required) - Test number (recommended) - Description (recommended) - Directive (only when necessary)

For # todo and # skip directives, an optional description is allowed indicating what needs doing, or the reason for skipping, respectively. tape does not appear to support this at the moment, but it would be easy to add, either as a separate option or just by setting a string instead of a bool. e.g. { skip: 'there is no replacement Beryllium Sphere on board' }.

If the directive starts with # TODO, the test is counted as a todo test, and the text after TODO is the explanation.
not ok 13 # TODO bend space and time

The harness should report the text after # SKIP\S*\s+ as a reason for skipping.

ok 23 # skip Insufficient flogiston pressure.
Similarly, one can include an explanation in a plan line, emitted if the test file is skipped completely:

1..0 # Skipped: WWW::Mechanize not installed

TAP version 13
1..5
ok 1 - approved operating system
# $^0 is solaris
ok 2 - # SKIP no /sys directory
ok 3 - # SKIP no /sys directory
ok 4 - # SKIP no /sys directory
ok 5 - # SKIP no /sys directory

TAP version 13
1..0 # skip because English-to-French translator isn't installed

[isaacs]

If tape currently just doesn't report anything for t.foo(..., { skip: true }) then that's not violating the spec. Remember, the spec is only about what the output needs to look like, not what a given test runner needs to output. That's a matter of artistic choices as much as spec compliance :)

If you DO choose to say something about tests that are skipped (which, imo, is a good idea), then yes, the format like ok 3 - some description # skip doesn't work on Windows is the way to do it.

node-tap lets you pass either a string or a boolean as the skip option. For example:

$ node -e 'require("./").pass("skip me once", {skip:"for reasons"})'
TAP version 13
ok 1 - skip me once # SKIP for reasons
1..1
# time=20.092ms

The default reporter shows this as "pending tests" in mocha-speak:

$ node -e 'require("./").test("child test", function (t) { t.pass("skip me once", {skip:"for reasons"});t.end()})' | tap -
child test ............................................ 0/1
  Skipped: 1
    skip me once for reasons

total ................................................. 0/1

  0 passing (41.431ms)
  1 pending

[shawnpetros]

Status on this? We're using tape and faucet and would love to see test.skip() and t.skip() in faucet reporter...

[ljharb]

@davidmason are you still interested in completing this?

If so, let's get a fresh rebase on master, please check the "allow edits" box on the right hand column of the PR, and let's get a restatement of what this PR is currently changing and what is still left to do (possibly in a future PR).

My sense is that "making skipped tests count towards the plan" is a useful, albeit breaking, change, and "including the skip count in the output" is also an excellent change. Making skip accept a string value in place of a truthy one, and showing that in the output, seems useful but not required in this PR (it can be added later).

[davidmason]

@ljharb I can't get to this any time soon. I've checked Allow edits from maintainers, so feel free to do anything you like with it.

[ljharb]

This seems good - @davidmason, is there any additional changes needed before you think this is ready to land?

[davidmason]

@ljharb what's here should be useful without any extra changes, so I'm happy if you are. I'm busy with a toddler and a new job for now, and neither of them use tape, so I'll have to rely on others to make the call for when/whether to merge.

[ljharb]

@davidmason thanks. do you think the last checkbox in the OP can be checked, or is there more changes to be done there?