Improve metadata checks #980

asafigan · 2016-07-24T20:17:39Z

Fixes #847

validate-test.js checks the following:

always should only be used with after or afterEach
todo may not have an implementation, but must have a title
todo should not be combined with any other modifiers. It's simply for documentation. So no skipped, no failing, no afterEach, etc.
skip.only should be a failure
failing shouldn't be used with hooks
failing shouldn't be skipped
only shouldn't be used with hooks

All unit tests are in test/validate-test.js. Integration tests are in test/runner.js. Moved some tests from test/test-collection.js to test/runner.js.

vadimdemedes · 2016-09-04T09:41:43Z

Good work, @asafigan! Sorry for not taking a look sooner. Let's hear one more LGTM and merge it in.

@sindresorhus @jamestalmage @novemberborn @sotojuan @jfmengels looks good to you?

jfmengels · 2016-09-04T10:00:55Z

lib/validate-test.js

+		}
+
+		if (metadata.failing) {
+			return '`only` tests cannot be failing';


I'm not well aware of how this works, but the message talks about only tests, not skipped tests. Are tests skipped due to the presence of an only marked as skipped, or is the message inconsistent with what is checked?

Not sure why failing tests should not be skipped/only-ed by the way.

I think just the message is wrong and should be fixed.

Not sure why failing tests should not be skipped/only-ed by the way.

I agree, failing tests should be allowed to skip and only.

asafigan · 2016-09-10T21:00:44Z

Thanks for the feed back guys. I'm going to be busy for awhile, but I will try to look into everything.

sotojuan · 2016-09-18T01:24:57Z

Few nitpicks but overall LGTM. Should be merged by the next push!

vadimdemedes · 2016-10-09T09:50:26Z

Hey @asafigan, any chance you have some free time to complete this? :)

asafigan · 2016-10-14T13:25:06Z

Sorry I have been so slow on this. I will do it this weekend.

vadimdemedes · 2016-10-14T17:27:22Z

@asafigan No worries, thanks!

…tadata-checks-issue-847 updating from upstream

asafigan · 2016-10-17T13:37:16Z

@vdemedes unless there is any other feedback, I believe it ready to merge

vadimdemedes · 2016-10-19T20:34:48Z

@asafigan Thanks for finishing this up! I'm a little busy now, I'll try to review ASAP, unless someone from the team beats me to it.

novemberborn

Thanks for coming back to this @asafigan!

I wonder if the tests should be added to test/runner.js instead. The examples are clearer that way and less coupled to how tests definitions are represented internally.

Also, @sindresorhus, do we have rules around when error messages end in a period? Looking at the deletions we haven't been consistent about this before, and perhaps we don't care, but wanted to raise it regardless.

sindresorhus · 2016-11-08T15:14:49Z

Also, @sindresorhus, do we have rules around when error messages end in a period? Looking at the deletions we haven't been consistent about this before, and perhaps we don't care, but wanted to raise it regardless.

See: sindresorhus/eslint-plugin-unicorn#26

sindresorhus · 2016-11-08T15:15:32Z

@asafigan Any chance you would be able to address @novemberborn's feedback so this can be merged?

sindresorhus · 2016-11-17T10:15:21Z

ping @asafigan

asafigan · 2016-11-17T14:15:46Z

So, I need to rewrite the tests in test/runner.js. What about reformatting the error messages? I couldn't tell if there was a discussion with that.

sindresorhus · 2016-11-18T04:59:26Z

So, I need to rewrite the tests in test/runner.js.

Yes, so they're not tied to the internal implementation. Just make them like the existing ones you already have there: https://github.com/avajs/ava/pull/980/files#diff-a0b87ab613d07bba77930609ec360b3dR323

What about reformatting the error messages? I couldn't tell if there was a discussion with that.

What you have now is fine.

novemberborn · 2017-01-17T17:23:04Z

@asafigan how's it going? Wondering if you're still interested in completing this? Thanks :)

novemberborn

(Just updating PR status.)

asafigan · 2017-01-22T23:44:01Z

I'm getting major issues when I try to test now.

asafigan · 2017-01-22T23:45:05Z

I pull from master and solved the merge conflicts but now there are a bunch of errors when I run npm test or npm run test-win

asafigan · 2017-01-22T23:45:43Z

I went to my master branch and pulled from upstream master and I get the same issues

sindresorhus · 2017-01-23T00:07:00Z

lib/runner.js

@@ -4,6 +4,7 @@ const Promise = require('bluebird');
 const optionChain = require('option-chain');
 const matcher = require('matcher');
 const TestCollection = require('./test-collection');
+const validateTest = require('./test-colection');


You have a typo. You're also importing the same thing twice.

asafigan · 2017-01-23T01:12:05Z

oh yeah I noticed that

asafigan · 2017-01-23T01:12:25Z

the real issue was that I forgot to rerun npm install

asafigan · 2017-01-23T01:23:33Z

I had to fix some other things, too.
Sorry, it took me so long to circle back on this.

novemberborn

Thanks for picking this up again @asafigan. Nearly there! 😄

novemberborn · 2017-01-23T09:49:57Z

test/runner.js

+
+	t.throws(() => {
+		runner.beforeEach.only('', noop);
+	}, {message: '`only` is only for tests and cannot be used with hooks'});


This is better as:

t.throws(() => { runner.beforeEach.only('', noop); }, TypeError, '`only` is only for tests and cannot be used with hooks');

Same goes for the other tests. See http://www.node-tap.org/asserts/#tthrowsfn-expectederror-message-extrafor specifics.

asafigan · 2017-01-24T14:30:57Z

Will do

sindresorhus · 2017-01-25T18:39:25Z

test/runner.js

@@ -234,7 +234,7 @@ test('skip test', t => {

 	t.throws(() => {
 		runner.skip('should be a todo');
-	}, {message: 'Expected an implementation. Use `test.todo()` for tests without an implementation.'});
+	}, TypeError, 'Expected an implementation. Use `test.todo()` for tests without an implementation.');


This is incorrect. You're no longer testing the error message. The third argument is just an assert failure message.

You could do:

t.throws(() => { runner.todo('todo', () => {}); }, new TypeError('`todo` tests are not allowed to have an implementation. Use `test.skip()` for tests with an implementation.'));

thanks for catching that

sindresorhus · 2017-01-26T03:18:05Z

Thank you @asafigan. Excited to finally land this :)

asafigan added 3 commits July 21, 2016 22:12

add validation passing all unit tests

76b4066

validate-test integrated

b50c365

tests now check error message from validate-test

824d936

vadimdemedes changed the title ~~Fixes #847~~ Improve metadata checks Aug 9, 2016

sindresorhus force-pushed the master branch from 5ccec78 to 30ab26f Compare August 18, 2016 16:21

jfmengels reviewed Sep 4, 2016
View reviewed changes

asafigan added 2 commits October 15, 2016 15:14

Merge branch 'master' of https://github.com/avajs/ava into improve-me…

238bef2

…tadata-checks-issue-847 updating from upstream

allow skipping failing tests

093060b

sindresorhus force-pushed the master branch 2 times, most recently from 4bc021e to 476c653 Compare October 18, 2016 03:23

novemberborn reviewed Oct 23, 2016

View reviewed changes

sindresorhus force-pushed the master branch 4 times, most recently from 8610723 to 4acb78f Compare November 22, 2016 17:46

novemberborn requested changes Jan 17, 2017

View reviewed changes

asafigan added 3 commits January 22, 2017 17:48

moved test to test/runner.js and changed them to use runner

3d04ff2

resolved merge conflicts

e935033

solved merge conflicts for test/runner.js

718cf6c

sindresorhus reviewed Jan 23, 2017

View reviewed changes

redid styling

514440b

asafigan added 2 commits January 22, 2017 20:20

add empty function to todo tests

5863914

fixed over replacement of functions

35aa667

novemberborn requested changes Jan 24, 2017

View reviewed changes

asafigan added 2 commits January 24, 2017 21:11

add backticks to error message

274307a

added error class checks to throw tests

aca3818

novemberborn approved these changes Jan 25, 2017

View reviewed changes

sindresorhus requested changes Jan 25, 2017

View reviewed changes

corrected throws assertions

5f2a5ea

sindresorhus approved these changes Jan 26, 2017

View reviewed changes

sindresorhus merged commit 9616dde into avajs:master Jan 26, 2017

asafigan deleted the improve-metadata-checks-issue-847 branch January 26, 2017 13:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve metadata checks #980

Improve metadata checks #980

asafigan commented Jul 24, 2016

vadimdemedes commented Sep 4, 2016

jfmengels Sep 4, 2016 •

edited

Loading

vadimdemedes Sep 4, 2016

asafigan commented Sep 10, 2016

sotojuan commented Sep 18, 2016

vadimdemedes commented Oct 9, 2016

asafigan commented Oct 14, 2016

vadimdemedes commented Oct 14, 2016

asafigan commented Oct 17, 2016

vadimdemedes commented Oct 19, 2016

novemberborn left a comment

sindresorhus commented Nov 8, 2016

sindresorhus commented Nov 8, 2016

sindresorhus commented Nov 17, 2016

asafigan commented Nov 17, 2016

sindresorhus commented Nov 18, 2016

novemberborn commented Jan 17, 2017

novemberborn left a comment

asafigan commented Jan 22, 2017

asafigan commented Jan 22, 2017

asafigan commented Jan 22, 2017

sindresorhus Jan 23, 2017

asafigan commented Jan 23, 2017

asafigan commented Jan 23, 2017

asafigan commented Jan 23, 2017

novemberborn left a comment

novemberborn Jan 23, 2017

asafigan commented Jan 24, 2017

sindresorhus Jan 25, 2017

asafigan Jan 25, 2017

sindresorhus commented Jan 26, 2017

Improve metadata checks #980

Improve metadata checks #980

Conversation

asafigan commented Jul 24, 2016

vadimdemedes commented Sep 4, 2016

jfmengels Sep 4, 2016 • edited Loading

Choose a reason for hiding this comment

vadimdemedes Sep 4, 2016

Choose a reason for hiding this comment

asafigan commented Sep 10, 2016

sotojuan commented Sep 18, 2016

vadimdemedes commented Oct 9, 2016

asafigan commented Oct 14, 2016

vadimdemedes commented Oct 14, 2016

asafigan commented Oct 17, 2016

vadimdemedes commented Oct 19, 2016

novemberborn left a comment

Choose a reason for hiding this comment

sindresorhus commented Nov 8, 2016

sindresorhus commented Nov 8, 2016

sindresorhus commented Nov 17, 2016

asafigan commented Nov 17, 2016

sindresorhus commented Nov 18, 2016

novemberborn commented Jan 17, 2017

novemberborn left a comment

Choose a reason for hiding this comment

asafigan commented Jan 22, 2017

asafigan commented Jan 22, 2017

asafigan commented Jan 22, 2017

sindresorhus Jan 23, 2017

Choose a reason for hiding this comment

asafigan commented Jan 23, 2017

asafigan commented Jan 23, 2017

asafigan commented Jan 23, 2017

novemberborn left a comment

Choose a reason for hiding this comment

novemberborn Jan 23, 2017

Choose a reason for hiding this comment

asafigan commented Jan 24, 2017

sindresorhus Jan 25, 2017

Choose a reason for hiding this comment

asafigan Jan 25, 2017

Choose a reason for hiding this comment

sindresorhus commented Jan 26, 2017

jfmengels Sep 4, 2016 •

edited

Loading