add isCancelled() regression tests #1239

Merged
merged 2 commits into from Jan 2, 2017

Projects

None yet

3 participants

@finnigantime
Contributor

Adding some regression tests around isCancelled(). These tests are green against bluebird master:

screen shot 2016-09-19 at 8 09 39 am

In our fork of bluebird, we moved over from bluebird2 to bluebird3 a few months ago so we're on a slightly older version of bluebird still. We have some issues with this version around cancellation, and the second unit test here isolates one of these issues - it fails in our fork:

screen shot 2016-09-19 at 8 10 10 am

Looks like we can merge the latest release of bluebird to fix the issue on our side. Adding these tests to protect this behavior.

@benjamingr

Looks good to me generally

test/mocha/cancel.js
+ });
+
+ specify("isCancelled() synchronously returns true after calling cancel() on pending promise", function() {
+ var resolver = Promise.pending();
@benjamingr
benjamingr Sep 19, 2016 Collaborator

Let's not use Promise.pending in new tests?

@finnigantime
finnigantime Sep 19, 2016 Contributor

I'm happy to clean this up. What should it be replaced with?

@benjamingr
benjamingr Sep 19, 2016 Collaborator

I think new Promise(function(){}) works fine.

test/mocha/cancel.js
+ });
+
+ specify("isCancelled() synchronously returns true after calling cancel() on promise created from .then()", function() {
+ var resolver = Promise.pending();
@benjamingr
benjamingr Sep 19, 2016 Collaborator

Same, let's not use Promise.pending in new tests

@finnigantime
Contributor

Feedback addressed.

@finnigantime
Contributor

@benjamingr Does this look ok to merge?

@finnigantime
Contributor
@petkaantonov petkaantonov merged commit d9a3a62 into petkaantonov:master Jan 2, 2017

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