TAP support

[vadimdemedes]

This PR fixes #27.

It adds --tap flag to a CLI to generate TAP-compatible output, which can then be used with any 3rd-party reporters.

$ ava --tap example.js | tap-nyan

Here are the things to be resolved before this PR can be merged:

• Test tracked assertions in test/assert.js
• Decide on assertion output
• Test TAP output (and probably separate into own module)
• Include unhandled rejections and exceptions into TAP output
• Support fail-fast mode via Bail out!
• Add information on enabling TAP output to Readme

Let's discuss!

[vadimdemedes]

Need to remove this.

[sindresorhus]

Looks very promising! :) 🍰

Decide on assertion output

What specifically needs to be decided on?

[jamestalmage]

Do we have to make assert a class? Can't we just track assertions here? Demanding the assert class track assertions is going to make it harder to support custom assertions down the road.

Is the issue simply that it is hard to know which argument is the message here?

[vadimdemedes]

Do we have to make assert a class? Can't we just track assertions here?

I'm open to suggestions!

Demanding the assert class track assertions is going to make it harder to support custom assertions down the road.

It will, see my comment below.

Is the issue simply that it is hard to know which argument is the message here?

The issue lies in AVA's feature - concurrency. As you know, tests run concurrently, so we have to track which assertions happened during that particular test. That's why Assert class is needed, because that way we can safely get a list of test's assertions.

[vadimdemedes]

Decide on assertion output
What specifically needs to be decided on?

Sorry for not making that clear. We need to decide what title do we display for assertions:

test something
  ✅ 'yes' === 'no'
  or
  ✅ t.is()
  or
  ✅ t.is('yes', 'no')

So, test have titles and assertion have titles too, so we need to decide on the latter.

[sindresorhus]

We need to decide what title do we display for assertions

'yes' === 'no' and the power-assert output when t.ok()/t.notOk(). We should also show the assert message when provided.

[jamestalmage]

@vdemedes - I know I linked it before somewhere, but here are the docs you need for the empower-core.

FYI: @twada keeps stuff he still considers "beta" under his GitHub account and only moves stuff to the power-assert-js GitHub group when he is committing to a stable, long-term API. That is why you had trouble finding those docs.

As for powerAssertContext, there is a good example of it's contents here. @twada may be able to provide a better link. You will likely need powerAssertContext.source.content.

Finally, we are going to need to come up with a solution for assertions like this:

t.same(foo, {
  blah: 'blah',
  bar: 'foo',
  zippo: [1, 2, 3]
});

That is probably a more than we want to log, even when compacted to a single line. We can probably do some pretty clever collapsing of the code eventually (i.e. t.same(foo, {blah: ...})). We could even change color to light gray for the parts we collapse.

For the first pass, maybe we don't use the powerAssertContext (or maybe only if source.content.length is less than some arbitrary number).

[twada]

For multiline assertions, power-assert normalizes the assertion lines to single line to render diagrams on reporting.
So powerAssertContext.source.content is always in a form of single line, no matter how much lines original assertion has.

For example, In @jamestalmage's case, powerAssertContext.source.content is
t.same(foo, { blah: \'blah\', bar: \'foo\', zippo: [1, 2, 3] })'

[jamestalmage]

powerAssertContext.source.content is always in a form of single line

Yes, my point was that even that will end up verbose given a large enough object declaration. I'm sure our eventual solution will share code with SuccinctReporter.

[sindresorhus]

nitpick, but equal => strictEqual

[vadimdemedes]

@jamestalmage So I tried to use latest changes to assertions and turned out it's not actually usable for tracking assertions. I inserted console.log(event) at the beginning of onAssertionEvent and it shows no useful information (in case there was no error):

{ originalMessage: undefined,
  enhanced: true,
  args: [ true, undefined ],
  assertionThrew: false,
  returnValue: undefined }

event.powerAssertContext is undefined.

The assertion is a simple t.true(true). Am I missing something?

[jamestalmage]

I looked into this a bit.
Turns out, powerAssertContext only attaches when the code is sufficiently complex to warrant it:

These will not attach a powerAssertContext:

t.ok(true);
t.is('a', 'a');

These will:

var a = 'a';
t.ok(a);
t.is(a, 'a');
t.is({a: 'a'}.a, 'a');

It should still be possible to build something close to the original source using args.

[vadimdemedes]

It should still be possible to build something close to the original source using args.

I don't think this will have good consequences, we need a stable solution. Otherwise we'll end up with a babel-like stream of issues.

[sindresorhus]

Can we defer the assert title complications for later and focus on just getting some TAP output in this PR? We can just go for naive assert titles for now.

[jprichardson]

Can we defer the assert title complications for later and focus on just getting some TAP output in this PR? We can just go for naive assert titles for now.

👍 It's (no tap) the only thing that's preventing me from deep diving into Ava :)

[jamestalmage]

defer the assert title complications for later

I agree. I think it will take us a while to figure that out correctly, and I don't want to hold up TAP support. Unfortunately, onAssert currently doesn't even tell you which method was called. @vdemedes - see #340 for a workaround on that (the identity function hack, not the whole PR).

[vadimdemedes]

Ok, let's postpone it for now, I'll come up with something simpler. I'm off tomorrow, so I'll post updates the day after.

@jamestalmage will take a look, thanks.

[sindresorhus]

Alright. I've opened an issue for it so we don't forget. #341

[vadimdemedes]

PR updated with additions to Readme. Now it can be merged and we'll handle assertion output later.

[sindresorhus]

Don't use bash as it's not valid shell script with the $.

[vadimdemedes]

Was using it, because sometimes it highlights executable name and symbols like |. Will remove it, no probs ;)

[sindresorhus]

Yes, but the highlighting is very random.

[sindresorhus]

I think something like this would look nicer:

console.log([
    '# ' + err.message,
    format('not ok %d - %s', ++i, err.message),
    '  ---',
    '    name: ' + err.name,
    '    at: ' + getSourceFromStack(err.stack, 1),
    '  ...'
].join('\n'));

[sindresorhus]

Can you add some tests?

[sindresorhus]

And add tap as a keyword to package.json.

[sindresorhus]

Would be more fun to illustrate with the nyan reporter, right? You also need to show installing the reporter. Otherwise we'll end up with support issues on how it doesn't work...

[vadimdemedes]

Should we include a screenshot or that'd be too much?

[sindresorhus]

Actually, drop the install instruction. We can't know if they want to use it locally or globally. It has to be globally installed if used like the above, or locally if used in a npm run script.

[sindresorhus]

Should we include a screenshot or that'd be too much?

Sure! Here you go. Make sure to use <img> and half its width so it becomes retina.

[vadimdemedes]

@sindresorhus

Can you add some tests?

I will!

[vadimdemedes]

I'm thinking if we should return generated output from lib/tap.js, instead of console.log()ing it from there. Would also be easier to test TAP output.

[sindresorhus]

👍

[vadimdemedes]

PR updated according to all suggestions:

• Improve readme and add a screenshot
• Return TAP output, instead of console.log()ing it directly from lib/tap.js
• Add tests for TAP
• Add "tap" keyword to package.json
• Clean up lib/tap.js

[sindresorhus]

double comma

[sindresorhus]

LGTM

[sindresorhus]

Support fail-fast mode via Bail out!

⬆️ Can you open an issue for this?

[sindresorhus]

(and probably separate into own module)

I'm surprised there's no compliance suite any TAP producer could use. Would be so useful (hint hint). Something like https://github.com/promises-aplus/promises-tests

[jamestalmage]

LGTM

I have run it against a number of projects, and the output looks great when everything passes. Something feels off when there are errors though:

classic reporter:

tail end of spec reporter:

Neither tells you which test the failure came from (these are not uncaught exceptions, but assertion errors).

[jamestalmage]

👍 LGTM

We can improve the messages incrementally.

[vadimdemedes]

@sindresorhus

Support fail-fast mode via Bail out!
⬆️ Can you open an issue for this?

I figured there's no need for this one, as we end the tests "safely". And not all reporters handle this stuff.

@jamestalmage Try to use tap-* reporters, not reporters for mocha, as it had some out-dated TAP output.

[vadimdemedes]

I'm surprised there's no compliance suite any TAP producer could use. Would be so useful (hint hint).

@sindresorhus good idea!

[vadimdemedes]

@sindresorhus @jamestalmage guys, need you to LGTM one more time. It does not want to merge it otherwise.

[vadimdemedes]

@sindresorhus @jamestalmage Never mind, merged!

[vadimdemedes]

[sindresorhus]

So much tap! 💃