TAP support #326

vadimdemedes · 2015-12-12T13:51:53Z

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 · 2015-12-12T13:53:21Z

lib/assert.js

+
+module.exports = Assert;
+
+var x = Assert.prototype;


Need to remove this.

sindresorhus · 2015-12-12T23:47:09Z

Looks very promising! :) 🍰

Decide on assertion output

What specifically needs to be decided on?

jamestalmage · 2015-12-13T00:39:07Z

lib/test.js

 		api.skip[el] = skipFn;
 		api[el] = function () {
 			try {
-				var fn = assert[el].apply(assert, arguments);
+				var fn = self.assert[el].apply(self.assert, arguments);


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?

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 · 2015-12-13T07:52:37Z

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 · 2015-12-14T02:14:07Z

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 · 2015-12-14T03:01:30Z

@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 · 2015-12-14T04:05:41Z

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 · 2015-12-14T04:25:52Z

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 · 2015-12-15T17:39:06Z

lib/test.js

@@ -20,13 +21,14 @@ function Test(title, fn) {
 		title = null;
 	}

-	assert.is(typeof fn, 'function', 'you must provide a callback');
+	assert.equal(typeof fn, 'function', 'you must provide a callback');


nitpick, but equal => strictEqual

vadimdemedes · 2015-12-15T19:09:35Z

@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 · 2015-12-15T21:08:01Z

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 · 2015-12-15T21:11:09Z

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 · 2015-12-15T21:19:55Z

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 · 2015-12-15T21:26:48Z

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 · 2015-12-15T22:30:14Z

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 · 2015-12-15T22:53:36Z

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 · 2015-12-15T23:05:36Z

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

vadimdemedes · 2015-12-21T13:25:09Z

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

sindresorhus · 2015-12-21T14:36:14Z

readme.md

+
+AVA can generate TAP output via `--tap` option for use with any 3rd-party reporters.
+
+```bash


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

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

Yes, but the highlighting is very random.

sindresorhus · 2015-12-21T14:40:33Z

lib/tap.js

+	output.push('  ...');
+
+	console.log(output.join('\n'));
+};


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 · 2015-12-21T14:42:37Z

Can you add some tests?

sindresorhus · 2015-12-21T14:43:10Z

And add tap as a keyword to package.json.

sindresorhus · 2015-12-21T14:47:54Z

readme.md

+AVA can generate TAP output via `--tap` option for use with any 3rd-party reporters.
+
+```bash
+$ ava --tap | tap-spec


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

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

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.

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 · 2015-12-21T14:50:31Z

@sindresorhus

Can you add some tests?

I will!

vadimdemedes · 2015-12-21T21:32:39Z

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 · 2015-12-21T21:38:15Z

👍

vadimdemedes · 2015-12-21T22:05:39Z

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 · 2015-12-21T23:15:06Z

cli.js

@@ -36,7 +37,8 @@ var cli = meow([
 	'  --init       Add AVA to your project',
 	'  --fail-fast  Stop after first test failure',
 	'  --serial     Run tests serially',
-	'  --require    Module to preload (Can be repeated)',
+	'  --require    Module to preload (Can be repeated)',,


double comma

sindresorhus · 2015-12-21T23:17:21Z

LGTM

sindresorhus · 2015-12-21T23:18:15Z

Support fail-fast mode via Bail out!

⬆️ Can you open an issue for this?

sindresorhus · 2015-12-21T23:19:48Z

(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 · 2015-12-21T23:23:23Z

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 · 2015-12-21T23:26:20Z

👍 LGTM

We can improve the messages incrementally.

vadimdemedes · 2015-12-22T07:19:19Z

@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 · 2015-12-22T07:20:00Z

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

@sindresorhus good idea!

vadimdemedes · 2015-12-22T07:26:52Z

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

TAP support

vadimdemedes · 2015-12-22T07:28:12Z

@sindresorhus @jamestalmage Never mind, merged!

vadimdemedes · 2015-12-22T07:38:36Z

sindresorhus · 2015-12-22T12:43:46Z

So much tap! 💃

vadimdemedes reviewed Dec 12, 2015
View reviewed changes

lib/assert.js

module.exports = Assert;

var x = Assert.prototype;

Copy link

Contributor Author

vadimdemedes Dec 12, 2015

Choose a reason for hiding this comment

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

Need to remove this.

jamestalmage reviewed Dec 13, 2015
View reviewed changes

sindresorhus mentioned this pull request Dec 14, 2015

Node.js-style --require CLI argument #296

Merged

sindresorhus reviewed Dec 15, 2015
View reviewed changes

sindresorhus mentioned this pull request Dec 15, 2015

Improve assertion titles #341

Closed

vadimdemedes force-pushed the tap-support branch 2 times, most recently from 74673c1 to 736a044 Compare December 21, 2015 13:23

sindresorhus reviewed Dec 21, 2015
View reviewed changes

sindresorhus changed the title ~~[WIP] TAP support~~ TAP support Dec 21, 2015

sindresorhus reviewed Dec 21, 2015
View reviewed changes

vadimdemedes force-pushed the tap-support branch 3 times, most recently from 7639370 to cccb2cd Compare December 21, 2015 22:00

sindresorhus reviewed Dec 21, 2015
View reviewed changes

add TAP support

d765dc2

vadimdemedes force-pushed the tap-support branch from cccb2cd to d765dc2 Compare December 22, 2015 07:21

vadimdemedes pushed a commit that referenced this pull request Dec 22, 2015

Merge pull request #326 from sindresorhus/tap-support

d4d8310

TAP support

vadimdemedes merged commit d4d8310 into master Dec 22, 2015

vadimdemedes deleted the tap-support branch December 22, 2015 07:27


		AVA can generate TAP output via `--tap` option for use with any 3rd-party reporters.

		```bash

TAP support #326

TAP support #326

Conversation

vadimdemedes commented Dec 12, 2015

Choose a reason for hiding this comment

sindresorhus commented Dec 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadimdemedes commented Dec 13, 2015

sindresorhus commented Dec 14, 2015

jamestalmage commented Dec 14, 2015

twada commented Dec 14, 2015

jamestalmage commented Dec 14, 2015

Choose a reason for hiding this comment

vadimdemedes commented Dec 15, 2015

jamestalmage commented Dec 15, 2015

vadimdemedes commented Dec 15, 2015

sindresorhus commented Dec 15, 2015

jprichardson commented Dec 15, 2015

jamestalmage commented Dec 15, 2015

vadimdemedes commented Dec 15, 2015

sindresorhus commented Dec 15, 2015

vadimdemedes commented Dec 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Dec 21, 2015

sindresorhus commented Dec 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadimdemedes commented Dec 21, 2015

vadimdemedes commented Dec 21, 2015

sindresorhus commented Dec 21, 2015

vadimdemedes commented Dec 21, 2015

Choose a reason for hiding this comment

sindresorhus commented Dec 21, 2015

sindresorhus commented Dec 21, 2015

sindresorhus commented Dec 21, 2015

jamestalmage commented Dec 21, 2015

jamestalmage commented Dec 21, 2015

vadimdemedes commented Dec 22, 2015

vadimdemedes commented Dec 22, 2015

vadimdemedes commented Dec 22, 2015

vadimdemedes commented Dec 22, 2015

vadimdemedes commented Dec 22, 2015

sindresorhus commented Dec 22, 2015