Support use of t.end as callback.

[timoxley]

• Binds all Test.prototype methods to current test.
• Checks for truthy error as first argument to Test.prototype.end.

Closes #180.

[sindresorhus]

if (arguments.length >= 1 && err) {?

[jamestalmage]

why not just if (err) ?

[timoxley]

oh yeah wtf am I doing

[timoxley]

Originally this was !!err but linter told me to Boolean(err) so I followed blindly.

[timoxley]

Updated.

[sindresorhus]

Looks good to me. @vdemedes @jamestalmage ?

[jamestalmage]

If they've used t.plan(), won't this increase their assertion count causing a plan error message?

[timoxley]

What happens if you have a plan and call t.end?

[vadimdemedes]

Except suggestions in comments, everything is good.

[jamestalmage]

I'm still genuinely curious why you are checking argument length.
If arguments.length === 0, isn't err going to be falsy no matter what?

[timoxley]

Idea is to only log the assertion if there's some arguments passed. Alternative is have every test log an additional ifError assertion at the end.

[jamestalmage]

No, I'm saying

if (err) {
  this.ifError(err)
}

[timoxley]

You're right.

[timoxley]

Perhaps it should always log the assertion if there's arguments.length? i.e. intent is to use as errback handler.

[jamestalmage]

Hmmm.

We need it to behave consistently whether or not it's being used as a callback

t.plan(1);
t.ok(true);
t.end();

should perform the same as

t.plan(1)
t.ok(true);
setImmediate(t.end);

[timoxley]

in neither of those situations will the t.end be called with any arguments though.

[timoxley]

The use-case I'm thinking about goes something like this:

test(t => {
    fs.readFile('data.txt', t.end);
});

[jamestalmage]

that is fine, but for your plan test do something that is not async:

test(t => {
    t.plan(1);
    t.ok(true);
    t.end(new Error('something bad happened'));
});

[vadimdemedes]

To avoid conflicts with t.plan(), this.ifError(err) could be replaced with:

this.assertError = new assert.AssertionError({
  actual: err,
  message: 'Callback called with an error → ' + err,
  operator: 'callback'
});

this.exit();

This is the same stuff we do when promise rejects.

[timoxley]

Also note we're opting for checking & printing error before checking if .end has been called more than once. This is how tape does it and I think it makes sense as the error message + stack is going to be more helpful messaging than simply 'called end more than once'. They'll get this message too, but only after they fix the actual source of the error.

[jamestalmage]

What happens if you have a plan and call t.end?

I don't know, but we probably should have a test for it.

[jamestalmage]

@timoxley looking at the code again I'm pretty sure you are going to get a bad error message if you use plan and end with this.

Please write a test that does this:

test(t => {
  t.plan(1);
  t.ok(1);
  t.end(new Error('you should get this error message, not a plan count error'));
});

And check the message of the thrown exception.

[jamestalmage]

@timoxley I think it's down to that one if statement and another test ensuring your use of ifError does not generate a plan count error message.

[vadimdemedes]

Hey @timoxley, could you please fix this suggestion (#181 (diff)), so that we can merge it in? Thanks!

[vadimdemedes]

Closed via 403a28c

Thanks @timoxley!