1309496 - Set up mocha testing framework for netmonitor

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Ricky Chien [:rickychien] (inactive)]

We'll start writing unit tests at the beginning to ensure react component render as expected.

React component should be stable enough once it's complete, so it makes sense to write enzyme unit tests after creating a react component.

Comment 1

[[e-mail]]

Attachment: Bug 1309496 - Set up mocha testing framework for netmonitor,

Review commit: https://reviewboard.mozilla.org/r/89204/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/89204/

Comment 2

[[e-mail]]

Comment on attachment 8805464 [details]
Bug 1309496 - Set up mocha testing framework for netmonitor,

I referenced new webconsole and setup the enzyme test for netmonitor. To run enzyme tests, you need run

1. cd devtools/client/netmonitor && npm install
2. npm test

Any file named as `**.test.js` within netmonitor/test folder will be treat as a mocha test.

I also made a simple test case for netmonitor/components/filter-buttons.js to validate if the test framework works.

Comment 3

[[e-mail]]

Comment on attachment 8805464 [details]
Bug 1309496 - Set up mocha testing framework for netmonitor,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89204/diff/1-2/

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8805464 [details]
Bug 1309496 - Set up mocha testing framework for netmonitor,

https://reviewboard.mozilla.org/r/89204/#review88364

Great progress on this!

F+

See my inline comments

Honza

::: devtools/client/netmonitor/package.json:14
(Diff revision 1)
> +    "expect": "^1.16.0",
> +    "jsdom": "^9.4.1",
> +    "jsdom-global": "^2.0.0",
> +    "mocha": "^2.5.3",
> +    "require-hacker": "^2.1.4",
> +    "sinon": "^1.17.5"

Do you have specific use cases/plans what to use Sinon for?

::: devtools/client/netmonitor/package.json:18
(Diff revision 1)
> +    "require-hacker": "^2.1.4",
> +    "sinon": "^1.17.5"
> +  },
> +  "scripts": {
> +    "postinstall": "cd ../ && npm install && cd netmonitor",
> +    "test": "NODE_PATH=`pwd`/../../../:`pwd`/../../../devtools/client/shared/vendor/ mocha test/**/*.test.js --compilers js:babel-register -r jsdom-global/register -r ./test/requireHelper.js"

I am seeing following error when running this on Win10:

'NODE_PATH' is not recognized as an internal or external command

(also pwd is not available on win)


Also, shoould 'react-dev' module be part of the dependency list? (it's used by the require-helper.js module)

::: devtools/client/netmonitor/test/components/filter-buttons.test.js:12
(Diff revision 1)
> +const { render } = require("enzyme");
> +const { createFactory } = require("devtools/client/shared/vendor/react");
> +const FilterButtons = createFactory(require("devtools/client/netmonitor/components/filter-buttons"));
> +const I = require("devtools/client/shared/vendor/immutable");
> +
> +const FiltersTypes = I.Record({

nit: Should be renamed to `FilterTypes` to correspond to `FilterButtons` and also to how filterTypes are named in test/components/filter-buttons.test.js

::: devtools/client/netmonitor/test/requireHelper.js:1
(Diff revision 1)
> +/* Any copyright is dedicated to the Public Domain.

nit: The file name should follow our code-style rules i.e. should be: require-helper.js

Comment 5

[[e-mail]]

Comment on attachment 8805464 [details]
Bug 1309496 - Set up mocha testing framework for netmonitor,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89204/diff/2-3/

Comment 6

[[e-mail]]

Comment on attachment 8805464 [details]
Bug 1309496 - Set up mocha testing framework for netmonitor,

The updated PR use real store (but use create-store mockup which does not support middleware) therefore we can test both presentation and container component behaviour.

It also use shx for cross platform `pwd` support.

I need opinion if its better to test in react-redux integration way  https://reviewboard.mozilla.org/r/89204/diff/3/

or to test in react unit test way
https://reviewboard.mozilla.org/r/89204/diff/1/

Comment 7

[Jarda Snajdr [:jsnajdr]]

I'm getting error that the "chrome" module isn't found - see below.

Two things:
- do you plan to load the l10n strings from the *.properties files before this lands?
- nit: files in the fixtures directory should use lowercase-convention-with-dashes.js

The error:

Jaroslavs-MBP:netmonitor jsnajdr$ npm test

> netmonitor@0.0.1 test /Users/jsnajdr/src/gecko-dev/devtools/client/netmonitor
> NODE_PATH=`shx pwd`/../../../:`shx pwd`/../../../devtools/client/shared/vendor/ mocha test/**/*.test.js --compilers js:babel-register -r jsdom-global/register -r ./test/requireHelper.js

[BABEL] Note: The code generator has deoptimised the styling of "/Users/jsnajdr/src/gecko-dev/devtools/client/shared/vendor/react-dev.js" as it exceeds the max of "100KB".
[BABEL] Note: The code generator has deoptimised the styling of "/Users/jsnajdr/src/gecko-dev/devtools/client/shared/vendor/immutable.js" as it exceeds the max of "100KB".
module.js:442
    throw err;
    ^

Error: Cannot find module 'chrome'
    at Function.Module._resolveFilename (module.js:440:15)
    at Function._module2.default._resolveFilename (/Users/jsnajdr/src/gecko-dev/devtools/client/netmonitor/node_modules/require-hacker/babel-transpiled-modules/require hacker.js:423:34)
    at Function.Module._load (module.js:388:25)
    at Module.require (module.js:468:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (/Users/jsnajdr/src/gecko-dev/devtools/client/netmonitor/request-utils.js:3:16)
    ...

Comment 8

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8805464 [details]
Bug 1309496 - Set up mocha testing framework for netmonitor,

https://reviewboard.mozilla.org/r/89204/#review88860

::: devtools/client/netmonitor/test/components/filter-buttons.test.js:17
(Diff revision 3)
> +const FilterButtons = createFactory(require("devtools/client/netmonitor/components/filter-buttons"));
> +
> +// unit test
> +describe("FilterButtons component:", () => {
> +  const store = configureStore();
> +  const expected = `<button id="requests-menu-filter-all-button" class="menu-filter-button checked" data-key="all">netmonitor.toolbar.filter.all</button><button id="requests-menu-filter-html-button" class="menu-filter-button" data-key="html">netmonitor.toolbar.filter.html</button><button id="requests-menu-filter-css-button" class="menu-filter-button" data-key="css">netmonitor.toolbar.filter.css</button><button id="requests-menu-filter-js-button" class="menu-filter-button" data-key="js">netmonitor.toolbar.filter.js</button><button id="requests-menu-filter-xhr-button" class="menu-filter-button" data-key="xhr">netmonitor.toolbar.filter.xhr</button><button id="requests-menu-filter-fonts-button" class="menu-filter-button" data-key="fonts">netmonitor.toolbar.filter.fonts</button><button id="requests-menu-filter-images-button" class="menu-filter-button" data-key="images">netmonitor.toolbar.filter.images</button><button id="requests-menu-filter-media-button" class="menu-filter-button" data-key="media">netmonitor.toolbar.filter.media</button><button id="requests-menu-filter-flash-button" class="menu-filter-button" data-key="flash">netmonitor.toolbar.filter.flash</button><button id="requests-menu-filter-ws-button" class="menu-filter-button" data-key="ws">netmonitor.toolbar.filter.ws</button><button id="requests-menu-filter-other-button" class="menu-filter-button" data-key="other">netmonitor.toolbar.filter.other</button>`; // eslint-disable-line max-len

Tests in this style are difficult to read and maintain. Instead of comparing two big blobs of HTML markup, the tests should test specific properties, for example:
- is the count of the rendered buttons correct?
- do the elements have the right classes?
- is the l10n working?

Comment 9

[[e-mail]]

Comment on attachment 8805464 [details]
Bug 1309496 - Set up mocha testing framework for netmonitor,

https://reviewboard.mozilla.org/r/89204/#review88860

> Tests in this style are difficult to read and maintain. Instead of comparing two big blobs of HTML markup, the tests should test specific properties, for example:
> - is the count of the rendered buttons correct?
> - do the elements have the right classes?
> - is the l10n working?

Test cases will be changed since I just wrote a simple test to make sure the test framework works

Comment 10

[[e-mail]]

Comment on attachment 8805464 [details]
Bug 1309496 - Set up mocha testing framework for netmonitor,

https://reviewboard.mozilla.org/r/89204/#review88364

> I am seeing following error when running this on Win10:
> 
> 'NODE_PATH' is not recognized as an internal or external command
> 
> (also pwd is not available on win)
> 
> 
> Also, shoould 'react-dev' module be part of the dependency list? (it's used by the require-helper.js module)

>  'NODE_PATH' is not recognized as an internal or external command

The current settings from webconsole does not compatible with windows. I'd use cross-env to set NODE_PATH instead

Comment 11

[[e-mail]]

Comment on attachment 8805464 [details]
Bug 1309496 - Set up mocha testing framework for netmonitor,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89204/diff/3-4/

Comment 12

[[e-mail]]

Comment on attachment 8805464 [details]
Bug 1309496 - Set up mocha testing framework for netmonitor,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89204/diff/4-5/

Comment 13

[[e-mail]]

I fixed cross platform test issues and rewrite the test cases. The l10n fixture will just return the string.

Comment 14

[[e-mail]]

Comment on attachment 8805464 [details]
Bug 1309496 - Set up mocha testing framework for netmonitor,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89204/diff/5-6/

Comment 15

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Nice!

33 passing (152ms)

(Win10)

Honza

Comment 16

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8805464 [details]
Bug 1309496 - Set up mocha testing framework for netmonitor,

https://reviewboard.mozilla.org/r/89204/#review89578

> Tests in this style are difficult to read and maintain. Instead of comparing two big blobs of HTML markup,
> the tests should test specific properties, for example:
> - is the count of the rendered buttons correct?
> - do the elements have the right classes?
> - is the l10n working?

I agree with Jarda here, but it's ok for me if this is done in a follow up.


Also, when I do 'npm ls' react-dev module is marked as 'extraneous'.
Is that expected?

Thanks!
Honza

::: devtools/client/netmonitor/test/fixtures/L10n.js:1
(Diff revision 6)
> +/* Any copyright is dedicated to the Public Domain.

Do not capitalize first letter of the file name.
(the name should be l10n.js)

::: devtools/client/netmonitor/test/fixtures/LocalizationHelper.js:1
(Diff revision 6)
> +/* Any copyright is dedicated to the Public Domain.

Name of the file should be localization-helper.js

Comment 17

[Lin Clark [:linclark]]

The test framework used is mocha. Enzyme is one of the packages used in the mocha tests. Just editing the title to reflect.

Comment 18

[Lin Clark [:linclark]]

Comment on attachment 8805464 [details]
Bug 1309496 - Set up mocha testing framework for netmonitor,

https://reviewboard.mozilla.org/r/89204/#review89632

I didn't give it a thorough review since I'm not familiar with the netmonitor code, but in general the npm/mocha stuff looks good to me.

Comment 19

[[e-mail]]

Comment on attachment 8805464 [details]
Bug 1309496 - Set up mocha testing framework for netmonitor,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89204/diff/6-7/

Comment 20

[[e-mail]]

Comment on attachment 8805464 [details]
Bug 1309496 - Set up mocha testing framework for netmonitor,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89204/diff/7-8/

Comment 21

[[e-mail]]

Issue addressed and reflect API change introduced from bug 1309192

Comment 22

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8805464 [details]
Bug 1309496 - Set up mocha testing framework for netmonitor,

https://reviewboard.mozilla.org/r/89204/#review90306

Looks good. I'm sure we'll make many improvements to this test infrastructure as we write more tests, but this path is certainly good enough for landing.

Comment 23

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8805464 [details]
Bug 1309496 - Set up mocha testing framework for netmonitor,

https://reviewboard.mozilla.org/r/89204/#review90310

Thanks for working on this!
Honza

::: devtools/client/netmonitor/test/fixtures/l10n.js:6
(Diff revision 8)
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// @TODO Load the actual strings from netmonitor.properties instead.

This would deserve a follow up so, it isn't forgotten.

Comment 24

[[e-mail]]

create followup bug 1317561 to Load the actual strings from netmonitor.properties

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/104403755426
Set up mocha testing framework for netmonitor,r=Honza,jsnajdr,linclark

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/104403755426