-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[QueueRunner] Support "thenables" returned by tests #1270
Conversation
Am I not allowed to use |
Fixed an edge case where the Let me know if there are other issues. 👍 |
Disclaimer: I'm not related to Jasmine in any way, just an enthusiastic user. This feature is quite popular and has been discussed and PR'd many times (PR #202, #681, PR #696, #923, PR #1029), and has always been rejected with the following workarounds:
In my opinion, it would be a great addition to Jasmine:
As a side note, @aleclarson : your PR breaks the CI build because of lints reported by jshint (https://travis-ci.org/jasmine/jasmine/jobs/202125138#L380) |
@BenoitZugmeyer Thanks for the heads up! All fixed. I'll squash the commits once a maintainer speaks up about any nits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general the plan seems good for this, but there are a couple issues with the current testing strategy related browsers that don't support certain features natively.
Additionally, the build for this PR is broken for your new tests, so that would need to be fixed as well.
|
||
it("supports promises (and thenables) returned by queued functions", function(done) { | ||
var queueableFn1 = { fn: function() { | ||
return Promise.resolve(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jasmine's tests need to run in IE 8+ and PhantomJS (as well as Edge, Chrome, Firefox, and Node.js) so we can't use bare Promise
directly in specs for browsers that don't support them.
expect(completeCallback).not.toHaveBeenCalled(); | ||
expect(queueableFn2.fn).not.toHaveBeenCalled(); | ||
|
||
setImmediate(function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, setImmediate
isn't available in all browsers that Jasmine supports and as such can't be used in Jasmine's tests.
|
||
it("calls 'done.fail' when a promise is rejected", function(done) { | ||
var error, queueableFn1 = { fn: function() { | ||
return Promise.reject(error = new Error('foo')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Native Promise
as above
expect(failFn).not.toHaveBeenCalled(); | ||
expect(queueableFn2.fn).not.toHaveBeenCalled(); | ||
|
||
setImmediate(function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setImmediate
as above
This commit allows
done
ordone.fail
to be called by a returned promise/thenable.This reduces the following boilerplate:
promise.then(done)
promise.catch(done.fail)
Includes tests for resolved promises and rejected promises.
Should I include one for pending promises, too?
Example
done
function is called.done.fail
function is called.Failure tests
To test failure cases, you can
catch
the error and callexpect(error.message)
.