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.

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

Add programmatic API #279

Closed
wants to merge 1 commit into from
Closed

Add programmatic API #279

wants to merge 1 commit into from

Conversation

vadimdemedes
Copy link
Contributor

This PR adds a long-awaited programmatic API (#83).

To-do:

It also opens a road for #27 (TAP support).

@jamestalmage
Copy link
Contributor

I think this should go in the base directory:

require('ava/api');
// is better than
require('ava/lib/api');

this.tests = [];

Object.keys(Api.prototype).forEach(function (key) {
this[key] = this[key].bind(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a typeof check?

if (typeof this[key] === 'function') {
  this[key] = this[key].bind(this);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copied from lib/test.js.

@jamestalmage
Copy link
Contributor

@vdemedes - see my change.

The issue you were having was stats never gets emitted if there is syntax error in the test file (that is why it started failing on the babel-hook fixture, which intentionally has ES2015 syntax in a file that will not be instrumented by babel).

@jamestalmage
Copy link
Contributor

I propose we change this up just a bit.

Rather than api tracking counts for unhandledRejection, uncaughtException, etc.

Use the array: api.errors, and every time one is encountered, push to it:

on('unhandledRejection', function(e) {
  self.errors.push({type: 'unhandledRejection', error: e});
});

on('uncaughtException', function(e) {
  self.errors.push({type: 'uncaughtException', error: e});
});

// ...
self.errors.push({
  type: 'nonZeroExit',
  error: new Error('process exited with code: ' + code)
});

// ...
self.errors.push({
  type: 'assertionError',
  // ...
});

Then extend API with methods like:

api.hasErrors(); // true / false
api.getErrors(); // returns all {type: typeName, error: errorObj}
api.getErrors(typeName); // returns all errors of matching type. 

I think the promise returned by the API should almost always resolve, and almost never reject. Users will then use hasErrors, and decide what to do. This makes unhandledRejection, uncaughtException, and nonZeroExit consistent with what we do with a failed assertion (we resolve the promise with a result that indicates there were errors).

We should continue emitting events so people can implement their own realtime logging, but it should be possible to create basically the same log output after the fact using just the array returned from getErrors().

Thoughts?

@jamestalmage
Copy link
Contributor

I've shared a number of thoughts, but overall, very much 👍

@jamestalmage
Copy link
Contributor

use API in tests instead of child_process.exec

I'm not sure of this one. I like that test/cli.js exercises the full stack. It currently acts as more of an integration test.

We might be able to move the majority of them to test/api.js, but some should definitely stay behind. Enough to fully exercise cli.js.

testCount += stats.testCount;
if (cli.flags.init) {
require('ava-init')();
process.exit();
Copy link
Member

Choose a reason for hiding this comment

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

You can't process.exit() here as ava-init won't have a time to finish. Just return and XO will pass when https://github.com/sindresorhus/ava/pull/277/files#diff-b9cfc7f2cdf78a7f4b91a753d10865a2R132 lands.

@sindresorhus
Copy link
Member

We might be able to move the majority of them to test/api.js, but some should definitely stay behind.

@jamestalmage Yup, that's the plan. We still want to explicitly test the CLI, but many of the current CLI tests really just test "API" behavior.

@sindresorhus
Copy link
Member

👍 Overall looks really good!

@vadimdemedes
Copy link
Contributor Author

@jamestalmage I appreciate your suggestions, but I'd like to ask you not to modify WIP-kind of PRs. I already had a fix in development, but I had to cancel all my changes to merge yours in. Or at least communicate that with PR author. Thanks.

@jamestalmage
Copy link
Contributor

Sure no - problem. Sorry for messing you up.

@vadimdemedes
Copy link
Contributor Author

@jamestalmage Good suggestion about errors!

@vadimdemedes
Copy link
Contributor Author

I'd really like to merge this now and add TAP support for the next release. I could add/convert tests in a separate PR. @sindresorhus @jamestalmage what do you think?

@jamestalmage
Copy link
Contributor

I would wait for the release. We already have one major refactor in this release. I think that we let major changes sit on master for at least a few days before release.

@sindresorhus
Copy link
Member

I think that we let major changes sit on master for at least a few days before release.

I agree. Not really any user-facing changes here anyways.

@sindresorhus sindresorhus changed the title [WIP] Add programmatic API Add programmatic API Nov 30, 2015
@sindresorhus sindresorhus deleted the programmatic-api branch November 30, 2015 06:48
@sindresorhus
Copy link
Member

Landed! Exactly what I had in mind when I opened the issue about it. Really nice work @vdemedes :)

vadimdemedes pushed a commit that referenced this pull request Nov 30, 2015
@sindresorhus
Copy link
Member

tumblr_mlf8zt33uq1qdlh1io1_400

@vadimdemedes
Copy link
Contributor Author

@jamestalmage
Copy link
Contributor

I need to step up my GIF game.

})
.then(flatten)
.filter(function (file) {
return path.extname(file) === '.js' && path.basename(file)[0] !== '_';
Copy link

Choose a reason for hiding this comment

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

How do .jsx test files run?
@vdemedes

Debugged to find this, but unfortunately/luckily this file has been refactored by @jamestalmage
Hoping that its fixed in next release or minor release to patch this is released soon.

Copy link

@azizhk azizhk May 24, 2016

Choose a reason for hiding this comment

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

Ok the code has been moved to https://github.com/avajs/ava/blob/e01ee00/lib/ava-files.js#L248
Will see if I can account extension from files or source and raise a PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants