Add tests for utils #27
Conversation
jimbo
force-pushed
the
jimbo/gallery-tests
branch
from
cad46c0 to
37020ab
Compare
theme/src/__tests__/utils.spec.js
Outdated
| return { foo: 'foo', default: 'bar' }; | ||
| } | ||
|
|
||
| describe('extract', () => { |
There was a problem hiding this comment.
Can we drop the describe so we're matching our tests across our other repos?
test('extract retrieves a named export');
test('extract retrieves the default export');There was a problem hiding this comment.
Sure. I think when this file has a few more functions the test output will be unreadable if not grouped by describe, but then, they're just tests.
| * | ||
| * @param {object} obj - A module's namespace object | ||
| * @param {string} name - The key to look up | ||
| * @param {string} name - The binding to retrieve |
There was a problem hiding this comment.
If we're listing the params, would make sense to list the return (then VSCode/Webstorm will give proper hinting).
@returns {Promise<*>}
| @@ -0,0 +1,12 @@ | |||
| { | |||
There was a problem hiding this comment.
Not a fan of having 2 separate Babel configuration files. We should choose 1 and confine all babel-related config to it
There was a problem hiding this comment.
If you want to keep the JS version, jestjs/jest#1468 (comment)
There was a problem hiding this comment.
Yeah, I'm also not a fan of it. I considered moving everything to .babelrc, but it's pretty verbose that way. Could still be better than hacking jest, though.
There was a problem hiding this comment.
Whatever route you choose is just temporary until Babel 7, and then we have first-class support for .babelrc.js
There was a problem hiding this comment.
Let's keep 2 files instead of using a hack. More maintenance, but the maintenance burden is obvious.
theme/src/__tests__/utils.spec.js
Outdated
| test('yields undefined', () => { | ||
| const gen = fixedObserver(2); | ||
|
|
||
| expect.assertions(3); |
There was a problem hiding this comment.
Any particular reason you're using an assertion counter? Typically only needed for async code where the "bad" path would exit without throwing.
Example:
test('Should throw', async () => {
try {
await foo();
// if foo does not reject, test passes (incorrectly)
} catch(err) {
expect(err).toBeTruthy();
}
});There was a problem hiding this comment.
Nope, no reason. Saw it in other tests @zetlen wrote and imitated it without opinion.
There was a problem hiding this comment.
It's not a huge deal here (small tests), but I have a strong pref in general to not use it when it isn't necessary. Last employer started having 50+ line tests, and any test you edited always failed because you'd forget to change the assertion counter - became a PITA.
There was a problem hiding this comment.
Yeah, I only add it when during my development of the test, it somehow passed without running my async assertions. I probably shouldn't do it--I should add expect.hasAssertions() for asserting any amount of later async assertions, which needs less upkeep.
| @@ -0,0 +1,12 @@ | |||
| { | |||
There was a problem hiding this comment.
Let's keep 2 files instead of using a hack. More maintenance, but the maintenance burden is obvious.
theme/src/__tests__/utils.spec.js
Outdated
| test('throws if the module fails to resolve', async () => { | ||
| const error = new Error('Invalid namespace object provided.'); | ||
|
|
||
| await expect(extract(null)).rejects.toEqual(error); |
There was a problem hiding this comment.
Testing rejections has been really annoying in Jest, but since this was fixed in 22, you can write a nicer test here:
test('throws if the module fails to resolve', () =>
expect(extract(null)).rejects.toEqual(error)
);(You don't have to async/await if you can return the Promise directly out of the test.)
There was a problem hiding this comment.
Sure, I can remove the superfluous await.
jimbo
force-pushed
the
jimbo/gallery-tests
branch
from
7c76d3f to
d572047
Compare
No description provided.