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

Report skipped assertions #197

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@davidmason
Copy link

davidmason commented Oct 2, 2015

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.

[New] Report skipped assertions
The standard indicates that skipped assertions should be summarized
at the end of output, and the specified place for # SKIP and # TODO
is before the message.
@Raynos

This comment has been minimized.

Copy link
Collaborator

Raynos commented Oct 2, 2015

This Lgtm.

Cc @substack

@substack

This comment has been minimized.

Copy link
Owner

substack commented Oct 2, 2015

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

@Raynos

This comment has been minimized.

Copy link
Collaborator

Raynos commented Oct 2, 2015

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

@davidmason

This comment has been minimized.

Copy link

davidmason commented Oct 5, 2015

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

This comment has been minimized.

Copy link
Contributor

isaacs commented Oct 5, 2015

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

@shime shime referenced this pull request Oct 6, 2015

Closed

handle assert.skip #26

@ljharb ljharb force-pushed the substack:master branch from 13654ad to 545db26 Feb 14, 2016

@shawnpetros

This comment has been minimized.

Copy link

shawnpetros commented Feb 28, 2017

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

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Feb 28, 2017

@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

This comment has been minimized.

Copy link

davidmason commented Mar 9, 2017

@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 ljharb force-pushed the davidmason:master branch from 6d9ee1e to 022c26c Jan 9, 2019

@ljharb

ljharb approved these changes Jan 9, 2019

Copy link
Collaborator

ljharb left a comment

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

@davidmason

This comment has been minimized.

Copy link

davidmason commented Jan 9, 2019

@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

This comment has been minimized.

Copy link
Collaborator

ljharb commented Jan 9, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment