Closed
Bug 1183884
Opened 9 years ago
Closed 9 years ago
Add test coverage for standalone, shared and desktop side
Categories
(Hello (Loop) :: General, enhancement, P3)
Hello (Loop)
General
Tracking
(firefox42 fixed)
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: andreio, Assigned: andreio)
References
Details
(Whiteboard: [tests])
Attachments
(1 file, 5 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
Need: As a developer I want good test coverage so that my code is easy to maintain and refactor. Deliverable: Test coverage statistics via istanbul https://gotwarlost.github.io/istanbul/
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → andrei.br92
Assignee | ||
Updated•9 years ago
|
Attachment #8633936 -
Flags: review?(mdeboer)
Comment 3•9 years ago
|
||
Comment on attachment 8633936 [details] [diff] [review] Add test coverage for Loop Review of attachment 8633936 [details] [diff] [review]: ----------------------------------------------------------------- Awesome, would love to have this in! Can you add this `run_all_loop_tests.sh`? ::: browser/components/loop/test/karma/karma.conf.base.js @@ +1,1 @@ > +module.exports = function(config) { missing: creative commons license block. @@ +1,4 @@ > +module.exports = function(config) { > + return { > + > + // base path that will be used to resolve all patterns (eg. files, exclude) nit: please change all the comments to start with a capital character and end with a period (.). @@ +1,5 @@ > +module.exports = function(config) { > + return { > + > + // base path that will be used to resolve all patterns (eg. files, exclude) > + basePath: '../../', Please change all the single quotes to double quotes throughout this file. @@ +55,5 @@ > + 'karma-coverage', > + 'karma-mocha', > + 'karma-firefox-launcher' > + ] > + } missing: semi-colon. @@ +56,5 @@ > + 'karma-mocha', > + 'karma-firefox-launcher' > + ] > + } > +} missing: semi-colon. ::: browser/components/loop/test/karma/karma.coverage.desktop.js @@ +1,1 @@ > +module.exports = function(config) { All comments from 'karma.conf.base.js' apply here too. @@ +47,5 @@ > + // preprocess matching files before serving them to the browser > + // available preprocessors: https://npmjs.org/browse/keyword/karma-preprocessor > + baseConfig.preprocessors = { > + 'content/js/*.js': ['coverage'] > + } nit: missing semi-colon. @@ +50,5 @@ > + 'content/js/*.js': ['coverage'] > + } > + > + config.set(baseConfig); > +} nit: missing semi-colon. ::: browser/components/loop/test/karma/karma.coverage.shared_standalone.js @@ +1,4 @@ > +module.exports = function(config) { > + var baseConfig = require('./karma.conf.base.js')(config); > + > + // list of files / patterns to load in the browser nit: how did you end up with 4-space indentation here? @@ +38,5 @@ > + 'standalone/content/js/standaloneRoomViews.js', > + 'standalone/content/js/standaloneMetricsStore.js', > + 'standalone/content/js/webapp.js', > + 'test/shared/*.js', > + 'test/standalone/*.js', nit: trailing comma. @@ +46,5 @@ > + // available preprocessors: https://npmjs.org/browse/keyword/karma-preprocessor > + baseConfig.preprocessors = { > + 'content/shared/js/*.js': ['coverage'], > + 'standalone/content/js/*.js': ['coverage'] > + } nit: missing semi-colon. @@ +49,5 @@ > + 'standalone/content/js/*.js': ['coverage'] > + } > + > + config.set(baseConfig); > +} nit: missing semi-colon. ::: browser/components/loop/test/karma/stubs.js @@ +1,1 @@ > +// Used for desktop coverage tests because triggering methods on DOM loaded Missing: creative commons license block. @@ +1,5 @@ > +// Used for desktop coverage tests because triggering methods on DOM loaded > +// proved to lead to race conditions. > + > +sinon.stub(document, "addEventListener"); > +console.log("[stubs.js] addEventListener stubed to prevent race conditions"); nit: s/stubed/stubbed ::: browser/components/loop/test/package.json @@ +8,5 @@ > + }, > + "devDependencies": { > + "istanbul": "^0.3.17", > + "karma": "^0.12.37", > + "karma-chrome-launcher": "^0.2.0", You're not using this plugin... can it be removed? @@ +11,5 @@ > + "karma": "^0.12.37", > + "karma-chrome-launcher": "^0.2.0", > + "karma-coverage": "^0.4.2", > + "karma-firefox-launcher": "^0.1.6", > + "karma-growl-reporter": "^0.1.1", Doesn't look like you're using this. Can you remove it?
Attachment #8633936 -
Flags: review?(mdeboer) → review-
Updated•9 years ago
|
Status: NEW → ASSIGNED
Updated•9 years ago
|
Iteration: --- → 42.2 - Jul 27
Points: --- → 5
Flags: qe-verify-
Flags: firefox-backlog+
Updated•9 years ago
|
Priority: -- → P3
Assignee | ||
Comment 4•9 years ago
|
||
Add test coverage for Loop
Attachment #8633936 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8636819 -
Flags: review?(mdeboer)
Comment 5•9 years ago
|
||
Comment on attachment 8636819 [details] [diff] [review] Add test coverage for standalone, shared and desktop side Review of attachment 8636819 [details] [diff] [review]: ----------------------------------------------------------------- Almost there!! Only thing I'd like to see is a bit more onboarding-friendly invocation in run-all-loop-tests.sh. ::: browser/components/loop/run-all-loop-tests.sh @@ +21,5 @@ > fi > echo 'eslint run finished.' > fi > > +# Build tests coverage nit: missing dot. @@ +22,5 @@ > echo 'eslint run finished.' > fi > > +# Build tests coverage > +(cd ${LOOPDIR}/test && npm run build-coverage) When this command fails, can you make it fail with a more elegant message? Like, 'Make sure all dependencies are up to date by running 'npm install' inside the 'test/' directory.' ::: browser/components/loop/test/karma/stubs.js @@ +1,4 @@ > +/* Any copyright is dedicated to the Public Domain. > + * http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +// Used for desktop coverage tests because triggering methods on DOM loaded Do you mean 'on DOMReady'? ::: browser/components/loop/test/package.json @@ +17,5 @@ > + "build-coverage-shared": "./node_modules/.bin/karma start karma/karma.coverage.shared_standalone.js", > + "build-coverage-desktop": "./node_modules/.bin/karma start karma/karma.coverage.desktop.js", > + "build-coverage": "npm run build-coverage-desktop && npm run build-coverage-shared" > + }, > + "author": "Andrei Oprea @ndreio" I don't think this bit is strictly necessary :) Your street creds are in the blame history already.
Attachment #8636819 -
Flags: review?(mdeboer) → review-
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to Mike de Boer [:mikedeboer] from comment #5) > Comment on attachment 8636819 [details] [diff] [review] > Add test coverage for standalone, shared and desktop side > > Review of attachment 8636819 [details] [diff] [review]: > ----------------------------------------------------------------- > ::: browser/components/loop/test/karma/stubs.js > @@ +1,4 @@ > > +/* Any copyright is dedicated to the Public Domain. > > + * http://creativecommons.org/publicdomain/zero/1.0/ */ > > + > > +// Used for desktop coverage tests because triggering methods on DOM loaded > > Do you mean 'on DOMReady'? It's DOMContentLoaded actually.
Assignee | ||
Updated•9 years ago
|
Attachment #8637711 -
Flags: review?(mdeboer)
Comment 8•9 years ago
|
||
Comment on attachment 8637711 [details] [diff] [review] Add test coverage for Loop Review of attachment 8637711 [details] [diff] [review]: ----------------------------------------------------------------- Great! Ship it! ::: browser/components/loop/run-all-loop-tests.sh @@ +23,5 @@ > fi > > +# Build tests coverage. > +MISSINGDEPSMSG="Make sure all dependencies are up to date by running > +'npm install' inside the 'test/' directory." nit: can you make this even more awesome to say: "\nMake sure all dependencies are up to date by running 'npm install' inside the 'browser/components/loop/test/' directory.\n"
Attachment #8637711 -
Flags: review?(mdeboer) → review+
Comment 9•9 years ago
|
||
Comment on attachment 8637711 [details] [diff] [review] Add test coverage for Loop Woa, I just noticed that now when I run `run_all_loop_tests.sh`, it _always_ exits right after the karma run. Please fix that.
Attachment #8637711 -
Flags: review+ → review-
Assignee | ||
Updated•9 years ago
|
Attachment #8637995 -
Flags: review?(mdeboer)
Comment 11•9 years ago
|
||
Comment on attachment 8637995 [details] [diff] [review] Add test coverage for Loop Review of attachment 8637995 [details] [diff] [review]: ----------------------------------------------------------------- \o/ WFM!
Attachment #8637995 -
Flags: review?(mdeboer) → review+
Updated•9 years ago
|
Rank: 30
Updated•9 years ago
|
Whiteboard: [tests]
Assignee | ||
Comment 12•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•9 years ago
|
Attachment #8637995 -
Attachment is obsolete: true
Comment 13•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/91164a877f8a
Keywords: checkin-needed
Updated•9 years ago
|
Iteration: 42.2 - Jul 27 → 42.3 - Aug 10
https://hg.mozilla.org/mozilla-central/rev/91164a877f8a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Comment 15•9 years ago
|
||
Can we be more descriptive/careful with our check-in comments in future please - "Change manual testing infrastructure" doesn't mean anything to me, was it mean to be "Add test coverage for Loop" ?
You need to log in
before you can comment on or make changes to this bug.
Description
•