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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jfparadis
Copy link

@jfparadis jfparadis commented Nov 20, 2019

  • include the count of skipped tests in the generated output
  • resolve an issue where test.skip(fn, cb) would behave differently from test(fn, { skip: true }, cb) and not mark the tests with prefix # SKIP

This is a rebase and simplification of PR #197

References:
Issue #90
Issue #115

@jfparadis
Copy link
Author

@ljharb, I've implemented what was discussed previously, as-is, but I think the output is confusing. For example:

1..536
# tests 536
# pass  284
# fail  252

Now becomes:

1..536
# tests 536
# pass  284
# skip  333
# fail  252

And the numbers don't add-up: when a test suite is marked as skipped, all assertions inside are ignored and not counted.

Maybe we need to add a count of test suites separate from the count of assertions that we have now.

@ljharb
Copy link
Collaborator

ljharb commented Nov 20, 2019

hmm, i'm confused. if they're not counted, they shouldn't show up in "tests" either, no?

@jfparadis
Copy link
Author

Correct.

The count of skip is a number mixing skipped assertions and skipped tests, and can't be reconciled in a formula #tests = #pass + #fail.

Showing #skip with those other numbers looks strange (they don't add up). Maybe this can be solved with formatting, but I'm not sure this would break the TAP format.

1..536
# tests 536 
# pass  284
# fail  252
(skipped 333)

@ljharb
Copy link
Collaborator

ljharb commented Nov 20, 2019

Is the issue that the "tests" doesn't include the "skips"? iow, tests = pass + skip + fail?

@jfparadis
Copy link
Author

Correct. It's a simple thing, but we should avoid confusion. That's why I'm on the fence with this one.

@ljharb
Copy link
Collaborator

ljharb commented Nov 21, 2019

so to restate:

  1. in master, skipped tests are omitted from the "tests" count.
  2. in this PR, a "skips" count is shown, but the "tests" count still omits the skipped tests.

It seems to me that the proper fix is to include skips in tests - since the total number of tests in the above example is actually 869 anyways - iow, it's a bug that they're not counted. Thoughts?

@jfparadis
Copy link
Author

Correct, that's the idea, but it's not possible. Here, by following the direction of the original PR, I realized the discrepancy:

Skip is usually done at the test level and count at the assertion level

Compare:

test("foo", t => {
   t.pass();
   t.pass();
});
# tests 2
# pass  2

With:

test("foo", t => {
   t.pass();
   t.pass();
});
# tests 0
# skip  1

Skip can be counted at the two levels:

I think we need two different counts:

  1. count of test()
  2. count of t.<assertion>()

Which would result in an output similar to Jest, with a distinction between test suites and tests:

# suites: 1
# pass:   1

# tests: 2
# pass:  2

And that's where I think we would interfere with the TAP protocol.

@ljharb
Copy link
Collaborator

ljharb commented Nov 30, 2019

Hmm, that seems to suggest that this PR isn't really an option unless a skipped test still evaluates, but all the tests i skip are because they will not successfully evaluate.

@r0mflip
Copy link
Contributor

r0mflip commented Nov 30, 2019

Adding the # suites: 1 count sounds cool. I think that adding it doesn't break the TAP protocol since it's just a "diagnostic line". But are tests and test suites relatable?

@ljharb
Copy link
Collaborator

ljharb commented Nov 30, 2019

That doesn’t seem like the right concept to me; a file isn’t automatically 1 suite, it might be N

@r0mflip
Copy link
Contributor

r0mflip commented Nov 30, 2019

I agree, a file isn't a single suite. As far as I can find neither does TAP describe any "suites" or diagnostic output lines like tape does. It's simply subjective taste and I think what this PR is trying to do falls under this category. TAP consumers just ignore the comment lines anyway.

@jfparadis
Copy link
Author

A "test suite" corresponds to a "describe()" test group in Jest, and it's not counting files.

Jest

describe('my beverage', () => {
  test('is delicious', () => {
    expect(myBeverage.delicious).toBeTruthy();
  });

  test('is not sour', () => {
    expect(myBeverage.sour).toBeFalsy();
  });
});
Test Suites: 1 passed, 1 total
Tests:       2 passed, 2 total

Tape

My suggestion would be to count "test()" groups in Tape.

test('my beverage', t => {
  t.test('is delicious', tt => {
    tt.ok(myBeverage.delicious);
  });

  t.test('is not sour', tt => {
    tt.notOk(myBeverage.sour);
  });
});
# suites: 1
# pass:   1

# tests: 2
# pass:  2

@ljharb
Copy link
Collaborator

ljharb commented Nov 30, 2019

That might map well in many cases, altho i often - in both jest and tape - have a single top level describe/test, with everything nested underneath - so that pattern wouldn’t really mesh well for my use cases.

@ljharb ljharb added semver-major: breaking change Any breaking changes semver-minor: new stuff Any additions. labels Dec 28, 2019
@@ -104,6 +105,7 @@ Results.prototype._watch = function (t) {
var write = function (s) { self._stream.queue(s); };
t.once('prerun', function () {
var premsg = '';
if (t._skip) self.skip ++;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this line count a skipped test as a skipped assertion. Unlike in line 122 which counts skipped asserts, this line seems to include numbers that aren't consistent.

There is no equivalent to Line 123 in this prerun callback.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, this is just an update to an old PR meant to illustrate the problem you are mentioning: tests and assertions are mixed together.

The reason the two are mixed together is that tape reports "assertions" as "tests", and doesn't count suites.

# tests 2
# pass  1
# fail  1

If we don't count suites, the only way to indicate that some assertions are skipped is to add one to tests because we don't know how many are actually skipped. That create the problem that you are mentioning.

The solution is probably to change the count and the output to be something like node-tap:

Suites:   1 failed, 1 passed, 1 skipped, 3 of 3 completed
Asserts:  2 failed, 3 passed, 2 skipped, of 7

My concern is not to break people with this change, so I will probably do this behind a flag. Once we agree on the output and the necessity of a flag, then the changes will be trivial.

What's your take? If would be great if you could help me by testing the upcoming changes in your environment. :)

@ljharb
Copy link
Collaborator

ljharb commented Apr 24, 2020

I'm preparing to release v5, but I'm not yet confident about the design of this feature to include it in v5, so I think it'll have to wait for v6.

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.

3 participants