1401189 - Console tests should be runnable via mach commands

Status: NEW

(DevTools :: Console, task, P2)

Description

[Alexandre Poirot [:ochameau]]

This is a serious blocker to console contribution for mozilla employees.
(i.e. not just devtools folks)

We should be able to run any devtools tests via mach.
And also run them on CI (bug 1312823)

I imagine being able to run the test locally is a first step toward running them on mozilla CI.

Comment 1

[Alexandre Poirot [:ochameau]]

I don't get how we got such far with this project without having tests being run on mozilla CI.

We can't have them running on CI if that's not runnable via mach.
Tests that run on CI should be easy to run in case you break one.

The current state of things make it:
* easy to break these uncovered test by any mozilla-central patch
* hard to contribute to them from mozilla-central

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Thanks for filing that Alex.
One of the main problem on running these tests on CI was that it requires doing `npm install`, and I think it can be problematic to reach the network in tests ?

Comment 3

[Alexandre Poirot [:ochameau]]

You should look into mozilla-central eslint integration, which had to cover this.

It uses this python module:
http://searchfox.org/mozilla-central/source/tools/lint/eslint/setup_helper.py

A mach command is as simple as adding such module:
  http://searchfox.org/mozilla-central/source/tools/lint/mach_commands.py#46-63
and registering it here:
  http://searchfox.org/mozilla-central/source/build/mach_bootstrap.py#36
Then it is mostly about writing python...

I think it should be reasonably easy by reusing setup_helper.py.

Before thinking about CI, you should get something to run the tests locally.
It isn't reasonable to have tests on CI that aren't trivial to run locally.

Otherwise, eslint on taskcluster seems to depend on the mach lint command...
http://searchfox.org/mozilla-central/source/taskcluster/ci/source-test/mozlint.yml#1-37
But yes, it looks like node_modules are already on the docker image.
Their node_modules are most likely more conservative than what we need to run console tests.

Comment 4

[Brian Grinstead [:bgrins]]

Nick, we briefly talked about this last week - do you have any ideas about how we could accomplish this?

I'm not sure about what CLI we should shoot for - perhaps a general purpose `./mach npm-test` command would be useful for other teams that have been writing similar tests as well (although I'm not aware of any of those inside m-c at the moment). The reason I mention `mach npm-test` instead of `mach mocha` or similar is that we have to do some setup before calling into mocha, so tests are run with `npm test` instead of mocha directly: https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/package.json#10.

Comment 5

[Nick Alexander :nalexander [he/him]]

(In reply to Brian Grinstead [:bgrins] from comment #4)
> Nick, we briefly talked about this last week - do you have any ideas about
> how we could accomplish this?
> 
> I'm not sure about what CLI we should shoot for - perhaps a general purpose
> `./mach npm-test` command would be useful for other teams that have been
> writing similar tests as well (although I'm not aware of any of those inside
> m-c at the moment). The reason I mention `mach npm-test` instead of `mach
> mocha` or similar is that we have to do some setup before calling into
> mocha, so tests are run with `npm test` instead of mocha directly:
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/
> package.json#10.

I have a few thoughts:

1) Alexandre did a great job outlining the Python mach command process in #c3.  I skimmed the code and the approach I would start with is to split tools/lint/eslint/setup_helper.py into tools/node_setup_helper.py (tool agnostic) and the existing tools/lint/eslint/setup_helper.py (eslint specific).  The eslint Python mach command is super special, 'cuz it uses ahal's fancy lint roller extensibility, which isn't all that relevant for invoking node.  I'd even suggest supporting `mach npm /path/to/package.json VERB` rather than only `npm-test`, since there are lots of interesting verbs and command line arguments to pass to `npm` too.  I had success with this approach for Android: we have `mach gradle ...` rather than `mach gradle-...`.

This is largely stylistic: do we see `npm-test` as its own flavour of test (like xpcshell or mochi), or do we see `npm` as an independent command?  I think I see it as an independent command.

2) In automation, the existing approach (used by eslint) is to bake dependencies into the build image.  That's not wrong or anything, but I've come to prefer toolchain jobs.  That is, a toolchain job collects dependencies -- in this case, packages up node_modules (I sure hope node_modules is path independent!) -- and then produces a toolchain archive of it that the actual test jobs consume.  I think this will lead to more reproducible builds, since the inputs to the toolchain job are hashed and the outputs correspond.  (Of course, the fetched dependencies can drift if they don't transitively pin their dependencies, but c'est la vie.)  This is how I'm handling/going to handle the identical process for Android Gradle builds, and it's conceptually appealing.

This isolates the network access into the toolchain job away from the test jobs.

Comment 6

[Nick Alexander :nalexander [he/him]]

gps, dustin: can you comment on 2) -- do y'all prefer toolchain archives of this sort or baking dependencies into Docker images?

Comment 7

[Alexandre Poirot [:ochameau]]

(In reply to Nick Alexander :nalexander from comment #5)
> This is largely stylistic: do we see `npm-test` as its own flavour of test
> (like xpcshell or mochi), or do we see `npm` as an independent command?  I
> think I see it as an independent command.

For the sole purpose of this bug, this should be another test flavour.
At the end of this, ./mach test would also work for console npm tests.
We would have npm.ini (like mochitest.ini/browser.ini/...) to help running/sherifing the tests on CI.

I imagine, behind the scene we will end up with a generic "npm" abstraction in python, like setup_helper.py.
Then, nothing prevent exposing a generic npm command, but its usage/purpose is unclear to me.

Comment 8

[Dustin J. Mitchell [:dustin] (he/him)]

Advantages to a toolchain:
 - easy for devs to pull locally
 - can have multiple toolchains in a single job (whereas you can only have one docker image)
 - easy to rebuild, and to have several different versions on different branches / trains

Advantages to a docker image:
 - easier to define than a toolchain
 - no worries about location independence, fiddly install processes, etc.

I tend to worry about some other issues, too:
 - Is this "a toolchain" or does the toolchain mechanics just look useful?  (similar to your "largely stylistic")
 - How big is this?  If the tarball rivals the size of a docker image, maybe a docker image is better
 - Is this reproducible?  I'd rather see yarn + yarn.lock than npm.

Comment 9

[Nick Alexander :nalexander [he/him]]

(In reply to Alexandre Poirot [:ochameau] from comment #7)
> (In reply to Nick Alexander :nalexander from comment #5)
> > This is largely stylistic: do we see `npm-test` as its own flavour of test
> > (like xpcshell or mochi), or do we see `npm` as an independent command?  I
> > think I see it as an independent command.
> 
> For the sole purpose of this bug, this should be another test flavour.
> At the end of this, ./mach test would also work for console npm tests.
> We would have npm.ini (like mochitest.ini/browser.ini/...) to help
> running/sherifing the tests on CI.

I understand why we have .ini files, but this approach makes it very difficult to avoid writing one's own test harness.  Honestly, it's not worth the effort until you really need to solve the sheriff problem at scale.  We should just use Mocha/Jasmine/Jest/anything except our own special snowflake until we really, really need to handle INI files, SUPPORT-FILES, etc.

> I imagine, behind the scene we will end up with a generic "npm" abstraction
> in python, like setup_helper.py.
> Then, nothing prevent exposing a generic npm command, but its usage/purpose
> is unclear to me.

Really?  Doesn't some devtools flow already expose an `npm serve` to run a development server?

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Nick Alexander :nalexander from comment #9)
> (In reply to Alexandre Poirot [:ochameau] from comment #7)
> > I imagine, behind the scene we will end up with a generic "npm" abstraction
> > in python, like setup_helper.py.
> > Then, nothing prevent exposing a generic npm command, but its usage/purpose
> > is unclear to me.
> 
> Really?  Doesn't some devtools flow already expose an `npm serve` to run a
> development server?

Yes, from the webconsole (or netmonitor) directory, `npm start` and `npm run dev` [0] start up the "launchpad" [1] connection that allows for local development in a tab.

One CLI decision we'd need to make if we went with a general purpose npm command is how to handle multiple package.json files in the tree. I suppose you'd pass the path to the folder with the package. `./mach npm [start|install|test] devtools/client/webconsole`, as opposed to what we do now: `cd devtools/client/webconsole && npm [start|install|test]`.

[0]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/package.json#9
[1]: https://github.com/devtools-html/devtools-core/tree/master/packages/devtools-launchpad

Comment 11

[Alexandre Poirot [:ochameau]]

(In reply to Nick Alexander :nalexander from comment #9)
> I understand why we have .ini files, but this approach makes it very
> difficult to avoid writing one's own test harness.  Honestly, it's not worth
> the effort until you really need to solve the sheriff problem at scale.  We
> should just use Mocha/Jasmine/Jest/anything except our own special snowflake
> until we really, really need to handle INI files, SUPPORT-FILES, etc.

The main goal is to have the test run on mozilla CI *and* make it easy for mozilla-central contributors to run and fix them.
These two sub goals are tightly related. You can't make them Tier 1/2 if that not easy to run locally.
Also you need sherifing and our ini files are very important for them.
The supports-files is specific to the test harness, you don't have to support that.
But the [skip-if] and all the flagging attributes are a key feature to have a decently sheriffed CI!
I already introduced a brand new test suite back in time (luciddream) and supporting test manifest (.ini files) isn't such a bid deal...

Comment 12

[Alexandre Poirot [:ochameau]]

(In reply to Brian Grinstead [:bgrins] from comment #10)
> (In reply to Nick Alexander :nalexander from comment #9)
> > (In reply to Alexandre Poirot [:ochameau] from comment #7)
> > > I imagine, behind the scene we will end up with a generic "npm" abstraction
> > > in python, like setup_helper.py.
> > > Then, nothing prevent exposing a generic npm command, but its usage/purpose
> > > is unclear to me.
> > 
> > Really?  Doesn't some devtools flow already expose an `npm serve` to run a
> > development server?
> 
> Yes, from the webconsole (or netmonitor) directory, `npm start` and `npm run
> dev` [0] start up the "launchpad" [1] connection that allows for local
> development in a tab.

This is really different from tests. I would suggest opening a specific issue for these kind of npm commands.
And TBH, I'm not sure that's a good setup to introduce.
If some folder need some npm magic, it should be streamlined into firefox build process.
Within ./mach build faster, ./mach watch or a more generic command where you would not need to know per-project/folder setup.
You shouldn't have to learn to go execute some special command to develop against a given folder.
Like, if you modify reps, remember to correctly run ./mach npm devtools/.../reps start,
but, if you also modify the console, please also do ./mach nom devtools/client/console start,
and so on. We already have 4 npm packages in devtools (debugger, inspector, console, netmonitor),
that can become a serious nightmare if you have to setup all these things in parallel.

See what I have in mind for console tests. ./mach test would work for console test using npm. You wouldn't have to know anything special. I hope you can do something similar with your serve/start/install ideas you have in mind.

Comment 13

[Gregory Szorc [:gps]]

CI and test automation should have the following qualities:

* Deterministic
* Reproducible
* Minimal, well-defined external dependencies
* Ideally fast and easy to use

We intentionally try to avoid having our CI hit third party services because every third party service represents a potential point of failure that can cause a tree closure as well as a potential injection point for security issues. In some cases, the load of Firefox's CI is enough to DDoS a third party service!

It is common for us to vendor dependencies in the repo so a) there is no run-time service we need to hit and therefore no potential for service outage b) changes are versioned in the repo and are therefore deterministic c) people can run things with a source checkout and no Internet connectivity.

That being said, these qualities are only really needed for tier 1 automation. There are several exceptions to them. If we're standing up new tier 2 or tier 3 CI, I think it is fine to bootstrap the CI hitting third party services. But we should ideally have a plan for eliminating the 3rd party service dependency.

As for toolchain archives vs Docker images, it is probably easier to start with Docker images. We can factor out components into toolchain archives later. The infrastructure for toolchain archives isn't very robust yet and I'm hesitant to scope bloat your work by suggesting that you use toolchain archives (even though toolchain archives are preferred).

Comment 14

[Mark Banner (:standard8)]

Some of the comments from bug 1379118 comment 3 might be useful here.

As a separate note, ESLint's npm is installed locally for developers (and we use npm-shrinkwrap.json as well), but on the builders, we have a tarball uploaded to releng's automation and a reference to that stored within m-c. The Docker images then get rebuilt with the tarball extracted in the right place, and hence it provides the deterministic qualities required, as well as not hitting the network.

There's a process in place for uploading new tarballs, which we generally do when we upgrade ESLint (https://searchfox.org/mozilla-central/source/tools/lint/eslint/update.sh).

Comment 15

[Mark Banner (:standard8)]

Also, fwiw, we were originally thinking about a `./mach mocha` hook up for ESLint, but decided it wasn't worth it at the time as it was a small bit of well contained code and it wasn't clear the future for elsewhere, we might reverse that now if this goes ahead.

My only concern about an `./mach npm` type command is that it could be that we end up with multiple test types underneath that, since "npm test" can test virtually anything (e.g. some tests could be mocha based, no UI runs, some could be selenium with the UI). I don't know how much that matters, and maybe it is too early to think about it.

The other thing to note that may be useful to you is that we have already have a reporter for mocha to format error messages in a way the various log parsers will understand: https://searchfox.org/mozilla-central/source/tools/lint/eslint/eslint-plugin-mozilla/reporters/mozilla-format.js

Comment 16

[Nick Alexander :nalexander [he/him]]

(In reply to Mark Banner (:standard8) from comment #14)
> Some of the comments from bug 1379118 comment 3 might be useful here.
> 
> As a separate note, ESLint's npm is installed locally for developers (and we
> use npm-shrinkwrap.json as well), but on the builders, we have a tarball
> uploaded to releng's automation and a reference to that stored within m-c.
> The Docker images then get rebuilt with the tarball extracted in the right
> place, and hence it provides the deterministic qualities required, as well
> as not hitting the network.
> 
> There's a process in place for uploading new tarballs, which we generally do
> when we upgrade ESLint
> (https://searchfox.org/mozilla-central/source/tools/lint/eslint/update.sh).

I got interested in this and started working on moving the NPM tarball process to a toolchain task.  My end goal is to auto-generate the |npm install| toolchain task, making it much more pleasant to say "this in-tree package.json, these npm commands".

Hacky WIP at https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e2dab9bf43825c5f2e3335736fff9d7c6745fa5 (ES lint is supposed to fail here, I wasn't certain it was actually running due to the --quiet flag).

standard8: can you explain why there's special and slightly different handling for eslint-plugin-mozilla and eslint-plugin-spidermonkey?

Comment 17

[Mark Banner (:standard8)]

(In reply to Nick Alexander :nalexander from comment #16)
> standard8: can you explain why there's special and slightly different
> handling for eslint-plugin-mozilla and eslint-plugin-spidermonkey?

eslint-plugin-spidermonkey doesn't get published and doesn't have tests - it is a preprocessor for the js/ files.

eslint-plugin-mozilla gets published to npm (hence a few more scripts), and also has tests that are running via mocha (the `mocha[tier2](epm)` on treeherder).

For the epm task, we're using the package.json from within the tools/lint/eslint/eslint-plugin-mozilla directory - it has a different dependency set from the main ESLint task (e.g. needs mocha), so we generate a node_modules package for it.

For the main ESLint task (ES), we're also using a bundled node_modules directory but a different one - it is generated from the top-level package.json. The bundled version does not have the eslint-plugin-{mozilla,spidermonkey} directories - they get symlinked in, so that we're always using the source tree version of them - there's not really much point in re-bundling every time they change (and at the moment, it would cause more work for developers).

Comment 18

[Nick Alexander :nalexander [he/him]]

> 2) In automation, the existing approach (used by eslint) is to bake
> dependencies into the build image.  That's not wrong or anything, but I've
> come to prefer toolchain jobs.  That is, a toolchain job collects
> dependencies -- in this case, packages up node_modules (I sure hope
> node_modules is path independent!) -- and then produces a toolchain archive
> of it that the actual test jobs consume.  I think this will lead to more
> reproducible builds, since the inputs to the toolchain job are hashed and
> the outputs correspond.  (Of course, the fetched dependencies can drift if
> they don't transitively pin their dependencies, but c'est la vie.)  This is
> how I'm handling/going to handle the identical process for Android Gradle
> builds, and it's conceptually appealing.

I started hacking on this, and while it required some tweaks to the toolchain kind in TC's taskgraph implementation, it basically works.  See the npm(epm) job in https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&selectedJob=155478792, which is running the (mocha) npm tests for eslint-plugin-mozilla.  There's also a trivial implementation of the equivalent for webconsole tests, namely the stanza at https://hg.mozilla.org/try/rev/1da56f63b6c6181582fbb1873dc33cd19738302f.

What that does is set up a node_modules job that:

* |cd devtools/client/webconsole && npm install|
* packages up the resulting node_modules

and an npm job that

* |cd devtools/client/webconsole|
* extracts the resulting node_modules;
* runs |npm run test|

And it almost works!  Except there's some kind of dependency between webconsole and netmonitor that's not captured in the package.json -- see the error.  Locally, if I run |cd devtools/client/netmonitor && yarn install| first, then the |npm run test| succeeds.

In my mind, webconsole (and netmonitor!) should have well-formed package.json files that allow those two things to live independently and each have a pair of tasks like I describe above.  Can that be achieved?

If not, can we find some way to declare a single package.json that handles both webconsole and netmonitor, so there's only one |npm install| and then multiple |npm run test... test...| invocations?  I'm not expert on how these bits in the tree get combined, so any commentary appreciated.

bgrinstead: NI to you to redirect to the correct people.

Comment 19

[Brian Grinstead [:bgrins]]

Nicolas or Honza: any ideas on how we can remove the interdependancy between the webconsole and netmonitor npm packages? I know this has been discussed but couldn't find a bug on file for it.

Comment 20

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

(In reply to Nick Alexander :nalexander from comment #18)
> And it almost works!  Except there's some kind of dependency between
> webconsole and netmonitor that's not captured in the package.json
> -- see the error.
Where can I see the error?

What I am seeing locally on my machine (when removing node_modules in netmonitor dir) is:

ReferenceError: Unknown plugin "transform-object-rest-spread" specified in "C:\\src\\mozilla.org\\mozilla-central\\devtools\\client\\netmonitor\\.babelrc" at 0, attempted to resolve relative to "C:\\src\\mozilla.org\\mozilla-central\\devtools\\client\\netmonitor"
    at C:\src\mozilla.org\mozilla-central\devtools\client\webconsole\node_modules\babel-core\lib\transformation\file\options\option-manager.js:180:17

It's related to: Bug 1420934 - Can't run console tests

---

I think that even if webconsole is having "babel-plugin-transform-object-rest-spread" in package.json - babel is attempting to resolve the module path relative to "netmonitor" directory. And so, at least this needs 'npm install' in netmonitor.

To explain the dependency. Console panel is (similarly to net panel) also displaying HTTP requests (if XHR or Requests filters are on). These HTTP requests (more precisely HTTP details, ie the body of expanded HTTP request) are rendered inside the Console panel using the same React components as the Net panel. So, we have just one set-of-components for rendering HTTP logs.

Obviously, when Console is running it needs these net modules and they should also be part of the result bundle if Console is running on top of the Launchpad.

In order to remove this dependency we could move all shared modules into 'shared' dir and deal with them just like we deal with other shared modules. But, I don't like this solution much since these modules are not generic modules that could be reused by other panels (like others in shared dir). They implement specific feature (displaying HTTP details), which is used in net and console panels and it's unlikely that other panels will ever use them.

We could rather investigate how to properly configure babel, so it looks for modules in the right place - in order to solve the babel error I am seeing.

For now, running `npm install` in netmonitor directory should solve the problem.

Also note that some tests are there to verify integration between these two panels. E.g. clicking on HTTP request in the Console and selecting "Open in Network panel" action. It navigates the user to the Network panel and selects the appropriate requests. This requires both panels to be available.

Honza

Comment 21

[Brian Grinstead [:bgrins]]

(In reply to Jan Honza Odvarko [:Honza] from comment #20)
> We could rather investigate how to properly configure babel, so it looks for
> modules in the right place - in order to solve the babel error I am seeing.

I looked into this some and couldn't find an easy way to provide a fallback require path for babel, but I did see that adding this to the "scripts" section of the webconsole package.json fixes the problem: `"preinstall": "cd ../netmonitor && npm install && cd ../webconsole"`. That's probably the easiest solution given that it's an actual dependency, and should ensure the two stay up to date in local dev as well.  I'll attach a patch to Bug 1420934 to do so.

Comment 22

[Brian Grinstead [:bgrins]]

Once Bug 1420934 hits m-c, `npm install && npm test` in the webconsole directory should work locally. To get it to work in automation I think it'll need some change on the initial packaging to also package up the netmonitor node_modules folder, and on the npm job to also extract that folder back into netmonitor/ when running in the test.

Thanks for looking into this Nick! Let me know if it's not going to work well with that approach. We can revisit looking into a way to make it work with just the webconsole directory. Another idea I had was to make netmonitor 'publish' the shared modules and then set it up as a dependency in the webconsole package.json.

Comment 23

[Nick Alexander :nalexander [he/him]]

(In reply to Jan Honza Odvarko [:Honza] from comment #20)
> (In reply to Nick Alexander :nalexander from comment #18)
> > And it almost works!  Except there's some kind of dependency between
> > webconsole and netmonitor that's not captured in the package.json
> > -- see the error.
> Where can I see the error?

It's at

https://public-artifacts.taskcluster.net/EJ4_APSkQOmgYEVZQ5lGYg/0/public/logs/live_backing.log

It's Babel-related, I think -- something about ** for Math.pow.

> What I am seeing locally on my machine (when removing node_modules in
> netmonitor dir) is:
> 
> ReferenceError: Unknown plugin "transform-object-rest-spread" specified in
> "C:\\src\\mozilla.org\\mozilla-central\\devtools\\client\\netmonitor\\.
> babelrc" at 0, attempted to resolve relative to
> "C:\\src\\mozilla.org\\mozilla-central\\devtools\\client\\netmonitor"
>     at
> C:\src\mozilla.org\mozilla-
> central\devtools\client\webconsole\node_modules\babel-
> core\lib\transformation\file\options\option-manager.js:180:17
> 
> It's related to: Bug 1420934 - Can't run console tests

I see this too, and I think it's equivalent.

<snip>

Thanks for the discussion.  If I understand correctly, webconsole truly depends on netmonitor, correct?

I see these file: links in https://searchfox.org/mozilla-central/source/package.json#9; could we use these to address the issue?

Right now, this pre-install stanza doesn't address the issue with my TC integration, since it only does |npm install| in one directory (devtools/client/webconsole) and packages up only the one node_modules directory.  I really don't think we should do multiple |npm install| invocations and package multiple node_modules -- any such action seems like an incorrectly packaged Node module.  We should make sure that |npm install| in the appropriate directory really does set up node_modules correctly, with no cross-tree linking required, etc.  If the way to achieve this is to lift the package.json up to devtools/client and capture all the subdependencies that way, I think that's fine.  (But that's up to your team to work through -- maybe that doesn't agree with the GH layout at all.)

I'd really like the in-tree package.json to agree with the GH package.json, too -- "landing" GH -> m-c should look like "cp -R gh/dir m-c/dir" as much as possible.  So that file:../netmonitor might not make sense in general.

Comment 24

[Brian Grinstead [:bgrins]]

(In reply to Nick Alexander :nalexander from comment #23)
> (In reply to Jan Honza Odvarko [:Honza] from comment #20)
> > (In reply to Nick Alexander :nalexander from comment #18)
> > > And it almost works!  Except there's some kind of dependency between
> > > webconsole and netmonitor that's not captured in the package.json
> > > -- see the error.
> > Where can I see the error?
> 
> It's at
> 
> https://public-artifacts.taskcluster.net/EJ4_APSkQOmgYEVZQ5lGYg/0/public/
> logs/live_backing.log
> 
> It's Babel-related, I think -- something about ** for Math.pow.

IIRC this one is related to the node version. Nicolas, is that right?

Comment 25

[Brian Grinstead [:bgrins]]

(In reply to Nick Alexander :nalexander from comment #23)
> Thanks for the discussion.  If I understand correctly, webconsole truly
> depends on netmonitor, correct?

Yes, that is correct. I may be missing other options here, so I'm cc'ing other folks who have been thinking about managing devtools dependencies, but I can think of representing this dependency either by:

1) Have the webconsole webpack configuration bundle the netmonitor modules at build time, and somehow ensure the netmonitor has been npm install'ed.  This is what we do now (with the small workflow enhancement landed in 1420934)
2) Set up netmonitor as a dependency in webconsole's package.json (either as a file path or a published npm module) and publish pre-bundled modules that the webconsole uses. I think this means the bundles need to be checked into tree and that at runtime we would not use the authored source from netmonitor but rather the bundled version. When we make changes to the netmonitor source the bundle should automatically be created so that the authored source never falls out of sync with what the webconsole uses, i.e. I think we'd need to integrate the bundling into `./mach build`.

> I see these file: links in
> https://searchfox.org/mozilla-central/source/package.json#9; could we use
> these to address the issue?

You mean from webconsole/package.json something like "file:../netmonitor"? I think that would be doable via something like (2). Or do you mean actually putting the reference in the root package.json in m-c as a way to make bundling for automation easier?

Comment 26

[Nick Alexander :nalexander [he/him]]

(In reply to Brian Grinstead [:bgrins] from comment #25)
> (In reply to Nick Alexander :nalexander from comment #23)
> > Thanks for the discussion.  If I understand correctly, webconsole truly
> > depends on netmonitor, correct?
> 
> Yes, that is correct. I may be missing other options here, so I'm cc'ing
> other folks who have been thinking about managing devtools dependencies, but
> I can think of representing this dependency either by:
> 
> 1) Have the webconsole webpack configuration bundle the netmonitor modules
> at build time, and somehow ensure the netmonitor has been npm install'ed. 
> This is what we do now (with the small workflow enhancement landed in
> 1420934)

I did not realize webpack was involved.  Oh dear.

> 2) Set up netmonitor as a dependency in webconsole's package.json (either as
> a file path or a published npm module) and publish pre-bundled modules that
> the webconsole uses. I think this means the bundles need to be checked into
> tree and that at runtime we would not use the authored source from
> netmonitor but rather the bundled version. When we make changes to the
> netmonitor source the bundle should automatically be created so that the
> authored source never falls out of sync with what the webconsole uses, i.e.
> I think we'd need to integrate the bundling into `./mach build`.

I very much do not want to depend on Node.js/npm/webpack at |mach build| time; that's a much larger and more contentious discussion.

However, I do think I need to understand what's happening right now:

1) if webconsole changes, how does the webpack/update-in-tree process happen?

2) if netmonitor changes, how does the webpack/update-in-tree process happen?

> > I see these file: links in
> > https://searchfox.org/mozilla-central/source/package.json#9; could we use
> > these to address the issue?
> 
> You mean from webconsole/package.json something like "file:../netmonitor"? I
> think that would be doable via something like (2).

I meant this -- if it works for GH/independent development.

> Or do you mean actually
> putting the reference in the root package.json in m-c as a way to make
> bundling for automation easier?

I would prefer not to do this.  It does make bundling for automation easier -- look, here's every dependency anything in the tree needs -- but it doesn't scale to lots of small parts of the tree, and it definitely doesn't make it really easy to drop Node.js/npm modules into m-c and have them "mostly work".  That's what I'd really like to improve -- making vendoring in dual Node.js/chrome-privileged-Firefox npm modules easier.

Comment 27

[Brian Grinstead [:bgrins]]

(In reply to Nick Alexander :nalexander from comment #26)
> > 2) Set up netmonitor as a dependency in webconsole's package.json (either as
> > a file path or a published npm module) and publish pre-bundled modules that
> > the webconsole uses. I think this means the bundles need to be checked into
> > tree and that at runtime we would not use the authored source from
> > netmonitor but rather the bundled version. When we make changes to the
> > netmonitor source the bundle should automatically be created so that the
> > authored source never falls out of sync with what the webconsole uses, i.e.
> > I think we'd need to integrate the bundling into `./mach build`.
> 
> I very much do not want to depend on Node.js/npm/webpack at |mach build|
> time; that's a much larger and more contentious discussion.
> 
> However, I do think I need to understand what's happening right now:
> 
> 1) if webconsole changes, how does the webpack/update-in-tree process happen?
> 
> 2) if netmonitor changes, how does the webpack/update-in-tree process happen?

So.. webpack is used only when running the tests in node or when doing local development via 'launchpad' (which is a way to develop the tools in a tab instead of having to build firefox). However, webpack doesn't get used when running these panels in the toolbox (inside Firefox), nor during our mochitest-browser tests. In this case, our devtools loader + native JS features are sufficient to run the code directly as it is on disk. That's why there's no checked in bundle for either of these tools.

Note that this is different than the debugger and probably other codebases, where there's a build step who's artifact *is* the bundle - and there is no attempt to run the source files directly in Firefox.

Comment 28

[Alexandre Poirot [:ochameau]]

(In reply to Brian Grinstead [:bgrins] from comment #27)
> Note that this is different than the debugger and probably other codebases,
> where there's a build step who's artifact *is* the bundle - and there is no
> attempt to run the source files directly in Firefox.

No attempt to run the sources files directly in Firefox ++yet++.
In the mono multi repo plans, we would like to have the sources in m-c to stop having to land webpack bundles.
But I have to agree with Nick, we should start with just having a custom mach command before thinking about involving npm in ./mach build.

Comment 29

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Brian Grinstead [:bgrins] from comment #24)
> (In reply to Nick Alexander :nalexander from comment #23)
> > (In reply to Jan Honza Odvarko [:Honza] from comment #20)
> > > (In reply to Nick Alexander :nalexander from comment #18)
> > > > And it almost works!  Except there's some kind of dependency between
> > > > webconsole and netmonitor that's not captured in the package.json
> > > > -- see the error.
> > > Where can I see the error?
> > 
> > It's at
> > 
> > https://public-artifacts.taskcluster.net/EJ4_APSkQOmgYEVZQ5lGYg/0/public/
> > logs/live_backing.log
> > 
> > It's Babel-related, I think -- something about ** for Math.pow.
> 
> IIRC this one is related to the node version. Nicolas, is that right?

It is : http://node.green/#ES2016-features-exponentiation------operator
It was unflagged on node 7.5.0.

Comment 30

[Nick Alexander :nalexander [he/him]]

OK, I looked into this further, and I feel like somebody who really understands the complex web of dependencies in the devtools code needs to figure this out.

The end goal is to have _one_ package.json that makes `{yarn,npm} install && {yarn,npm} test` run the webconsole mocha tests.  (I don't care if it's yarn or npm.  I don't see why we wouldn't use yarn, which just seems better, but it seems like we use a mix throughout the tree.)

I tried two things (after removing the "preinstall" script recently added, which I think is just wrong):

1) add devtools/client/package.json with file:netmonitor and file:webconsole
2) put file:../netmonitor into devtools/client/webconsole/package.json

Option 1) needed work to update the testing commands and a bunch of module remapping, but it clearly should be possible to make it work.  I don't want to dig through a bunch of complicated file path remapping myself to get this working.

Option 2) didn't play nicely with Babel's plugin resolution: there seem to be known issues here and many work-arounds, but again I don't want to try them all myself.

Neither of these things seem impossible -- they're just effort -- but they should be done by somebody who understands the web of dependencies.

It's possible that the end goal I state above is not possible or not worth the effort.  If that's true, convince me!  I really don't think we should scatter this configuration throughout the tree and not use the dependency systems in place (npm/yarn) to you know, track dependencies, but I'm not expert in this area.

Comment 31

[Alexandre Poirot [:ochameau]]

Nick, may be you could post your current patch somewhere so that we can pick it up from here.

Comment 32

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1401189 - Part 1: Add new npm TC kind.

This patch adds a new npm kind.  A single npm declaration corresponds
to a single npm command against a single package.json in the source
tree.  The npm declaration expands to two task declarations:

1) a toolchain task that runs |npm install| and collects the resulting
   `node_modules` directory;

2) another task that installs the `node_modules` directory produced
   by the toolchain task and then runs whatever npm command was
   specified.

This two stage process -- dependency collection before task execution
-- has worked well for the Android-Gradle dependency hierarchy.

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

Comment 33

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1401189 - Part 2: Add webconsole tests.

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

Comment 34

[Nick Alexander :nalexander [he/him]]

Attachment: Try to make |npm install| work in webconsole.

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

Comment 35

[Nick Alexander :nalexander [he/him]]

(In reply to Alexandre Poirot [:ochameau] from comment #31)
> Nick, may be you could post your current patch somewhere so that we can pick
> it up from here.

Oh!  I linked to my try builds, but here's a reviewer-less push to RB:

https://reviewboard.mozilla.org/r/213300

and a try build that fails due to the npm preinstall that was added:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=70451909f58b6c0ad217ae795d08da27b36bc247
 
I also pushed a tiny "don't preinstall, do file: link to netmonitor" commit (that doesn't solve the problem).  Here's the try build -- I expect the TL(webconsole) to succeed and a mocha(webconsole) job that uses it to fail, like my earlier patches:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=124ccdeb4b60c60a82f58674ee4bbe0c7e21ea29

Hope that's enough to get started with.  Just FYI, those try builds are the result of `./mach try fuzzy -q "'npm"` -- and the TL(epm) and mocha(epm) are doing the same process but with eslint-plugin-mozilla (the example I worked from).

Comment 36

[Alexandre Poirot [:ochameau]]

I thought you were contributing to mach, but it looks like you only focused on Taskcluster.
Unless I misunderstood you patch, it doesn't help run console test locally?

Bug 1312823 has a patch to run console tests on TC for a while now, a very simple and naive one (most likely broken now).
I imagine it may be easier to have specific jobs for the webconsole as it requires a more recent node version than the one used on default TaskCluster workers. 
Your TC job runs with node v6.9.1 while nicolas said in comment 29 you need at least node 7.5.0:
  https://searchfox.org/mozilla-central/search?q=6.9.1&case=true&path=

But more importantly, I think we should focus on getting mach support first.
Before running things on the CI, we should ensure any m-c contributor can very easily run anything that runs on CI.
Without having to read any README in sub-folder (without having to run npm install somewhere),
without having to setup any special environment (having ./mach bootstrap or special ./mach eslint setup to do all what is needed).

I imagine that's better to work in this order as mach support may impact how we run tests on TC, while the opposite is unlikely.

What about trying to bump node version used for eslint to 7.5+?
And then see how we can reuse ./mach eslint npm setup for a ./mach npm command?
It would have the advantage of allowing to debug locally...

Comment 37

[Alexandre Poirot [:ochameau]]

FYI: npm install is still broken for the console, I opened bug 1431023. It throws with the same exceptions you get on TC.

Comment 38

[Alexandre Poirot [:ochameau]]

FYI2: bug 1431457 should have fixed the npm setup issue you had on your try run.

Comment 39

[Alexandre Poirot [:ochameau]]

Nick, I pushed your patch with recent tip:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a511bf4ef217c8fced1a59529daf1ff4d68909f&selectedJob=158202006

The npm install job passed, but the npm test failed with:
[task 2018-01-24T14:31:22.355Z] + npm run test
[task 2018-01-24T14:31:22.669Z] 
[task 2018-01-24T14:31:22.669Z] > webconsole@0.0.1 test /builds/worker/checkouts/gecko/devtools/client/webconsole
[task 2018-01-24T14:31:22.669Z] > cross-env NODE_ENV=test NODE_PATH=../../../ mocha new-console-output/test/**/*.test.js --compilers js:babel-register -r jsdom-global/register -r ./new-console-output/test/require-helper.js
[task 2018-01-24T14:31:22.669Z] 
[task 2018-01-24T14:31:27.329Z] [BABEL] Note: The code generator has deoptimised the styling of "/builds/worker/checkouts/gecko/devtools/client/shared/vendor/react-dom.js" as it exceeds the max of "500KB".
[task 2018-01-24T14:31:28.384Z] [BABEL] Note: The code generator has deoptimised the styling of "/builds/worker/checkouts/gecko/devtools/client/shared/vendor/react-dom-server.js" as it exceeds the max of "500KB".
[task 2018-01-24T14:31:29.351Z] /builds/worker/checkouts/gecko/devtools/client/shared/components/reps/reps.js:2174
[task 2018-01-24T14:31:29.351Z]       async function enumIndexedProperties(objectClient, start, end) {
[task 2018-01-24T14:31:29.351Z]             ^^^^^^^^
[task 2018-01-24T14:31:29.351Z] SyntaxError: Unexpected token function
[task 2018-01-24T14:31:29.351Z]     at Object.exports.runInThisContext (vm.js:76:16)


Nicolas, do you think it is related to the node version used on Taskcluster?
  node v6.9.1
  npm  v3.10.8

Comment 40

[Nicolas Chevobbe [:nchevobbe]]

Yes, async functions are supported since 7.10.1 (see http://node.green/#ES2017-features-async-functions)

Comment 41

[Julian Descottes [:jdescottes]]

Nick: Wanted to check if this is blocked on having node & npm in the build system? Any intention to decouple "running mocha tests on try" and "have node/npm in the build system", similar to what is done for eslint?

Comment 42

[Julian Descottes [:jdescottes]]

(some context: I am currently writing tests for a new devtools panel in Bug 1450073, and I would be happy to use mocha/jest if there is CI support)

Comment 43

[Nick Alexander :nalexander [he/him]]

(In reply to Julian Descottes [:jdescottes][:julian] from comment #41)
> Nick: Wanted to check if this is blocked on having node & npm in the build
> system? Any intention to decouple "running mocha tests on try" and "have
> node/npm in the build system", similar to what is done for eslint?

Julian, this is not technically (in both senses -- technology and pedantry) blocked on having Node in the build system.  There are lots of ways to support Node tests, and not all of them require full support in the build system.  It's more that my intentions for supporting tests like these evolved into "full Node support in the build system" before being down-graded to "opt-in Node support in the build system", and along the way my focus changed to producing code using Node processes at build time and not at enabling tests.  I'm working with jlast and dmose on that project; my last try build is at

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d138f854139f2e389867b01d2f2afe59f2975783

and has a green --enable-node-environment artifact build that uses Webpack to produce some Activity Stream code.

What does that mean for tests in this project?  I'd prefer to see you write Mocha/Jest, which is the "end game" here, than to use some Mozilla-specific work-around.  But I understand that not having them run easily in CI is not pleasant.  Unfortunately I can't commit to getting anything landed for this ticket.

dmose: I'm CCing you here for your situational awareness; maybe you could think a little about how to support Activity Stream and these new devtools panels.

Mark Banner (standard8) might also be able to comment on special approaches for these tests, similar to what is done for eslint.

Comment 44

[Mark Banner (:standard8)]

(In reply to Nick Alexander :nalexander from comment #43)
> Mark Banner (standard8) might also be able to comment on special approaches
> for these tests, similar to what is done for eslint.

We've already got builders & support for running mocha based tests for ESLint. However, the part we avoided at the time was integration with ./mach (test) as ESLint isn't generally changed a lot. ahal & others would probably be able to help you hook that up.

Comment 45

[Andrew Halberstadt [:ahal]]

I'd like to see these tests registered in moz.build somehow, which would be a requirement for getting them to run with |mach test|.

A couple counter-points to comment 9:

(In reply to Nick Alexander :nalexander from comment #9)
> I understand why we have .ini files, but this approach makes it very
> difficult to avoid writing one's own test harness.  Honestly, it's not worth
> the effort until you really need to solve the sheriff problem at scale.  We
> should just use Mocha/Jasmine/Jest/anything except our own special snowflake
> until we really, really need to handle INI files, SUPPORT-FILES, etc.

1) Registering tests with moz.build doesn't mean we need to use .ini manifests (e.g reftests)
2) Using .ini manifests doesn't mean we need to write our own test harness (e.g python-tests which use manifestparser + pytest)

I recommend we use .ini manifests to generate a list of tests to run, then forward that list into 'mocha'. This approach is tried and true and will integrate seamlessly with the build system, mach, CI, etc. It's also pretty trivial to set up, I'd be happy to point you in the right direction.

For example, here is the |mach python-test| implementation. It uses .ini manifests defined in moz.build, then forwards them into the 3rd party pytest framework:
https://searchfox.org/mozilla-central/source/python/mach_commands.py#88

Comment 46

[Nick Alexander :nalexander [he/him]]

ato: we talked about vendoring node_modules. There's some WIP code on this ticket that did that, in a pretty similar context. Maybe will inform your work on Puppeteer?