[COMPASS-1785] Support Electron Renderer Tests #11
Conversation
…lectron-renderer tests via Karma. Updated package.json with new dependencies and new test command for Karma. Added new webpack config for Karma.
There was a problem hiding this comment.
Comments inline, and one more general question:
Will the renderer tests be executed with npm test? I think not, which is a behavioral change to what we previously had. That means they would not be executed by travis and evergreen currently. Maybe we hook them into the ci script, e.g. npm run check && npm test && npm run test:karma? (npm run ci is what travis/evergreen call).
template/karma.conf.js
Outdated
| const webpackConfig = require('./config/webpack.karma.config'); | ||
|
|
||
| module.exports = function(config) { | ||
| config.set({ |
There was a problem hiding this comment.
Are those tabs? can you switch your editor to use 2 spaces for indentation?
There was a problem hiding this comment.
- Fix code style
template/src/actions/Actions.js
Outdated
| ]); | ||
|
|
||
| export default {{pascalcase name}}Actions; | ||
| export { {{pascalcase name}}Actions }; |
There was a problem hiding this comment.
needs newline at end of file or npm run check won't pass.
There was a problem hiding this comment.
- Fix code style
|
|
||
| module.exports = {{pascalcase name}}Store; | ||
| export default {{pascalcase name}}Store; | ||
| export { {{pascalcase name}}Store }; |
There was a problem hiding this comment.
Newline at end of file.
There was a problem hiding this comment.
- Fix code style
template/src/stores/Store.js
Outdated
| @@ -0,0 +1,92 @@ | |||
| import Reflux from 'reflux'; | |||
There was a problem hiding this comment.
(comment about the filename) What's the convention with uppercase vs lowercase file names? Before, we typically used all lowercase. I thought uppercase was just for React components but this is now for a store.
There was a problem hiding this comment.
@rueckstiess In the case of React it's an Eslint AirBnb code style guide requirement to use PascalCase for imports in the case of classes (http://airbnb.io/javascript/#naming--PascalCase). I assumed (possibly incorrectly) that the default import of Reflux was also a class, or at least an instantiated object.
TL;DR, I don't have a good reason for or against. Based on our code style guides what would you prefer the syntax to be?
There was a problem hiding this comment.
Discussed amongst Sydney devs and majority would prefer to keep all file names slug-case (i.e. all lower-case and with dashes), rather than converting to PascalCase for classes.
Reasons for the change:
- reduced mental mapping from slug-case to PascalCase (the component name)
- webpack can warn of case-insensitive imports
Reasons against change:
- all other mongodb-js modules follow this convention
- potential issues with case-insensitive file systems
There was a problem hiding this comment.
- Update the filenames (use
git mvto achieve this otherwise git won't recognise the change in name in the case of case insensitive filesystems like Mac OSX)
template/test/renderer/Store.spec.js
Outdated
| import Store from 'stores'; | ||
|
|
||
| describe('{{pascalcase name}}Store [Store]', () => { | ||
| beforeEach(function() { |
There was a problem hiding this comment.
these tests are just a copy of the Store.spec.js file, right? Might be confusing to users of this plugin. Can we think of a test that actually needs to be executed in the renderer process instead?
There was a problem hiding this comment.
@rueckstiess I agree, and given more time I could probably come up with a more contrived example to better demonstrate, but the important thing was simply having something there to test to demonstrate the Karma-Electron renderer tests run and pass.
If you're ok with leaving this as is for now, I can address this comment with a better example as part of another PR when I have more time and fewer higher priority items to get through. How does that sound?
There was a problem hiding this comment.
ok, let's leave as is. But could you add a comment on the top of the file, something like this (feel free to change the wording).
/*
* Place tests that must run in a renderer context inside Electron here.
*
* Note: The tests below are just a copy of the store unit tests as an example.
* More complex plugins will require actual renderer/integration tests to be
* executed here.
*/
There was a problem hiding this comment.
- Add comment
@rueckstiess Yeah that's a good idea, I'll add the comment and then return to this in another PR.
… correct coding style settings that support editorconfigs.
…ussion on naming convention.
…iscussed with the team.
This PR adds support for running tests in an electron renderer context inside electron by utilising Karma.
Previously this support was provided by hadron-build, via electron-mocha - but given we now have a more complicated pre-compilation step requiring webpack, electron-mocha doesn't facilitate a means for us to achieve this. Hence the switch to Karma (facilitated by karma-webpack and karma-electron).
Renderer tests (for the time being) live under
test/rendereras there is otherwise an issue with chai-enzyme usage within our current unit tests that are run with mocha-webpack. A subsequent PR will be raised to solve this issue, and allow us to run our entire test suite through the electron-renderer as well.Down the road, testing should hopefully be centralised back to hadron-build once all existing plugins have been upgraded to the new template. This is a stop gap on the road to achieving that goal.
As part of this PR I also completed the following chores:
index.jsinstoresandactionsnow is simply an import/export file, the implementation of an action/store has been moved to its own file to align with new documentation on best practices.test/renderer.npm run test:karmato package.json.