Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support use of t.end as callback. #181

Closed
wants to merge 1 commit into from
Closed

Support use of t.end as callback. #181

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion lib/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ function Test(title, fn) {
if (this.title === 'callee$0$0') {
this.title = '[anonymous]';
}

Object.keys(Test.prototype).forEach(function (key) {
this[key] = this[key].bind(this);
}, this);
}

module.exports = Test;
Expand Down Expand Up @@ -134,7 +138,11 @@ Test.prototype.run = function () {
}.bind(this));
};

Test.prototype.end = function () {
Test.prototype.end = function (err) {
if (arguments.length >= 1 && err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I'm saying

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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'));
});

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

this.ifError(err);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

if (this.endCalled) {
throw new Error('.end() called more than once');
}
Expand Down
13 changes: 13 additions & 0 deletions readme.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,19 @@ test(async t => {

*You don't have to manually call `t.end()`.*

### Callback support

AVA supports using `t.end` as the final callback when using node-style
error-first callback APIs. AVA will consider any truthy value passed as
the first argument to `t.end` to be an error.

```js
test(t => {
// t.end automatically checks for error as first argument
fs.readFile('data.txt', t.end);
});
```


## API

Expand Down
18 changes: 18 additions & 0 deletions test/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,24 @@ test('handle non-assertion errors', function (t) {
});
});

test('end can be used as callback without maintaining thisArg', function (t) {
ava(function (a) {
setTimeout(a.end);
}).run().then(function (a) {
t.false(a.assertError);
t.end();
});
});

test('end can be used as callback with error', function (t) {
ava(function (a) {
a.end(new Error('failed'));
}).run().catch(function (err) {
t.true(err instanceof Error);
t.end();
});
});

test('handle non-assertion errors even when planned', function (t) {
ava(function (a) {
a.plan(1);
Expand Down