Add test coverage for standalone, shared and desktop side

RESOLVED FIXED in Firefox 42

Status

enhancement
P3
normal
Rank:
30
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: andreio, Assigned: andreio)

Tracking

unspecified
mozilla42
Points:
5
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox42 fixed)

Details

(Whiteboard: [tests])

Attachments

(1 attachment, 5 obsolete attachments)

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/
Posted patch Add test coverage for Loop (obsolete) — Splinter Review
Assignee: nobody → andrei.br92
Posted patch Add test coverage for Loop (obsolete) — Splinter Review
Attachment #8633800 - Attachment is obsolete: true
Attachment #8633936 - Flags: review?(mdeboer)
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-
Status: NEW → ASSIGNED
Iteration: --- → 42.2 - Jul 27
Points: --- → 5
Flags: qe-verify-
Flags: firefox-backlog+
Priority: -- → P3
Add test coverage for Loop
Attachment #8633936 - Attachment is obsolete: true
Attachment #8636819 - Flags: review?(mdeboer)
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-
(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.
Posted patch Add test coverage for Loop (obsolete) — Splinter Review
Attachment #8636819 - Attachment is obsolete: true
Attachment #8637711 - Flags: review?(mdeboer)
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 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-
Posted patch Add test coverage for Loop (obsolete) — Splinter Review
Attachment #8637711 - Attachment is obsolete: true
Attachment #8637995 - Flags: review?(mdeboer)
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+
Rank: 30
Whiteboard: [tests]
Keywords: checkin-needed
Attachment #8637995 - Attachment is obsolete: true
Iteration: 42.2 - Jul 27 → 42.3 - Aug 10
https://hg.mozilla.org/mozilla-central/rev/91164a877f8a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
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" ?
Duplicate of this bug: 989242
You need to log in before you can comment on or make changes to this bug.