Skip to content
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
Choose a base branch
from

Conversation

davidmason
Copy link

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.

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
Copy link
Collaborator

Raynos commented Oct 2, 2015

This Lgtm.

Cc @substack

@ghost
Copy link

ghost commented Oct 2, 2015

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

@Raynos
Copy link
Collaborator

Raynos commented Oct 2, 2015

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

@davidmason
Copy link
Author

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
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

@shawnpetros
Copy link

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

@ljharb
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
Copy link
Author

@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.

Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@davidmason
Copy link
Author

@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
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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants