Write tests with jest (Coverage 100%) #19
Conversation
Update to upstream
…dd .prettierrc.js, prettify tests
|
Hola, is anybody out there % )? |
There was a problem hiding this comment.
Hey @timhagn. I'm a new collaborator here.
First of all I'd say thank you for this contribution. It adds a really good value on it since the idea is keep adding functionality, adding support for emotion/glamour and having a good test suit is the best way to start.
I made some comments to refine a bit the PR but in general looks really good! nice job
Keep in mind my suggestion are not final, we can of course discuss them if you have a different opinion.
| module.exports = { presets }; | ||
| const testPresets = [ | ||
| [ | ||
| "@babel/react", |
There was a problem hiding this comment.
Why @babel/react?
In Jest docs they recommend to use @babel/preset:
There was a problem hiding this comment.
@babel/preset-env is for advanced ES6+ transpilation @babel/react does the same for JSX, etc. (react-test-renderer), see: https://jestjs.io/docs/en/tutorial-react
| @@ -0,0 +1,7 @@ | |||
| module.exports = { | |||
There was a problem hiding this comment.
all options in this file are redundant with prettier defaults but singleQuote.
Maybe you can remove those and only keep singleQuote
There was a problem hiding this comment.
Have to say I have no clue anymore, from which of my older projects I copied the prettierrc ; )...
|
|
||
| describe(`pxToEm()`, () => { | ||
| it('pxToEm to equal defaultBreakpoints converted to em', () => { | ||
| expect(pxToEm(defaultBreakpoints, 16)).toEqual({ |
There was a problem hiding this comment.
suggestion: In this file, all those .toEqual could be replaced to .toMatchInlineSnapshot. This would bring the same value and capability to change/refactor as doing manually.
There was a problem hiding this comment.
I once was a fan of snapshot testing, but wouldn't recommend it in this case - especially when thinking about CI/CD. .toMatch[Inline]Snapshot() gets overwritten with each deployment, that's why I rather equality tests than them.
| @@ -0,0 +1,183 @@ | |||
| // For a detailed explanation regarding each configuration property, visit: | |||
There was a problem hiding this comment.
Would be nice having some clean up in this file and keep only what's been setup.
There was a problem hiding this comment.
Yeah, I concur, I just took the template and adapted it (after all, this should once been extended ; ).
| * @param {'rem' | 'em'} unit | ||
| */ | ||
| function pxToEmOrRem(breakpoints, ratio = 16, unit) { | ||
| function pxToEmOrRem(breakpoints, ratio, unit) { |
There was a problem hiding this comment.
why the default ratio was removed? It might be a breaking change?
| * @param {Object} breakpoints - Map labels to breakpoint sizes | ||
| * @return {string|*} | ||
| */ | ||
| function getSizeFromBreakpoint(breakpointValue, breakpoints) { |
There was a problem hiding this comment.
Also here, why the default breakpoints was been removed?
There was a problem hiding this comment.
Same as above. We don't have to fear breakpoints could ever be empty, as generateMedia gets called with (breakpoints = defaultBreakpoints) when no arguments are given. Also, with them set 100% coverage would never have been possible, as their branches were never taken cause of this ; ).
| @@ -0,0 +1,173 @@ | |||
| import 'jest-styled-components'; | |||
There was a problem hiding this comment.
I think this test suit is testing more than it should be.
In a nutshell, creating react components and check if the styles were applied to them is a task for styled component itself, since we're using a method from them.
The library:
- returns an object with some methods already configured;
- each function of this is calling
styled-components/csstag template literals with a string
In that sense, it might be a cleaner implementation:
- remove all React stuffs;
- mock
cssfromstyled-component - Check if it has been called with the case template.
What do you think about this?
There was a problem hiding this comment.
Sounds like more work than I can invest at the moment ; ). Gonna try looking into it, soon, anyhow.
|
Hi there, welcome to the collaborator club : ). Am in the suggestions - but keep in mind that I wrote this PR three quarts of a year ago ; ). Best, Tim. |
As written in the now closed #15, I updated the tests in regard to the changes made to this repo in the meantime.
As the default values for the internal functions
pxToEmOrRem()andgetSizeFromBreakpointwere never used, I removed them.Now the tests are at 100% and I only added packages as devDependencies : ).
Is this now mergeable ; )?
Best,
Tim.