Improve metadata checks

[jamestalmage]

We currently validate the test metadata property in two different places:

• todo is checked here
• always and only are checked here
• We currently allow failing on any test or hook, but as discussed here, we probably only want to allow that on tests, not hooks.
• I'm not sure we prevent skip.only anywhere, but we probably should.

This should probably all be extracted into lib/validate-test.js, with something like this:

// validate-test.js
module.exports = function(title, fn, metadata) {
  // throws? Then we need to wrap with a try/catch (which is slow).
  // returns a String with the rejection reason, returning null means it's good?
};

This would simplify our tests for these validations as well (they could be unit tests for this validation module, instead of integration tests that require spinning up forks).

[jamestalmage]

Also, from what I can tell, we don't really prevent todo chaining, and chaining a todo really makes no sense.

See #846 (comment)

[SamVerschueren]

👍 on the todo chaining. It really doesn't make sense to have test.serial.todo('Add a test for foo').

[novemberborn]

It does make sense if you know you need to write a serial test (e.g. because all related tests in the same file are serial).

[asafigan]

Could I take this issue on?

[sotojuan]

@asafigan You're more than welcome to!

[asafigan]

Thanks!
I need some more information before I get started. Where do you want the validation tests to be called from? Also are these the only validations that we need?

[jamestalmage]

Non comprehensive list of validations:

• always used with anything besides after or afterEach - already checked here
• todo may not have an implementation, but must have a title - already checked [here]
• todo should not be combined with any other modifiers. It's simply for documentation. So no skipped, no failing, no afterEach, etc.
• skip.only makes no sense - should be a failure
• failing probably doesn't belong on hooks
• failing probably shouldn't be skipped

Where do you want the validation tests to be called from?

Create a new module lib/validate-test.js, and a new set of tests in test/validate-test.js.

[asafigan]

Do you just want me to make the module and test? or do you want me to also integrate it into the other modules?

[jamestalmage]

Integrate it. So maybe one/two integration tests to test/runner.js that validates your new module is called correctly.

[asafigan]

should we prevent skipping hooks?

[asafigan]

also should we allow only on hooks?

[asafigan]

So for integration I'm thinking that validation would be done in the runner's _addTest method. It would run the validation and if the result isn't null, it would throw an error with the string from the validation test. Does that seem good?

[asafigan]

When I was doing the integration I found that 'skip' should work with hooks

[sindresorhus]

should we prevent skipping hooks?

No

also should we allow only on hooks?

No

[asafigan]

ok, that's what I assumed after looking into it myself.
I have made a pull request that fixes this issue. Please take a look at it.