Closed Bug 1309496 Opened 3 years ago Closed 3 years ago

Set up mocha testing framework for netmonitor

Categories

(DevTools :: Netmonitor, defect, P1)

defect

Tracking

(firefox53 fixed)

RESOLVED FIXED
Firefox 53
Iteration:
53.1 - Nov 28
Tracking Status
firefox53 --- fixed

People

(Reporter: rickychien, Assigned: gasolin)

References

(Blocks 3 open bugs)

Details

(Whiteboard: [netmonitor])

Attachments

(1 file)

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.
Whiteboard: [netmonitor]
Blocks: 1309497
Flags: qe-verify?
Flags: qe-verify? → qe-verify-
Priority: -- → P2
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.
Attachment #8805464 - Flags: feedback?(rchien)
Attachment #8805464 - Flags: feedback?(odvarko)
Attachment #8805464 - Flags: feedback?(jsnajdr)
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
Attachment #8805464 - Flags: feedback?(odvarko)
Attachment #8805464 - Flags: feedback?(jsnajdr)
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/
Attachment #8805464 - Flags: feedback?(rchien)
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Iteration: --- → 52.3 - Nov 7
Priority: P2 → P1
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 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 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 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
I fixed cross platform test issues and rewrite the test cases. The l10n fixture will just return the string.
Blocks: 1314528
Nice!

33 passing (152ms)

(Win10)

Honza
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
Attachment #8805464 - Flags: review?(odvarko)
The test framework used is mocha. Enzyme is one of the packages used in the mocha tests. Just editing the title to reflect.
Summary: Set up enzyme testing framework for netmonitor → Set up mocha testing framework for netmonitor
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.
Attachment #8805464 - Flags: review?(lclark) → review+
Issue addressed and reflect API change introduced from bug 1309192
Depends on: 1309192
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.
Attachment #8805464 - Flags: review?(jsnajdr) → review+
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.
Attachment #8805464 - Flags: review?(odvarko) → review+
Iteration: 52.3 - Nov 14 → 53.1 - Nov 28
Blocks: 1317561
create followup bug 1317561 to Load the actual strings from netmonitor.properties
Keywords: checkin-needed
Blocks: 1316484
Pushed by jsnajdr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/104403755426
Set up mocha testing framework for netmonitor,r=Honza,jsnajdr,linclark
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/104403755426
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.