Add unit and basic snapshot tests for two components from common folder #33
Conversation
lotrien
changed the title
lotrien
force-pushed
the
tests-for-common
branch
from
02e252e to
b494f8c
Compare
ikalnytskyi
left a comment
ikalnytskyi
left a comment
There was a problem hiding this comment.
Looks almost good to me except for irrelevant package-lock.json changes. Could you please remove them? Also, see other comments inline.
| "eslint-plugin-jsx-a11y": "^6.0.2", | ||
| "eslint-plugin-react": "^7.4.0", | ||
| "extract-text-webpack-plugin": "^3.0.2", | ||
| "fetch-mock": "^6.0.0-beta.2", |
testSetup.js
Outdated
| import { configure } from 'enzyme'; | ||
| import Adapter from 'enzyme-adapter-react-16'; | ||
|
|
||
| configure({ adapter: new Adapter() }); |
There was a problem hiding this comment.
Please, add few lines that explains what is it and why we need it.
| "./testSetup.js" | ||
| ], | ||
| "moduleNameMapper": { | ||
| "^.+\\.(css|styl|svg)$": "identity-obj-proxy" |
package.json
Outdated
| ], | ||
| "jest": { | ||
| "setupFiles": [ | ||
| "./testSetup.js" |
There was a problem hiding this comment.
Hm.. why start with dot? Why not tests/testSetup.js?
There was a problem hiding this comment.
hm why not, about to move to test folder and fix this line.But anyway with changed directory path without dot won't work
package-lock.json
Outdated
| "version": "8.0.2", | ||
| "resolved": "https://registry.npmjs.org/yargs/-/yargs-8.0.2.tgz", | ||
| "integrity": "sha1-YpmpBVsc78lp/355wdkY3Osiw2A=", | ||
| "version": "10.0.3", |
There was a problem hiding this comment.
there are a lot of irrelevant changes in package-lock.json. could you please remove them?
lotrien
force-pushed
the
tests-for-common
branch
2 times, most recently
from
8c3a0d4 to
0f9d443
Compare
ikalnytskyi
left a comment
ikalnytskyi
left a comment
There was a problem hiding this comment.
One iteration and we are good :D
package.json
Outdated
| }, | ||
| "dependencies": { | ||
| "codemirror": "^5.33.0", | ||
| "enzyme": "^3.3.0", |
There was a problem hiding this comment.
I believe enzyme should be in devDependencies, shouldn't it?
There was a problem hiding this comment.
As well as enzyme-adapter-react-16.
There was a problem hiding this comment.
I also think so, but our linter doesn't
There was a problem hiding this comment.
No way. Just tried to move it under devDependencies - everything works fine.
tests/testSetup.js
Outdated
|
|
||
| // To make enzyme work we need to configure Adapter. According | ||
| // to the doc this Adapter have additional peer dependencies, | ||
| // in our case these dependencies are react, react-dom and react-test-renderer. |
There was a problem hiding this comment.
I don't think there's much sense in mentioning of peer dependencies – they may change in future while this comment will be here. :P
Also, I'd somehow rewrite it to say:
// Enzyme is React version agnostic and can work with any version through
// adapters system. Enzyme Adapter is a sort of engine that knows how to
// manipulate underlying UI library (it can be used not only with React).
// Since we use React 16, we need to set a proper adapter.
| @@ -0,0 +1,11 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
Can you please move component tests into tests/components/Spinner.test.jsx ? I mean ,let's try to follow file structure we have in src/ in order to make it easier to navigate.
lotrien
force-pushed
the
tests-for-common
branch
from
0f9d443 to
20fcbdd
Compare
Since we have to provide reliable components there is a need to cover them with tests. This commit contains tests for Title and Spinner components.
lotrien
force-pushed
the
tests-for-common
branch
from
20fcbdd to
bc2a774
Compare
Since we have to provide reliable components there is a need to cover
them with tests. This commit contains tests for Title and Spinner
components.