resolves #2149, #1257: Make TAP reporter conform to TAP13 #3452

mollstam · 2018-08-08T07:44:42Z

Description of the Change

The TAP reporter has been made to conform with TAP13.

Changes to the actual reporter are small: a version string is emitted as the first line, and error message and stack are both emitted inside a YAML block to be parsed by any TAP Consumer.

A quite large integration test has been added that encodes the expectations of the TAP13 spec, and the existing TAP reporter unit tests have been updated to be compatible with the new spec.

Finally 'reporters.fixture.js' was added to have something to run that generates test results with varying properties.

Alternate Designs

It was considered to add either a new reporter (e.g. tap13) or hide the new spec under a reporter option but since there are no breaking changes in TAP13 this was deemed unnecessary.

Why should this be in core?

Mocha relies on a default selection of usable reporters. A bunch of TAP Consumers that I've surveyed are poorly written and expect TAP13 or will otherwise break thus rendering the current TAP reporter in mocha less useful.

Benefits

A larger ecosystem of tools that are only compatible with TAP13 are made available to use together with mocha. Taking a step back, benefits in the TAP13 spec vs v12 are inherited, such as being able to encode richer error information in the now allowed YAML block in the TAP report is a Good Thing.

Possible Drawbacks

Many TAP Consumers I've encountered are not as flexible as one might wish so it is possible they don't fully conform to the old v12 spec, thus not being compatible with the TAP13 spec resulting in undefined behaviour. For example, currently in mocha the error stack is simply inserted into the report following the failed test, something that is allowed by the v12 spec as an "unknown" line that shouldn't be an error and may be handled by the test harness in any way. If a test harness handles mocha's stacks by e.g. using regex it will probably break now that the stack is wrapped in what TAP13 calls a "YAML block".

Applicable issues

#2149
#1257

Note

I've decided to not address issue #2410 with this PR as it involves communicating with third party tools (e.g. tap-spec) over TAP13 in a new way which I feel is outside the scope of simply bringing the TAP reporter up to spec, whereas 2410 is more about establishing a new spec regarding the structure of the YAML blocks (unspecified by TAP13).

mollstam · 2018-08-08T07:47:43Z

Hi y'all, thought I'd get the ball rolling on having mocha's tap reporter output v13 spec compliant reports since I need it. 😬

Any thoughts?

Technically v13 is not backwards compatible with v12 so this is a breaking change for any TAP Consumers that don't also support v13 but I'd defer to a maintainer to deem this being a patch or minor update.

(Kind of wished bots ignored WIP PRs)

coveralls · 2018-08-08T07:56:13Z

Coverage increased (+0.08%) to 90.32% when pulling 3dd336f on mollstam:reporter-tap-v13 into 2bb4124 on mochajs:master.

Bamieh · 2018-08-08T08:02:26Z

@mollstam thanks for taking your time to work on this here. Just a thought, we can provide the new version under a new flag, until we eventually deprecate the old one.

I looked at the v13 spec But it does not list/mention the breaking changes, can you provide the breaking changes to give us a better idea about what we're dealing with? Also if this gets pushed, we'll include those breaking changes in the release notes.

mollstam · 2018-08-08T08:23:28Z

@Bamieh Actually you're right, the v12 is quite clear:

A TAP parser is required to not consider an unknown line as an error but may optionally choose to capture said line and hand it to the test harness, which may have custom behavior attached. This is to allow for forward compatability.

I thought the version string would confuse it but it seems I was wrong, good good.

I starting thinking I'd hide the 13 spec under a reporter option but that doesn't seem to be necessary then.

mollstam · 2018-08-08T15:04:58Z

All right, I've at this point created a couple of integration tests to ensure that the output conforms to spec v13 and also some other Nice Things regarding the structure of the YAML block containing error message and stack. Only a few changes had to be made to tap.js to bring it up to spec.

The YAML test in itself is a bit hairy but I feel like it's good enough for now. I'd like to create a new fixture that generates a report with "a little bit of everything" so I can make sure that it looks good, and so that I can also stop using uncaught.fixture.js.

I'd like to also keep looking a bit at putting more data about errors into the YAML block for some nice reporting by tap-diff, tap-spec and other tape/node-tap related consumers. The TAP spec gives implementors free reign of content in YAML block would be nice to inventory what most consumers expect and see if we can provide it.

Finally I'm going to mull over issue #2410 a bit as well.

I'm off for today. 🐨

mollstam · 2018-08-09T10:07:20Z

All right, I feel like I'm done here and I'd like to open up for review.

Also the semver-major label is no longer required.

plroebuck · 2018-09-14T20:15:52Z

Since there's no huge major difference between TAP12 and YAP13/14 output, any consideration given to providing the TAP version via reporterOptions - making the TAP producer (reporter) both forward and backward compatible?

Add TAPProducer(version) class instance to TAP reporter, and defer all writing of TAP output to it.

As TAP12 is apparently ancient nowadays, perhaps TAP13 should be unspecified default. Or leave the default "as is" and allow this to be added to a "semver-minor" Mocha release.

plroebuck · 2018-09-14T20:21:58Z

Have these changes been run against the TAP 14 draft?

plroebuck · 2018-09-14T20:25:37Z

Without personally really diving into TAP, does this reporter's output match that which would be produced by Node-based TAP producers node-tap and tape?

lib/reporters/tap.js

plroebuck · 2018-09-14T20:34:55Z

lib/reporters/tap.js

@@ -32,6 +32,7 @@ function TAP(runner) {
  var failures = 0;

  runner.on('start', function() {
+    console.log('TAP version 13');


Consider using self.write() and self.writeln() so that file-based output via reporterOptions could be a trivially implemented possibility.

xunit reporter has a write() and I find nothing about writeln(). Can you point me in the right direction? console.log is what most reporters seem to be doing. 😱

Sorry, using functions from another proposed PR.
Until that's ready, you could implement those as:

/** * Write out the given string. * * @param {string} s */ TAP.prototype.write = function(s) { if (this.fileStream) { this.fileStream.write(s); } else { process.stdout.write(s); } }; /** * Write out the given string and terminate the line. * * @param {string} s */ TAP.prototype.writeln = function(s) { this.write(s + '\n'); };

Glancing your console.log usage, will also need to add an sprintf() function to handle your string substitutions.

test/integration/fixtures/reporters.fixture.js

test/integration/reporters.spec.js

plroebuck · 2018-09-14T20:59:14Z

As a (hopefully non-digressive) follow-on question, what TAP consumers were used to verify output from this PR? While I don't think these should be added to Mocha's build toolchain, any related test scripts you used for such can be preserved here as comment attachments.

plroebuck · 2018-09-15T14:59:42Z

lib/reporters/tap.js

-    }
-    if (err.stack) {
-      console.log(err.stack.replace(/^/gm, '  '));
+    var emitYamlBlock = err.message != null || err.stack != null;


I see "diff"-able Errors in Mocha codebase with actual and expected properties. Would they be considered "interesting" as output?

I think this is a great feature to have but it doesn't really have much to do with TAP13, rather a "while we're at it" thing and if I remember correctly I ran into some problems investigating it, but this was I while back. I think this feature shouldn't ride on this PR tbh.

But wasn't that exactly part of the tape output we should be attempting to match?

plroebuck · 2018-09-15T15:12:49Z

On suite end event, should Mocha emit a timing comment line?

A la this unbuffered subtest reference output.

TAP version 13
# Subtest: foo
    # Subtest: bar
        1..1
        ok 1 - this is fine
    ok 1 - bar # time=1.831ms

    1..1
ok 1 - foo # time=11.106ms

1..1
# time=19.039ms

mollstam · 2018-09-19T07:42:00Z

Hello friend, thanks for having a look! :)

Since there's no huge major difference between TAP12 and YAP13/14 output, any consideration given to providing the TAP version via reporterOptions - making the TAP producer (reporter) both forward and backward compatible?

Add TAPProducer(version) class instance to TAP reporter, and defer all writing of TAP output to it.

As TAP12 is apparently ancient nowadays, perhaps TAP13 should be unspecified default. Or leave the default "as is" and allow this to be added to a "semver-minor" Mocha release.

As the TAP13 spec is both forward and backward compatible I'm not sure keeping the TAP12 output around is necessary, what's your thoughts?

Have these changes been run against the TAP 14 draft?

I had a look at the TAP14 draft back when implementing this and decided not to implement it since it is only a draft and I wanted to keep the footprint of the PR as small as possible.

Without personally really diving into TAP, does this reporter's output match that which would be produced by Node-based TAP producers node-tap and tape?

I did recreate the fixture in tape, mostly to test how they handle subtests/suites (as discussed in issue #2410). Here is a gist of the tape version and here is a diff from tape report to this version of the mocha tap reporter. The main difference is the tape report having more structured data (operator, expected, actual) in the YAML rather than simply "baking it" into the message property. I investigated reaching similar formatting but the footprint of the fix grew and I wanted to keep things simple.

As you can see the running theme is me wanting to make as little of change as possible to reach what I set out to do. This iteration of the TAP reporter is not by any means a "final form" but a lot of the future work relies on the YAML format being standardized (consumers vary wildly) and for that I think TAP14 needs to get ratified, or more effort to accomodate the popular consumers out there.

Let me know your thoughts and thanks again.

Was failing since it matched entire output array and first line is now the version line per TAP v13 spec.

Only expect now is that version line is first line, more to come. Removed the previously added unit test which now is superflous.

plroebuck · 2018-09-20T14:24:29Z

Plan:

Leave Mocha TAP12 as is (default).
Use this PR's format as Mocha TAP13 (via reporterOptions.tap.version).
Future TAP14 format would add support for expected/actual in YAML block.

@mollstam, what do you think?

mollstam · 2018-09-20T14:49:44Z

Plan:

Leave Mocha TAP12 as is (default).

Use this PR's format as Mocha TAP13 (via reporterOptions.tap.version).

Future TAP14 format would add support for expected/actual in YAML block.

@mollstam, what do you think?

Sounds good! I named the option spec before reading this, would you prefer version? I'm indifferent. 🙂

mollstam · 2018-09-20T14:52:37Z

@plroebuck I think the expected/actual stuff can be added to the TAP13 producer since it's just extra data in the YAML, we wouldn't destroy anything.

I would like to also clean up the stack property to not include the message, but I'll have a look at that when I try to reach parity with other popular Producers, which will have to wait for next week.

plroebuck · 2018-09-20T16:45:15Z

lib/reporters/tap.js

  Base.call(this, runner);

+  var self = this;
  var n = 1;


n is same as runner.stats.tests. Remove.

plroebuck · 2018-09-20T16:49:00Z

lib/reporters/tap.js

+};
+
+TAPProducer.prototype.writeEnd = function(runner) {
+  console.log('# tests ' + (runner.stats.passes + runner.stats.failures));


Just noticed this, but think this is possibly an unreported existing bug.
runner.stats.tests will not agree with this math if pending tests occurred.

plroebuck · 2018-09-20T16:49:54Z

lib/reporters/tap.js

+  console.log('# tests ' + (runner.stats.passes + runner.stats.failures));
+  console.log('# pass ' + runner.stats.passes);
+  console.log('# fail ' + runner.stats.failures);
+};


Should this include a summary line for pending tests?

lib/reporters/tap.js

plroebuck · 2018-09-20T16:56:34Z

lib/reporters/tap.js

-  var passes = 0;
-  var failures = 0;
+
+  var tapSpec = '12';


Should this be numeric instead?

plroebuck · 2018-09-20T16:58:52Z

lib/reporters/tap.js


  runner.on('start', function() {
-    var total = runner.grepTotal(runner.suite);
-    console.log('%d..%d', 1, total);
+    self.producer.writeStart(runner);
  });

  runner.on('test end', function() {
    ++n;
  });


Remove test end event handler.

lib/reporters/tap.js

plroebuck · 2018-09-20T17:24:32Z

test/reporters/tap.spec.js

+          expectedString = string;
+          return expectedTotal;
+        };
+        TAP.call({}, runner);


TAP.call({}, runner, options);

plroebuck · 2018-09-20T17:27:00Z

test/integration/fixtures/reporters.fixture.js

+    it('should be able to survive global mass extinction events', function(done) {
+      throw new Error("How do we even test for this without causing one?")
+      done();
+    });


Can we add a pending test somewhere in this fixture?

it('just might one day');

plroebuck · 2018-09-21T20:03:22Z

lib/reporters/tap.js

-    console.log('# tests ' + (passes + failures));
-    console.log('# pass ' + passes);
-    console.log('# fail ' + failures);
+    self.producer.writeEnd(runner);


self.producer.writeEnd(stats);

plroebuck · 2018-09-21T20:04:04Z

lib/reporters/tap.js

+  console.log('ok %d %s', n, title(test));
+};
+
+TAPProducer.prototype.writeEnd = function(runner) {


TAPProducer.prototype.writeEnd = function(stats) {

plroebuck · 2018-09-21T20:10:22Z

lib/reporters/tap.js

+    this.producer = new TAP13Producer();
+  } else {
+    this.producer = new TAP12Producer();
+  }

  runner.on('start', function() {


runner.on('start', function() { var ntests = runner.grepTotal(runner.suite); self.producer.writeStart(ntests); });

plroebuck · 2018-09-21T20:11:59Z

lib/reporters/tap.js

  Base.call(this, runner);

+  var self = this;


Add this line:
var stats = runner.stats;

plroebuck · 2018-09-21T20:14:19Z

lib/reporters/tap.js

  });

  runner.on('test end', function() {
    ++n;
  });

  runner.on('pending', function(test) {
-    console.log('ok %d %s # SKIP -', n, title(test));
+    self.producer.writePending(n, test);


var n = stats.tests; self.producer.writePending(n, test);

plroebuck · 2018-10-02T11:14:35Z

In "test/reporters/tap.spec.js", let's make use of this function:

  var showOutput = false;

  /**
   * Run reporter using stream reassignment to capture output.
   *
   * @param {Object} stubSelf - Reporter-like stub instance
   * @param {Runner} runner - Mock instance
   * @param {boolean} [tee=false] - If `true`, echo captured output to screen
   */
  function runReporter(stubSelf, runner, tee) {
    // Reassign stream in order to make a copy of all reporter output
    var stdoutWrite = process.stdout.write;
    process.stdout.write = function(string, enc, callback) {
      stdout.push(string);
      if (tee) {
        stdoutWrite.call(process.stdout, string, enc, callback);
      }
    };

    // Invoke reporter
    TAP.call(stubSelf, runner);

    // Revert stream reassignment here so reporter output
    // can't be corrupted if any test assertions throw
    process.stdout.write = stdoutWrite;
  }

Bamieh added area: reporters involving a specific reporter semver-major implementation requires increase of "major" version number; "breaking changes" pr-needs-work labels Aug 8, 2018

mollstam mentioned this pull request Aug 8, 2018

🐛 Bug: Tap reporter doesn't adhere to spec #2410

Open

mollstam changed the title ~~[WIP] Make TAP reporter conform to v13 spec~~ Make TAP reporter conform to v13 spec Aug 9, 2018

mollstam changed the title ~~Make TAP reporter conform to v13 spec~~ resolves #2149 #1257 : Make TAP reporter conform to v13 spec Aug 9, 2018

mollstam changed the title ~~resolves #2149 #1257 : Make TAP reporter conform to v13 spec~~ resolves #2149 #1257 : Make TAP reporter conform to TAP13 Aug 9, 2018

mollstam changed the title ~~resolves #2149 #1257 : Make TAP reporter conform to TAP13~~ resolves #2149, #1257: Make TAP reporter conform to TAP13 Aug 9, 2018

plroebuck mentioned this pull request Sep 14, 2018

Provide list of breaking changes between TAP12 and TAP13 TestAnything/testanything.github.io#126

Closed

plroebuck reviewed Sep 14, 2018

View reviewed changes

lib/reporters/tap.js Outdated Show resolved Hide resolved

plroebuck reviewed Sep 14, 2018

View reviewed changes

test/integration/fixtures/reporters.fixture.js Show resolved Hide resolved

plroebuck reviewed Sep 14, 2018

View reviewed changes

test/integration/reporters.spec.js Show resolved Hide resolved

plroebuck reviewed Sep 15, 2018

View reviewed changes

mollstam added 4 commits September 19, 2018 10:05

added test to make sure first line of report is version line

d3c2df9

reporter will now print version line as first line

c067a6e

Changed test to look for plan (1..N) on second line.

9c78b61

Was failing since it matched entire output array and first line is now the version line per TAP v13 spec.

Added integration tests to make sure we conform to spec.

78ab6c5

Only expect now is that version line is first line, more to come. Removed the previously added unit test which now is superflous.

extracted pass reporting to producers

7951bf0

mollstam added 3 commits September 20, 2018 16:30

extracted end reporting to producers and refactored to common base

ff80cf4

fixed bad redeclare

0496ef3

fix to integrations tests to use TAP13 spec

42035e7