Nicolas Chevobbe [:nchevobbe] Assignee
	Description • 7 years ago

This is due to babelrc configuration being picked up in tests when it shouldn't

	Alexandre Poirot [:ochameau]
	Comment 3 • 7 years ago
mozreview-review

Comment on attachment 8951209 [details]
Bug 1438476 - Fix mocha tests broken due to React 16 update; .

https://reviewboard.mozilla.org/r/220470/#review226402

	Alexandre Poirot [:ochameau]
	Comment 4 • 7 years ago
mozreview-review

Comment on attachment 8951209 [details]
Bug 1438476 - Fix mocha tests broken due to React 16 update; .

https://reviewboard.mozilla.org/r/220470/#review226404

	Alexandre Poirot [:ochameau]
	Comment 5 • 7 years ago
mozreview-review

Comment on attachment 8951208 [details]
Bug 1438476 - Fix launchpad configuration for console and netmonitor; .

https://reviewboard.mozilla.org/r/220468/#review226406

I'm not sure I'm qualified enough in babel to understand this change.
I'm still convinced there is something wrong here.
Can't we have a babelrc file in devtools/client/webconsole/new-console-output/test/ that would overload the one from webconsole folder? Also could you explain why babelrc file from webconsole is involved here? I don't see why it is involved when processing mocha-test-setup.js module?

Otherwise, this patch addresses:
  ReferenceError: Unknown plugin "transform-flow-strip-types" specified in "/mnt/desktop/gecko/devtools/client/webconsole/.babelrc"
But there is still:
  ReferenceError: Unknown plugin "transform-react-jsx" specified in "/mnt/desktop/gecko/devtools/client/netmonitor/.babelrc"

Are you sure you cleaned your would m-c tree??
The following command will help you ensure you removed all your node_modules:
  $ find devtools/ -name node_modules

If I apply similar trick to netmonitor's babelrc,
I either get failure because spread operator isn't supported by node, or, get this error if I keep it in babelrc:
  ReferenceError: Unknown plugin "transform-object-rest-spread" specified
Whereas we do have this plugin registered in test/package.json?

So, how does spread works for the console, is it solely thanks to this in babelrc:
  "./new-console-output/test/node_modules/babel-plugin-transform-object-rest-spread"
?
This all looks very brittle to me. Shouldn't we now consider the test folder as a completely independant package and prevent depending things from webconsole to depend on test package?

	Julian Descottes [:jdescottes]
	Comment 12 • 7 years ago
mozreview-review

Comment on attachment 8951208 [details]
Bug 1438476 - Fix launchpad configuration for console and netmonitor; .

https://reviewboard.mozilla.org/r/220468/#review226490

Very high level questions for now, I feel like some comments are required here.

::: devtools/client/netmonitor/.babelrc:18
(Diff revision 4)
> +  "plugins": [
> +    "transform-object-rest-spread"
> +  ]

Is this still needed? Or maybe it should be under the "test" env to make it clear that when running mocha tests we only use that.

It would be nice to explain why different sets of plugins are needed for the different environments. I'm guessing it's mainly to simplify the dependency management on CI?

::: devtools/client/webconsole/new-console-output/.babelrc:1
(Diff revision 4)
>  {

I tried reading the conversation on slack, but I don't understand why we wouldn't have a single babelrc file in webconsole/new-console-output, with three environments defined: test, development and production. Why do we need separate files?

Can we add a comment making the purpose of the file clear?

::: devtools/client/webconsole/package.json:8
(Diff revision 4)
> -    "start": "cross-env NODE_ENV=production node bin/dev-server",
> -    "dev": "node bin/dev-server"
> +    "start": "cross-env NODE_PATH=./node_modules:../../.. NODE_ENV=production node bin/dev-server",
> +    "dev": " cross-env NODE_PATH=./node_modules:../../.. NODE_ENV=development node bin/dev-server"

Would be nice to have consistent ordering with devtools/client/webconsole/new-console-output/test/package.json :

NODE_ENV to be moved before NODE_PATH ?

The NODE_PATH value seems worth a comment too?

	Nicolas Chevobbe [:nchevobbe] Assignee
	Comment 13 • 7 years ago
mozreview-review-reply

Comment on attachment 8951208 [details]
Bug 1438476 - Fix launchpad configuration for console and netmonitor; .

https://reviewboard.mozilla.org/r/220468/#review226490

Thanks for asking those questions Julian. Hope the answers make sense

> Is this still needed? Or maybe it should be under the "test" env to make it clear that when running mocha tests we only use that.
> 
> It would be nice to explain why different sets of plugins are needed for the different environments. I'm guessing it's mainly to simplify the dependency management on CI?

yes, it is needed for test environment, and yes it would maybe be better to put it in a test env object, or at least add comments

> I tried reading the conversation on slack, but I don't understand why we wouldn't have a single babelrc file in webconsole/new-console-output, with three environments defined: test, development and production. Why do we need separate files?
> 
> Can we add a comment making the purpose of the file clear?

we still need to have a file so node_modules/ files are transpiled (which we need for the launchpad, since it uses jsx and flow)

```
|- netmonitor
|  |- node_modules
|  |- src
|  |- .babelrc (1)
|  
|- webconsole
|  |- node_modules
|  |- new-console-output
|  |- .babelrc (2)
```

We need multiple babelrc because they are picked up relatively to the file being transpiled. So here, when running tests, file from new-console-output will be transpiled according to `.babelrc (2)`, and files from netmonitor, which are required in the console, will be transpiled according to  `.babelrc (1)`.

The thing that we could do, I think is have only one babelrc that would contain the actual config, and another one which "extends" it (See in https://babeljs.io/docs/usage/api/#options). I don't know if we should have a dedicated shared babelrc, i.e. outside of webconsole/netmonitor, or if one of the babelrc from webconsole/netmonitor could play this role.

> Would be nice to have consistent ordering with devtools/client/webconsole/new-console-output/test/package.json :
> 
> NODE_ENV to be moved before NODE_PATH ?
> 
> The NODE_PATH value seems worth a comment too?

Right

	Julian Descottes [:jdescottes]
	Comment 14 • 7 years ago
mozreview-review-reply

Comment on attachment 8951208 [details]
Bug 1438476 - Fix launchpad configuration for console and netmonitor; .

https://reviewboard.mozilla.org/r/220468/#review226490

> we still need to have a file so node_modules/ files are transpiled (which we need for the launchpad, since it uses jsx and flow)
> 
> ```
> |- netmonitor
> |  |- node_modules
> |  |- src
> |  |- .babelrc (1)
> |  
> |- webconsole
> |  |- node_modules
> |  |- new-console-output
> |  |- .babelrc (2)
> ```
> 
> We need multiple babelrc because they are picked up relatively to the file being transpiled. So here, when running tests, file from new-console-output will be transpiled according to `.babelrc (2)`, and files from netmonitor, which are required in the console, will be transpiled according to  `.babelrc (1)`.
> 
> The thing that we could do, I think is have only one babelrc that would contain the actual config, and another one which "extends" it (See in https://babeljs.io/docs/usage/api/#options). I don't know if we should have a dedicated shared babelrc, i.e. outside of webconsole/netmonitor, or if one of the babelrc from webconsole/netmonitor could play this role.

My question was about having one babelrc in /webconsole and one babelrc in /webconsole/new-webconsole-output. From your answer I understand that we need different files in netmonitor and webconsole, but I'm still not sure about webconsole and webconsole/new-webconsole-output.

	Nicolas Chevobbe [:nchevobbe] Assignee
	Comment 15 • 7 years ago

(In reply to Julian Descottes [:jdescottes][:julian] from comment #14)
> My question was about having one babelrc in /webconsole and one babelrc in
> /webconsole/new-webconsole-output. From your answer I understand that we
> need different files in netmonitor and webconsole, but I'm still not sure
> about webconsole and webconsole/new-webconsole-output.

Right, let's only have one

	Nicolas Chevobbe [:nchevobbe] Assignee
	Comment 16 • 7 years ago

For the record, with Nick's patch, and some tweaks (node 8, updated module location), mocha tests are run in CI https://treeherder.mozilla.org/logviewer.html#?job_id=162455070&repo=try&lineNumber=384-423

	Nick Alexander :nalexander [he/him] (PTO until Montreallhands August 21, 2023)
	Comment 17 • 7 years ago

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #16)
> For the record, with Nick's patch, and some tweaks (node 8, updated module
> location), mocha tests are run in CI
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=162455070&repo=try&lineNumber=384-423

That's great!  I'm not quite sure what the future should look like here, 'cuz gps and I are pushing towards requiring node in the build system, which might change the approach we end up landing for this type of thing.  But it would not be too hard to try to push this across the line, even as a temporary measure.  NI for me to circle back to this in the next few days and see if I can get the build/TC bits ready to land.

Regardless, I think organizing package.json files to stand alone, not muck with paths, etc, is a good step forward, and I'm pleased to see it happen.

	Alexandre Poirot [:ochameau]
	Comment 21 • 7 years ago

Thanks Nicolas for reviewing everything in detail, it looks like we have a much better understanding of it now.
The inlined comments will help!

I confirm that it still works fine here, with node 7.10 npm 4.2.

About requiring node 8, debugger.html requires 7, could we align to that?
https://github.com/devtools-html/debugger.html/blob/master/docs/getting-setup.md
(It is weird as their package.json refer to 6.9, I reported an issue https://github.com/devtools-html/debugger.html/issues/5445)

Let's wait for Nick to see what will happen on Taskcluster side,
but it may not be that easy to bump the node version used in m-c.
(The node version has to be aligned with the one ./mach eslint uses...)

Otherwise, I don't understand why babel is needed.
Spread operator seems to be available from node 5.12:
  http://node.green/#ES2015-syntax-spread-------operator
But may be I'm confused, may be node.green doesn't list this very particular spread usage
as it seems to be only stage 3:
  https://stackoverflow.com/questions/42735450/does-node-js-6-3-1-not-supports-object-rest-spread-properties
This page reports node 8.3 supports this natively, could you verify?
  This has been added to v8 in v6.0.75+:
    https://developers.google.com/web/updates/2017/06/object-rest-spread
  But node 7.10 is on 5.5:
    $ node -p process.versions.v8
    5.5.372.43

If we could drop babel, I have a way to stop using NODE_PATH via require-hacker.

	Nicolas Chevobbe [:nchevobbe] Assignee
	Comment 22 • 7 years ago

http://node.green/#ES2015-syntax-spread-------operator is about the array spread operator, not the object one.
But in http://node.green/#ES2018-features-object-rest-spread-properties , it says that the spread operator is supported since 8.6. And indeed, I can remove babel from the package.json when using node 8.9 and tests run just fine.


With this in mind, and your work on pruning NODE_PATH if we don't have to care about babel, I'd suggest that we stick with this node version for now, and then try to fix it if it is an issue for Taskcluster. What do you think ?

	Code Review Bot [:reviewbot]
	Comment 26 • 7 years ago
mozreview-review

Comment on attachment 8951519 [details]
Bug 1438476 - Fix and document webconsole test's package.json; .

https://reviewboard.mozilla.org/r/220818/#review226836


Code analysis found 3 defects in this patch:
 - 3 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/webconsole/new-console-output/test/mocha-test-setup.js:31
(Diff revision 2)
>  
>    // Some modules depend on Chrome APIs which don't work in mocha. When such a module
>    // is required, replace it with a mock version.
>    switch (path) {
>      case "devtools/shared/l10n":
> -      return `module.exports = require("devtools/client/webconsole/new-console-output/test/fixtures/LocalizationHelper")`;
> +      return getModule("devtools/client/webconsole/new-console-output/test/fixtures/LocalizationHelper");

Error: Line 31 exceeds the maximum line length of 90. [eslint: max-len]

::: devtools/client/webconsole/new-console-output/test/mocha-test-setup.js:33
(Diff revision 2)
>    // is required, replace it with a mock version.
>    switch (path) {
>      case "devtools/shared/l10n":
> -      return `module.exports = require("devtools/client/webconsole/new-console-output/test/fixtures/LocalizationHelper")`;
> +      return getModule("devtools/client/webconsole/new-console-output/test/fixtures/LocalizationHelper");
>      case "devtools/shared/plural-form":
> -      return `module.exports = require("devtools/client/webconsole/new-console-output/test/fixtures/PluralForm")`;
> +      return getModule("devtools/client/webconsole/new-console-output/test/fixtures/PluralForm");

Error: Line 33 exceeds the maximum line length of 90. [eslint: max-len]

::: devtools/client/webconsole/new-console-output/test/mocha-test-setup.js:36
(Diff revision 2)
> -      return `module.exports = require("devtools/client/webconsole/new-console-output/test/fixtures/LocalizationHelper")`;
> +      return getModule("devtools/client/webconsole/new-console-output/test/fixtures/LocalizationHelper");
>      case "devtools/shared/plural-form":
> -      return `module.exports = require("devtools/client/webconsole/new-console-output/test/fixtures/PluralForm")`;
> +      return getModule("devtools/client/webconsole/new-console-output/test/fixtures/PluralForm");
>      case "Services":
>      case "Services.default":
> -      return `module.exports = require("devtools/client/webconsole/new-console-output/test/fixtures/Services")`;
> +      return getModule("devtools/client/webconsole/new-console-output/test/fixtures/Services");

Error: Line 36 exceeds the maximum line length of 90. [eslint: max-len]

	Julian Descottes [:jdescottes]
	Comment 30 • 7 years ago
mozreview-review

Comment on attachment 8951519 [details]
Bug 1438476 - Fix and document webconsole test's package.json; .

https://reviewboard.mozilla.org/r/220818/#review226868

Thanks for the additional comments, that is much easier to understand now!

::: devtools/client/webconsole/new-console-output/test/package.json:14
(Diff revision 3)
> +      "Here's the script to run tests with `npm test`. Here's what it does: ",
> +      " * Run mocha on components, middleware, store and utils folders, on .test.js files.",
> +      "   We need to specify them so we don't run unwanted tests (e.g. in node_modules).",
> +      " * We require jsdom-global to inject `document` and `window` objects which are ",
> +      "   not in nodejs natively.",
> +      " * Finally we require mocha-test-setup where we configure Enzyme and to",

nit: "and to" -> "to" ? (if I understand correctly)

::: devtools/client/webconsole/new-console-output/test/package.json:15
(Diff revision 3)
> +      " * Run mocha on components, middleware, store and utils folders, on .test.js files.",
> +      "   We need to specify them so we don't run unwanted tests (e.g. in node_modules).",
> +      " * We require jsdom-global to inject `document` and `window` objects which are ",
> +      "   not in nodejs natively.",
> +      " * Finally we require mocha-test-setup where we configure Enzyme and to",
> +      "   intercept require() calls  with require-hacker and modify them if needed."

nit: two spaces in "calls  with" :P

	Nicolas Chevobbe [:nchevobbe] Assignee
	Comment 31 • 7 years ago

Last set of patches removes the babel dependency which makes things a lot simpler !
Thanks Alex for the nudge on this :)

	Julian Descottes [:jdescottes]
	Comment 35 • 7 years ago
mozreview-review

Comment on attachment 8951208 [details]
Bug 1438476 - Fix launchpad configuration for console and netmonitor; .

https://reviewboard.mozilla.org/r/220468/#review226872

Launchpad breaks for me both in the webconsole or in the netmonitor as it fails to handle object spread expressions.
Webpack compilation fails with errors such as:

  ERROR in ../netmonitor/src/utils/headers-provider.js
  Module build failed: SyntaxError: Unexpected token (19:2)

    17 |  */
    18 | var HeadersProvider = {
  > 19 |   ...ObjectProvider,
       |   ^
    20 | 
    21 |   getChildren(object) {
    22 |     if (object.value instanceof HeaderList) {

   @ ../netmonitor/src/components/HeadersPanel.js 26:4-40
   @ ../netmonitor/src/components/TabboxPanel.js
   @ ./new-console-output/components/message-types/NetworkEventMessage.js
   @ ./new-console-output/components/MessageContainer.js
   @ ./new-console-output/components/ConsoleOutput.js
   @ ./new-console-output/new-console-output-wrapper.js
   @ ./local-dev/index.js
   @ multi ./local-dev/index.js


I tried with the previous changeset and launchpad builds successfully. I have Node v8.5.0

	Julian Descottes [:jdescottes]
	Comment 36 • 7 years ago
mozreview-review

Comment on attachment 8951208 [details]
Bug 1438476 - Fix launchpad configuration for console and netmonitor; .

https://reviewboard.mozilla.org/r/220468/#review226880

Works well with the last patch!

	Nick Alexander :nalexander [he/him] (PTO until Montreallhands August 21, 2023)
	Comment 37 • 7 years ago

> Let's wait for Nick to see what will happen on Taskcluster side,
> but it may not be that easy to bump the node version used in m-c.
> (The node version has to be aligned with the one ./mach eslint uses...)

Replying only to this point: we can easily run in a different Docker image with a different Node version.  We could allow to specify the Node version in the Task Cluster yaml without too much difficulty, in fact.

	Pulsebot
	Comment 38 • 7 years ago

Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9610e7e13865
Fix and document webconsole test's package.json; r=jdescottes.
https://hg.mozilla.org/integration/autoland/rev/01e06518eb95
Fix mocha tests broken due to React 16 update; r=ochameau.
https://hg.mozilla.org/integration/autoland/rev/7b08a8b076fd
Fix launchpad configuration for console and netmonitor; r=jdescottes.

	Alexandre Poirot [:ochameau]
	Comment 39 • 7 years ago

(In reply to Nick Alexander :nalexander from comment #37)
> > Let's wait for Nick to see what will happen on Taskcluster side,
> > but it may not be that easy to bump the node version used in m-c.
> > (The node version has to be aligned with the one ./mach eslint uses...)
> 
> Replying only to this point: we can easily run in a different Docker image
> with a different Node version.  We could allow to specify the Node version
> in the Task Cluster yaml without too much difficulty, in fact.

I know, but I tried and it was full of unexpected complexity.
Having a custom docker image *and* still have m-c being automatically cloned in this docker image doesn't work as easily as you would think :/ It isn't an issue with Taskcluster really, but more with all the configuration files specific to m-c.

See this try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdfe44c8c065ffb2c894e54cc84c1f499ba0cfbd&selectedJob=159864953
I tried working around that error with jlorenzo in many different ways without being able to get it to work.

	Raul Gurzau (:RaulG)
	Comment 40 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/9610e7e13865
https://hg.mozilla.org/mozilla-central/rev/01e06518eb95
https://hg.mozilla.org/mozilla-central/rev/7b08a8b076fd