Adding tests for the module #1
Conversation
There was a problem hiding this comment.
Nice. It seems it does not test one essential aspect: it should resolve after all given promises have settled, not earlier. Perhaps we could manually resolve promises to test this; perhaps like so?:
let resolvePromise1
let rejectPromise2
const promises = [
new Promise((resolve, reject) => { resolvePromise1 = resolve }),
new Promise((resolve, reject) => { rejectPromise2 = reject }),
]
const thenableFunc = jest.fn()
const combinedPromise = whenAllSettled(promises).then(thenableFunc)
expect(thenableFunc).toHaveBeenCalledTimes(0)
resolvePromise1()
expect(thenableFunc).toHaveBeenCalledTimes(0)
rejectPromise2()
expect(thenableFunc).toHaveBeenCalledTimes(1)
I'm not sure however whether/how we should account for the possibility that a then/catch function is not called immediately. If a Promise implementation would schedule it to happen on a next tick or something, our tests would be wrong. Worth a bit of research to figure out whether the Promise specification guarantees synchronous invocation of these functions, and if not what would be an easy way to give them a moment to invoke their callbacks (maybe add delays into the tests?).
package.json
Outdated
| @@ -23,6 +26,7 @@ | |||
| "babel-preset-stage-1": "^6.24.1" | |||
| }, | |||
| "dependencies": { | |||
| "jest": "^20.0.4", | |||
There was a problem hiding this comment.
Should be a dev-dependency. yarn add --dev jest.
__test__/index.test.js
Outdated
| @@ -0,0 +1,62 @@ | |||
| /* eslint-env jest */ | |||
|
|
|||
| import whenAllSettled from '../src/.' | |||
There was a problem hiding this comment.
(no need for the . at the end, looks a bit funny; technically there's no need for the / either, but I think that is fine to keep)
__test__/index.test.js
Outdated
| const promises = [] | ||
| promises.push(new Promise((resolve, reject) => { | ||
| reject(new Error('Failed Promise')) | ||
| })) |
There was a problem hiding this comment.
They made Promise.reject and Promise.resolve as shorthands exactly for this usage (docs). So it could just be e.g. const promises = [Promise.reject(new Error('Failed Promise'))]
__test__/index.test.js
Outdated
| describe('whenAllSettled', () => { | ||
| test('should call console.error if no rejection handler is provided', async () => { | ||
| const promises = [] | ||
| promises.push(Promise.reject(new Error('Failed Promise'))) |
There was a problem hiding this comment.
For readability: there seems no reason to use .push. I'd put the promises directly in the const definition.
Otherwise, looks all good to me. I suggest we merge it now and we can add more test(s) later.
Sure thing, I will keep working on how to work the tests better. For now these seem adequate to keep the module stable. |
I would have preferred to have the test side by side of the index file but due to the increased simplicity of the babel configuration and its nature to pull all the js files into build, I have put the test in its own directory.