Add `todo` test modifier #565

BarryThePenguin · 2016-02-18T11:39:48Z

Seems too simple...

Fixes #563

mention-bot · 2016-02-18T11:39:48Z

By analyzing the blame information on this pull request, we identified @Carnubak and @sohamkamani to be potential reviewers

vadimdemedes · 2016-02-18T11:42:00Z

lib/runner.js

@@ -46,6 +46,8 @@ util.inherits(Runner, EventEmitter);
 module.exports = Runner;

 optionChain(chainableMethods, function (opts, title, fn) {
+	fn = fn || function () {};


I think having function noop() {} at the top of the file and using it here instead of anonymous function inline would be better.

Cool, can do. Was also wondering if || is enough, or should I explicitly be checking for typeof fn === 'undefined'?

Nah, || is enough here.

vadimdemedes · 2016-02-18T11:42:42Z

Thanks for this PR, @BarryThePenguin!

sindresorhus · 2016-02-18T11:45:17Z

Can you also update the docs?

https://github.com/sindresorhus/ava#testskiptitle-body
https://github.com/sindresorhus/ava#skip-tests

sindresorhus · 2016-02-18T11:45:43Z

This allows omitting test function for all tests, not just .skip, @vdemedes are we ok with that?

vadimdemedes · 2016-02-18T11:49:35Z

@sindresorhus Very good point. I guess we came back to the problems discussed in #9, test.todo() makes more sense in that case. But for now, maybe we should restrict function-less tests only to .skip.

BarryThePenguin · 2016-02-18T11:57:40Z

Mmm... in this context skip and todo are synonymous with each other. Maybe it's worth exploring?

BarryThePenguin · 2016-02-18T12:08:12Z

Also, noting the api change, this allows for test.skip(). Not sure how helpful that is to people

kentcdodds · 2016-02-18T16:47:16Z

Also, noting the api change, this allows for test.skip(). Not sure how helpful that is to people

That makes sense. I wonder though if we should think about explicitly disallowing that to avoid people abusing it and confusing others.

BarryThePenguin · 2016-02-18T23:36:15Z

I'd be happy to investigate the possibility of having both a .skip() and .todo() modifier.

Thoughts so far:

skip has optional title, requires a test function
todo requires a test title, won't accept a test function

Would also require adding todo to the tap reporter output

kentcdodds · 2016-02-18T23:47:36Z

You know, I would actually prefer to leave skip as it is and introduce a todo that only accepts a message. I would still like to have the todo in the output so it's a constant reminder. I think that's the value of coding the todo rather than just using a comment.

BarryThePenguin · 2016-02-19T02:35:48Z

I've had a go at the todo modifier. I also had a go at implementing the tap output for both todo and skip directives

novemberborn · 2016-02-26T15:31:55Z

lib/reporters/tap.js

@@ -27,6 +27,10 @@ TapReporter.prototype.start = function () {
 TapReporter.prototype.test = function (test) {
 	var output;

+	var passed = test.todo ? 'not ok' : 'ok';
+	var todoDirective = test.todo ? '# TODO' : '';
+	var skipDirective = test.skipped ? '# SKIP' : '';


Perhaps do:

var directive = ''; if (test.todo) { directive = '# TODO'; } else if (test.skipped) { directive = '# SKIP'; }

Which simplifies the todoDirective || skipDirective || '' statement below and removes the unnecessary || '' fall-through.

Ha, great. Will do 💩

kentcdodds · 2016-02-26T15:48:58Z

I'm not certain I follow the results of all the changes. But I'll just say that I would recommend that skip require a function and the only way to avoid having to specify a function is if you use todo. Fewer ways to do the same thing is 👍 :D

novemberborn · 2016-02-26T15:56:08Z

But I'll just say that I would recommend that skip require a function and the only way to avoid having to specify a function is if you use todo. Fewer ways to do the same thing is 👍

Yes agreed. Would be good to make test.skip('my test') fail with a helpful error, pointing at test.todo().

@BarryThePenguin changes are looking good generally, nice work. Would be good to clear up some of the logic and state management so it's easier to follow how these special test cases (pun very much intended) are handled.

BarryThePenguin · 2016-02-26T23:38:03Z

Just went through the review.

Added Mini and Verbose reporter handling for todo + tests

vadimdemedes · 2016-02-28T10:28:08Z

I like it, LGTM from me!

@sindresorhus @novemberborn @jamestalmage ?

sindresorhus · 2016-02-28T10:30:26Z

lib/runner.js

+			throw new TypeError('Expected a string');
+		}
+	} else if (opts.skipped && typeof fn !== 'function') {
+		throw new TypeError('Expected a function. Use todo for tests without a function');


throw new TypeError('Expected a function. Use `test.todo()` for tests without a function.');

sindresorhus · 2016-02-28T10:33:43Z

Looks pretty good. The docs needs to be updated and some inline feedback resolved.

sindresorhus · 2016-02-28T10:37:34Z

lib/runner.js

+		fn = noop;
+
+		if (typeof title !== 'string') {
+			throw new TypeError('Expected a string');


throw new TypeError('`todo` tests require a title');

BarryThePenguin · 2016-02-28T11:18:01Z

Updated. Happy to squash/rebase to make the commit history a little more sane 👍

sindresorhus · 2016-02-28T11:20:08Z

LGTM

sindresorhus · 2016-02-28T11:20:11Z

Happy to squash/rebase to make the commit history a little more sane

👍

novemberborn · 2016-02-28T16:08:42Z

lib/reporters/tap.js

+
+	if (test.todo) {
+		directive = '# TODO';
+	} else if (test.skipped) {


This should be test.skip, from https://github.com/BarryThePenguin/ava/blob/bb3c972cc4df4b3df0139b5d6f738cb18da0e6e7/lib/runner.js#L99.

novemberborn · 2016-02-28T16:14:42Z

lib/runner.js

 		this.stats.testCount++;

-		if (result.result.metadata.skipped) {
+		if (test.metadata.skipped && !test.metadata.todo) {


You still need to increase the todoCount, as well as initialize it to 0 in the run() method. Then in https://github.com/sindresorhus/ava/blob/0e1886582833986b896c31859aa61d8da404e996/api.js#L233 you need to compute the total todoCount. These counts are mocked in the reporter tests.

Oh and here https://github.com/sindresorhus/ava/blob/0e1886582833986b896c31859aa61d8da404e996/api.js#L207.

vadimdemedes · 2016-03-01T12:18:10Z

lib/reporters/tap.js

+	var passed = test.todo ? 'not ok' : 'ok';
+
+	if (test.todo) {
+		directive = '# TODO';


If I'm not mistaken, there's no such directive as # TODO in TAP spec. For TAP, we could treat todo tests just like skip and output them the same way.

In that case, great! :)

kentcdodds · 2016-03-01T20:38:49Z

This is awesome! Thanks everyone for your work on this. Looking forward to seeing this merged :-)

sindresorhus · 2016-03-02T07:56:11Z

Still some things to iron out I think.

import test from 'ava';
test.todo('foo');
test.todo('bar');

Gives no output with the default reporter:

~/dev
❯ ava

And with --verbose:

~/dev
❯ ava --verbose

  - foo
  - bar

  2 tests failed

I don't think todo tests should be failing.

import test from 'ava';
test('foo', t => t.pass());
test.todo('bar');

Outputs it's passing, but no mention of the 1 todo and the test run still exit with exit code 1 even though the output indicates differently:

~/dev
❯ ava

  1 passed

novemberborn · 2016-03-02T11:15:51Z

lib/runner.js

 		this.stats.testCount++;

-		if (result.result.metadata.skipped) {
+		if (test.metadata.todo) {
+			this.stats.todoCount++;


You need to initialize todoCount to 0 in the Runner#run() method.

novemberborn · 2016-03-02T11:21:07Z

@BarryThePenguin would you mind adding a test in test/api.js that verifies the passCount, skipCount, todoCount and failCount values? There is no explicit test for at the moment. The reporter tests use mocked values so everything seems fine until it's verified by hand.

BarryThePenguin · 2016-03-03T11:33:13Z

@sindresorhus sure, I'll see if I can work these out

I'm starting to question my reasoning for initially marking todo tests as failed. I've been following the TAP spec and I'm not sure it's quite clear.

These tests represent a feature to be implemented or a bug to be fixed and act as something of an executable “things to do” list. They are not expected to succeed. Should a todo test point begin succeeding, the harness should report it as a bonus. This indicates that whatever you were supposed to do has been done and you should promote this to a normal test point.

This implies that todo tests fail until they are implemented. This PR currently expects todo tests to have no implementation. Does this mean we mark them as failed? Again, I'm not sure how tightly we want AVA coupled with the TAP spec.

BarryThePenguin · 2016-03-03T11:37:09Z

Also, thanks everyone for providing feedback and spending the time to help out. It's been a really enjoyable experience 🐹

sindresorhus · 2016-03-03T14:24:52Z

The TAP description doesn't explicitly say they should fail either, though, just that they shouldn't succeed like a normal test. I think we should rather enable users to fail todo tests in the linting step, not in AVA. Todo tests should be like TODO comments, informative, but not affecting.

novemberborn · 2016-03-03T14:31:50Z

I think we should rather enable users to fail todo tests in the linting step, not in AVA. Todo tests should be like TODO comments, informative, but not affecting.

👍

BarryThePenguin · 2016-03-03T21:02:57Z

Updated.

@vdemedes @novemberborn todo is no longer skipped as well. Instead, in test-collection we check for the skipped and todo modifier.

@novemberborn I've added the test/api.js test

@sindresorhus I'm not quite sure about the exit code being 1, when should the exit code be 1? These are the outputs for the examples you provided above

mini reporter


  2 todo

verbose reporter


  - foo
  - bar

  0 tests passed
  2 tests todo

test + todo example


  1 passed  1 todo

sindresorhus · 2016-03-04T06:55:54Z

@BarryThePenguin Perfect. Exactly what I was thinking :)

LGTM

vadimdemedes · 2016-03-04T07:42:17Z

Looks fine to me as well, thanks @BarryThePenguin!

sindresorhus · 2016-03-04T08:17:26Z

Landed. Thank you for tirelessly working on this one @BarryThePenguin. It turned out really good.

jfmengels · 2016-03-04T10:35:15Z

Thanks @BarryThePenguin!

kentcdodds · 2016-03-04T13:22:26Z

This is terrific. I love the level of thought for quality coming from this project. Thanks everyone (especially @BarryThePenguin)!

vadimdemedes reviewed Feb 18, 2016
View reviewed changes

novemberborn reviewed Feb 26, 2016
View reviewed changes

sindresorhus changed the title ~~Omit function for skipped test~~ Add todo test modifier Feb 26, 2016

sindresorhus reviewed Feb 28, 2016
View reviewed changes

novemberborn reviewed Feb 28, 2016
View reviewed changes

This was referenced Feb 28, 2016

AVA hangs when test is used incorrectly #574

Closed

t.todo(), like t.skip() but without a function #563

Closed

naptowncode mentioned this pull request Feb 29, 2016

TAP: Should test.only() contribute to skipped count summary? #471

Closed

vadimdemedes reviewed Mar 1, 2016
View reviewed changes

sindresorhus force-pushed the master branch from c6214aa to 2f10448 Compare March 2, 2016 07:42

novemberborn reviewed Mar 2, 2016
View reviewed changes

sindresorhus mentioned this pull request Mar 3, 2016

no-todo-test rule avajs/eslint-plugin-ava#31

Closed

jfmengels mentioned this pull request Mar 3, 2016

add no-todo-test rule (fixes #31) avajs/eslint-plugin-ava#32

Merged

BarryThePenguin added 3 commits March 4, 2016 07:53

Implementation for todo test modifier and tests

16607e1

Reporters and reporter tests for todo test modifier

dde79a4

Documentation for todo test modifier

38b2ab2

sindresorhus closed this in e697629 Mar 4, 2016

BarryThePenguin deleted the skip-noop branch March 4, 2016 08:14

Add todo test modifier #565

Add todo test modifier #565

Conversation

BarryThePenguin commented Feb 18, 2016

mention-bot commented Feb 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadimdemedes commented Feb 18, 2016

sindresorhus commented Feb 18, 2016

sindresorhus commented Feb 18, 2016

vadimdemedes commented Feb 18, 2016

BarryThePenguin commented Feb 18, 2016

BarryThePenguin commented Feb 18, 2016

kentcdodds commented Feb 18, 2016

BarryThePenguin commented Feb 18, 2016

kentcdodds commented Feb 18, 2016

BarryThePenguin commented Feb 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds commented Feb 26, 2016

novemberborn commented Feb 26, 2016

BarryThePenguin commented Feb 26, 2016

vadimdemedes commented Feb 28, 2016

Choose a reason for hiding this comment

sindresorhus commented Feb 28, 2016

Choose a reason for hiding this comment

BarryThePenguin commented Feb 28, 2016

sindresorhus commented Feb 28, 2016

sindresorhus commented Feb 28, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds commented Mar 1, 2016

sindresorhus commented Mar 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

novemberborn commented Mar 2, 2016

BarryThePenguin commented Mar 3, 2016

BarryThePenguin commented Mar 3, 2016

sindresorhus commented Mar 3, 2016

novemberborn commented Mar 3, 2016

BarryThePenguin commented Mar 3, 2016

sindresorhus commented Mar 4, 2016

vadimdemedes commented Mar 4, 2016

sindresorhus commented Mar 4, 2016

jfmengels commented Mar 4, 2016

kentcdodds commented Mar 4, 2016

Add `todo` test modifier #565

Add `todo` test modifier #565