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

Feature: promises #537

Merged
merged 12 commits into from Feb 28, 2016

Conversation

@rluba
Copy link
Contributor

rluba commented Feb 27, 2016

Implements #536 and fixes two bugs related to naming tests and setup / teardown blocks at the root level (outside any experiment).


finish(err, 'done');
});
if (itemResult && itemResult.then instanceof Function) {
itemResult.then(() => finish(null, 'done'), (err) => finish(err, 'done'));

This comment has been minimized.

Copy link
@Marsup

Marsup Feb 27, 2016

Member

2nd arg is only used in case of error.

This comment has been minimized.

Copy link
@rluba

rluba Feb 27, 2016

Author Contributor

Thx


finish(err, 'done');
});
if (itemResult && itemResult.then instanceof Function) {
itemResult.then(() => finish(null), (err) => finish(err, 'done'));

This comment has been minimized.

Copy link
@Marsup

Marsup Feb 27, 2016

Member

Thought it was obvious but you don't really need null either, you could just .then(finish ;)

This comment has been minimized.

Copy link
@rluba

rluba Feb 27, 2016

Author Contributor

I thought you might mean that – but that would be incorrect. If the promise resolves with a value, then(finish) would pass the resolve value to finish, which would interpret it as an error!

This comment has been minimized.

Copy link
@Marsup

Marsup Feb 27, 2016

Member

Correct, nvm then.

@Marsup

This comment has been minimized.

Copy link
Member

Marsup commented Feb 27, 2016

You'll need tests with multiple it to check promises are not capturing anything.
I know the code should work but it's worth testing to avoid regressions.

@rluba

This comment has been minimized.

Copy link
Contributor Author

rluba commented Feb 27, 2016

Seems like travis hit a timeout on 6c7df4a.

@rluba

This comment has been minimized.

Copy link
Contributor Author

rluba commented Feb 27, 2016

to check promises are not capturing anything.

@Marsup, what do you mean by that?

@Marsup

This comment has been minimized.

Copy link
Member

Marsup commented Feb 27, 2016

Promises wrap everything in a try catch, so if finish failed synchronously the promise inside your test would catch it and you'd never know. It's prevented by the setImmediate of finish.

@rluba

This comment has been minimized.

Copy link
Contributor Author

rluba commented Feb 27, 2016

You’re right. Proper promise libraries like Bluebird have built-in protection against such mistakes but the native Promise does not.

Any suggestions on how to test for that without pulling in Bluebird or something similar?

@Marsup

This comment has been minimized.

Copy link
Member

Marsup commented Feb 27, 2016

For lab's small scope I'd suggest just testing that synchronous throws don't leak into any previous promise returning before/after/it.

rluba added 2 commits Feb 27, 2016
…romise chain of the previous test

Thanks to @Marsup for the suggestion.
@rluba

This comment has been minimized.

Copy link
Contributor Author

rluba commented Feb 27, 2016

Sorry for the sloppy commits. I accidentally pushed my test of native promise behavior on unhandled rejections. Removed it in 411dc96.

@geek geek added the feature label Feb 27, 2016
const settings = Utils.mergeOptions(this._current.options, options, ['only']);

const test = {
path: this._path,
title: this._titles.join(' ') + ' ' + title,
title: (this._path.length ? this._titles.join(' ') + ' ' : '') + title,

This comment has been minimized.

Copy link
@geek

geek Feb 27, 2016

Member

is this meant to be this_path.length or this._titles.length ?

This comment has been minimized.

Copy link
@rluba

rluba Feb 28, 2016

Author Contributor

It’s equivalent, these arrays alway have the same length. I can change it, if you think it’s confusing.

This comment has been minimized.

Copy link
@Marsup

Marsup Feb 28, 2016

Member

Isn't it easier to just do this._titles.concat(title).join(' ') ?

This comment has been minimized.

Copy link
@rluba

rluba Feb 28, 2016

Author Contributor

Done.

@geek geek self-assigned this Feb 27, 2016
@geek geek added this to the 10.1.0 milestone Feb 27, 2016
@Marsup

This comment has been minimized.

Copy link
Member

Marsup commented Feb 28, 2016

Maybe we should increase the timeout on this test, it keeps failing on travis.

@rluba

This comment has been minimized.

Copy link
Contributor Author

rluba commented Feb 28, 2016

@Marsup Not caused by this PR, but I can piggyback a fix.

rluba added 2 commits Feb 28, 2016
… because they keep failing on travis CI
@rluba

This comment has been minimized.

Copy link
Contributor Author

rluba commented Feb 28, 2016

The JSON reporter test takes about a second on my machine, so I can understand that travis surpasses the 3s mark frequently.

@@ -1116,7 +1116,6 @@ describe('Reporter', () => {
expect(result.lint[0].filename).to.exist();
expect(result.lint[0].errors).to.exist();
done();
done = function () {};

This comment has been minimized.

Copy link
@Marsup

Marsup Feb 28, 2016

Member

It's been done on purpose by 3e14d48. @geek was that a mistake ?

This comment has been minimized.

Copy link
@rluba

rluba Feb 28, 2016

Author Contributor

I know, but I see no way for it to be relevant unless there’s a bug in Lab.report that causes it to call its callback more than once.

And if such a case exists in Lab.report, I’d prefer this test to fail violently instead of shadowing the problem. Or did I miss something?

This comment has been minimized.

Copy link
@geek

geek Feb 28, 2016

Member

thanks, removing the setting of done is a good move

geek added a commit that referenced this pull request Feb 28, 2016
Feature: promises
@geek geek merged commit a7363c5 into hapijs:master Feb 28, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.