1183884 - Add test coverage for standalone, shared and desktop side

Status: RESOLVED FIXED

(Hello (Loop) :: General, enhancement, P3)

Description

[Andrei Oprea [:andreio]]

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/

Comment 1

[Andrei Oprea [:andreio]]

Attachment: Add test coverage for Loop

Comment 2

[Andrei Oprea [:andreio]]

Attachment: Add test coverage for Loop

Comment 3

[Mike de Boer [:mikedeboer]]

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?

Comment 4

[Andrei Oprea [:andreio]]

Attachment: Add test coverage for standalone, shared and desktop side

Add test coverage for Loop

Comment 5

[Mike de Boer [:mikedeboer]]

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.

Comment 6

[Andrei Oprea [:andreio]]

(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.

Comment 7

[Andrei Oprea [:andreio]]

Attachment: Add test coverage for Loop

Comment 8

[Mike de Boer [:mikedeboer]]

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"

Comment 9

[Mike de Boer [:mikedeboer]]

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.

Comment 10

[Andrei Oprea [:andreio]]

Attachment: Add test coverage for Loop

Comment 11

[Mike de Boer [:mikedeboer]]

Comment on attachment 8637995 [details] [diff] [review]
Add test coverage for Loop

Review of attachment 8637995 [details] [diff] [review]:
-----------------------------------------------------------------

\o/ WFM!

Comment 12

[Andrei Oprea [:andreio]]

Attachment: Change manual testing infrastructure NPOTB, DONTBUILD

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/91164a877f8a

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/91164a877f8a

Comment 15

[Mark Banner (:standard8)]

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" ?