Add AggregateError to .any() in supported environments #25
Conversation
|
I missed some semicolons in the tests, I'll fix those in a second |
index.spec.js
Outdated
| @@ -842,6 +842,43 @@ describe("synchronous-promise", function () { | |||
| expect(capturedError).property("errors").to.contain("123"); | |||
| }); | |||
|
|
|||
| it("should reject with AggregateError if supported", function () { | |||
There was a problem hiding this comment.
minor: suggest putting this test in its own describe block and having the mocked AggregateError set up in a beforeEach and torn down in an afterEach -- covers the situation where the test, for some reason, fails.
There was a problem hiding this comment.
I'll give that a go, the only tiny problem I see is that this would never test the code without the AggregateError available.
There was a problem hiding this comment.
Nevermind, if I nest the beforeEach in the describe it is scoped to that test.
There was a problem hiding this comment.
I would call the test "static any (browsers with AggregateError)", do you have a better name for that?
|
I moved the test into a nested describe and put the mocking into hooks. I also split up the test to make it more concice and descriptive, as the two cases affect different parts of the code. |
|
My only request is to refactor out a function for the repeated logic of generating the correct Error type. Shouldn't take long. Please ping me when you've done so. |
This adds the AggregateError spec to the
SynchronousPromise.anymethod. (Promise.any)If
windowandwindow.AggregateErrorare defined, the method rejects with an instance ofAggregateError.I disabled JSHint for both of the blocks to allow the undefined
windowvariable to be safely used after the checks.The tests define a simple mock for the error and check if that implementation was used correctly.
This closes #24