Planned number of tests is ignored

[RReverser]

Got issues where tests were silently failing at 60% of the suite, and tap-summary reports as if all the tests passed (it's hard to notice an issue unless you pay attention and care to the number reported). As per TAP specification, this is exactly kind of problem plan is supposed to solve - reporter should compare total number of assertions with those given in the plan and report failures if they don't match (or if the plan wasn't provided at all - it should be either in beginning or in the very end of the suite).

[zoubin]

tests were silently failing at 60% of the suite

@RReverser I'm sorry I don't quite understand the problem. Would you please give an example? I'd be glad to check that.

[RReverser]

@zoubin It's simply that test runner crashes due to some global issue, so output is incomplete (not all the assertions were written out).

[Hypercubed]

@RReverser Can you share an example. When I try to simulate by adding a process.exit this I get the TAP output not ok 2 plan != count which tap-summary is reporting correctly. If I force an error TAP outputs an error.

[Hypercubed]

I think I understand. tap-out populates the plans array when the stream outputs the "plans" line at the end (e.g. 1..30). If the end of stream is reached without emitting plans then the test failed to complete. We need, at a minimum, to check that the plans array is not empty.

[RReverser]

@Hypercubed Well, I actually emitted plans line right after TAP version line, so seems to be a slightly different case. Let me try later today.

[Hypercubed]

How does other TAP transformers act in this case? tap-spec seems to do what I suggested: https://github.com/scottcorgan/tap-spec/blob/master/index.js#L70

[RReverser]

Yes, test failures if count mismatches is what I originally asked for, too.

[RReverser]

So a simple artificial example to show the problem:

> echo "TAP version 13
1..4
ok 1
ok 2
" | ../js/node_modules/.bin/tap-summary

------------------------------------ Tests -------------------------------------

(still happening in published 2.1.1, not sure why this issue has been closed)

[RReverser]

@zoubin @Hypercubed Can you please reopen the issue?

[Hypercubed]

@RReverser, I'll look into checking the plan counts. Have you seen other TAP consumers that handle the plan counts correctly? I'm wondering how they handle subtests or the concatenated output from multiple tests.

I can't reopen, I'm a just a user. 😞

[Hypercubed]

The "plan" line can be written multiple times in case of subtests or globbing. So we need to sum the plan counts and compare to the assertion counts. Something like:

    var planned = res.plans.reduce(function (p, c) {
      return c.to - c.from + 1 + p;
    }, 0)

    if (res.asserts.length < planned) {
      dup.failed = true
      process.exit(1)
    }

How does that sound? @RReverser @zoubin

Edit: Probably should be res.asserts.length !== planned

[zoubin]

@RReverser This issue got automatically closed when I merged #9 .
I reopen it now.

[zoubin]

@Hypercubed I just made you a collaborator :)

[Hypercubed]

@zoubin Thanks. I'll still go through the PR process.

I was playing with this. I guess we need to decide if the planned tests don't meet the assertion count do we:

1. Write an error message and fail hard with a status code > 0 in tap.on('output')
2. Set dup.failed, continue reporting, print a message in the summary (e.g. "planned: XX plans don't match final assertion count"), let cmd.js exit with status code > 0

[RReverser]

Have you seen other TAP consumers that handle the plan counts correctly?

Unfortunately, not yet (not in JS at least), but this is part of the spec so I'd really want them to.

I was playing with this. I guess we need to decide if the planned tests don't meet the assertion count do we:

IIUC 2nd option, I'm for it. Was thinking about reporting any tests starting from the mismatched one as failed, so that in case N tests were planned but M assertions found, summary would say that (N-M) tests failed.

[Hypercubed]

@zoubin if it's alright with you I'll work on this. No hard fails just output in the summary and the people exit code.

If plans exceed asserts the difference counts as fails. If no plans are found it reports that and exit code is set to 1. Not sure if there can ever be a case where the asserts exceed the plans. This should already be a fail.

[zoubin]

@Hypercubed I'm also with 2nd option.

Just for your information, there is a special case:

test('empty', function (t) {
  t.end()
})

The plan line will be 1..0.
So, c.to - c.from is -1 and res.asserts.length < planned will be false.

[Hypercubed]

If it is always 1..N even if N == 0 then maybe:

    var planned = res.plans.reduce(function (p, c) {
      return c.to + p;
    }, 0)

Then planned will be '>= 0'.

[zoubin]

The issue closed again when #10 was merged.
@RReverser Does 2.1.2 resolves this problem?

[RReverser]

Looks so, thanks @Hypercubed!

One rather unrelated question/issue: is it by design that tap-summary doesn't support output of just

TAP version 13
1..4
ok 1 test 1
ok 2 test 2
ok 3 test 3
ok 4 test 4

but does support

TAP version 13
1..4
# test 1
ok 1
# test 2
ok 2
# test 3
ok 3
# test 4
ok 4

?

Spec allows first one as valid too, whereas comment lines are optional and their behavior is implementation-dependant (I'm outputting TAP from custom non-JS producer according to the spec, so had to match up with tape format and add comments just so that it would be accepted by tap-summary).

[zoubin]

tap-summary is supposed to be a reporter, which consumes the output of a parser which processes the TAP output.
Right now tap-summary uses tap-out to parse the TAP output, and it seems that it does not support the first format.
An issue could be submitted to tap-out, I believe.

[RReverser]

The reason I asked here is because tap-out seems to parse it well, so it's rather result handling issue:

{
  "tests": [],
  "asserts": [
    {
      "type": "assert",
      "raw": "ok 1 test 1",
      "ok": true,
      "number": 1,
      "name": "test 1",
      "test": 0
    },
    {
      "type": "assert",
      "raw": "ok 2 test 2",
      "ok": true,
      "number": 2,
      "name": "test 2",
      "test": 0
    },
    {
      "type": "assert",
      "raw": "ok 3 test 3",
      "ok": true,
      "number": 3,
      "name": "test 3",
      "test": 0
    },
    {
      "type": "assert",
      "raw": "ok 4 test 4",
      "ok": true,
      "number": 4,
      "name": "test 4",
      "test": 0
    }
  ],
  "versions": [
    {
      "type": "version",
      "raw": "TAP version 13"
    }
  ],
  "results": [],
  "comments": [],
  "plans": [
    {
      "type": "plan",
      "raw": "1..4",
      "from": 1,
      "to": 4
    }
  ],
  "pass": [
    {
      "type": "assert",
      "raw": "ok 1 test 1",
      "ok": true,
      "number": 1,
      "name": "test 1",
      "test": 0
    },
    {
      "type": "assert",
      "raw": "ok 2 test 2",
      "ok": true,
      "number": 2,
      "name": "test 2",
      "test": 0
    },
    {
      "type": "assert",
      "raw": "ok 3 test 3",
      "ok": true,
      "number": 3,
      "name": "test 3",
      "test": 0
    },
    {
      "type": "assert",
      "raw": "ok 4 test 4",
      "ok": true,
      "number": 4,
      "name": "test 4",
      "test": 0
    }
  ],
  "fail": []
}

[zoubin]

@RReverser Thanks for the information. I'll check it later in #11