Closed Bug 1436690 Opened 6 years ago Closed 6 years ago

Decouple webconsole launchpad workflow from mocha tests

Categories

(DevTools :: Console, enhancement)

Product:
DevTools
Component:
Console
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: jdescottes, Assigned: nchevobbe)

References

Details

Attachments

(5 files)

Bug 1436690 - Add a webconsole tests dedicated package.json file; .
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1436690 - Switch react (et al.) to version 16.2, and update dependencies; .
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1436690 - Remove unecessary amd load for Reps in GripMessageBody; .
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1436690 - Fix React 16 warning in NetworkMessage component; .
59 bytes, text/x-review-board-request
bgrins
: review+
Details
Bug 1436690 - Fix mocha tests due to React and Enzyme updates; .
59 bytes, text/x-review-board-request
bgrins
: review+
Details 
We have a single package.json for the webconsole responsible both for running mocha tests and for starting the console in launchpad.

https://searchfox.org/mozilla-central/source/devtools/client/webconsole/package.json

As discussed in https://github.com/devtools-html/rfcs/issues/35, launchpad workflows are very optional workflows we are keeping alive for now and maintaining when we have time.

WebConsole mocha tests are being integrated with mach & CI (Bug 1401189 and Bug 1312823). They need to be maintained much more carefully than the launchpad workflow. Moving them to a separate package.json with clear dependencies will make it easier to finish this work.
Would it be possible to run on Node 6.9 by doing this split?
If that's the case, it would help making progress on bug 1401189 and bug 1312823 as ./mach and taskcluster images are still using a 6.9 version of node.
with the proper babel plugins, it might be doable indeed
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
See Also: → 1401189
So this is a WIP, but it is working if you yarn && yarn test from devtools/client/webconsole/new-console-output/test/ .
I installed 6.9.0 with nvm and it seems to work fine.

Next step would be to cleanup webconsole/package.json as well I think, and update the 2 readme.md files we have describing the webconsole project.

Julian, Alex, how does this looks to you ?
I applied Nick's patch to run npm test on TRY, and my patch on top. Let's see how it goes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=106d889e862a42bb94f36997637aa8584978f7ec
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #5)
> I applied Nick's patch to run npm test on TRY, and my patch on top. Let's
> see how it goes:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=106d889e862a42bb94f36997637aa8584978f7ec

So the toolchain task (npm install) is green, but the test fails due to

[task 2018-02-09T08:44:11.702Z] Error: Cannot find module 'jsdom-global/register'

What's happening is that the test task (npm test) is hacking up NODE_PATH=../../.. which is probably causing things to fail.

It's my opinion that the next step is to "regularize" all of these devtools package.json files, so that they could -- in theory -- all be hosted on GH.  That is, make it so that {webconsole, netmonitor} both have independent package.json files (with no path hackery/cross-directory shenanigans) so that |npm install| and then |npm test| just works, without any reference to the rest of mozilla-central.  debugger.html achieves that, AFAICT.  If there's a reason that webconsole and netmonitor can't achieve that, I'd love to understand it!

Sorting this out should green-ify this approach.  Thanks for pushing this forward, Nicolas!
yes, I agree. I didn't put any update this afternoon but I have been working on making this work, with node 6.9.
Babel is causing some issue, but I hope to have something working on Monday
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #7)
> yes, I agree. I didn't put any update this afternoon but I have been working
> on making this work, with node 6.9.
> Babel is causing some issue, but I hope to have something working on Monday

If Node 8 helps, we can easily arrange for that.  I used the lint Docker image (https://searchfox.org/mozilla-central/source/taskcluster/docker/lint/Dockerfile) for convenience, but we can easily use new Docker image with different tooling.
That would be really helpful as it would reduce our need of babel only for the object spread operator
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #9)
> That would be really helpful as it would reduce our need of babel only for
> the object spread operator

Can you try the following patch?

diff --git a/taskcluster/docker/recipes/install-node.sh b/taskcluster/docker/recipes/install-node.sh
--- a/taskcluster/docker/recipes/install-node.sh
+++ b/taskcluster/docker/recipes/install-node.sh
@@ -1,12 +1,12 @@
 #!/bin/bash
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
-# This script installs Node v6.
+# This script installs Node v8.
 
-wget --progress=dot:mega https://nodejs.org/dist/v6.9.1/node-v6.9.1-linux-x64.tar.gz
-echo 'a9d9e6308931fa2a2b0cada070516d45b76d752430c31c9198933c78f8d54b17  node-v6.9.1-linux-x64.tar.gz' | sha256sum -c
-tar -C /usr/local -xz --strip-components 1 < node-v6.9.1-linux-x64.tar.gz
+wget --progress=dot:mega https://nodejs.org/dist/v8.9.4/node-v8.9.4-linux-x64.tar.gz
+echo '21fb4690e349f82d708ae766def01d7fec1b085ce1f5ab30d9bda8ee126ca8fc  node-v8.9.4-linux-x64.tar.gz' | sha256sum -c
+tar -C /usr/local -xz --strip-components 1 < node-v8.9.4-linux-x64.tar.gz
 node -v  # verify
 npm -v

Apply that, then repush your try job.  The docker image should be rebuilt before things proceed.
Hello Nick, I got the tests running locally which is a good start.
But my push to TRY is still failing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b95b37188c032e956ce2be707d102d709665f671&selectedJob=161693042

Here babel complains about a plugin not being found, although it is declared in netmonitor package.json and does work locally.
Is there a special case in how the npm install is done which would explain that modules are not installed in netmonitor ?
Flags: needinfo?(nalexander)
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #11)
> Hello Nick, I got the tests running locally which is a good start.
> But my push to TRY is still failing:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=b95b37188c032e956ce2be707d102d709665f671&selectedJob=1
> 61693042
> 
> Here babel complains about a plugin not being found, although it is declared
> in netmonitor package.json and does work locally.
> Is there a special case in how the npm install is done which would explain
> that modules are not installed in netmonitor ?

I'm glad the Node 8 thing worked; we can think about the right way to expose that to these tests.

In the failing task, I see

[task 2018-02-12T13:28:39.343Z] > cross-env NODE_ENV=test NODE_PATH=../../../../../ mocha "./**/*.test.js" -r babel-register  -r jsdom-global/register -r ./mocha-test-setup.js

that NODE_PATH means that node_modules is looked for somewhere else, right? In $topsrcdir/node_modules, if I'm counting the .. pairs correctly.  That's not going to work, since we have *only* devtools/.../webconsole/node_modules in this invocation.  Can you investigate that?  (Locally, this is like hg clone, hg purge (so there are no node_modules hanging around), cd <webconsole>, npm install, npm test.  So there will be no other node_modules around.  That postinstall command, if it's still around, will not work with the automation setup.
Flags: needinfo?(nalexander)
Comment on attachment 8950243 [details]
Bug 1436690 - Switch react (et al.) to version 16.2, and update dependencies; .

https://reviewboard.mozilla.org/r/219482/#review225700
Attachment #8950243 - Flags: review?(bgrinstead) → review+
Comment on attachment 8950244 [details]
Bug 1436690 - Remove unecessary amd load for Reps in GripMessageBody; .

https://reviewboard.mozilla.org/r/219484/#review225702
Attachment #8950244 - Flags: review?(bgrinstead) → review+
Comment on attachment 8950245 [details]
Bug 1436690 - Fix React 16 warning in NetworkMessage component; .

https://reviewboard.mozilla.org/r/219486/#review225704
Attachment #8950245 - Flags: review?(bgrinstead) → review+
Comment on attachment 8950246 [details]
Bug 1436690 - Fix mocha tests due to React and Enzyme updates; .

https://reviewboard.mozilla.org/r/219488/#review225706

::: commit-message-c01da:1
(Diff revision 1)
> +Bug 1436690 - Fix mocha tests do to React and Enzyme updates; r=bgrins.

Nit: "due to"
Attachment #8950246 - Flags: review?(bgrinstead) → review+
Comment on attachment 8949459 [details]
Bug 1436690 - Add a webconsole tests dedicated package.json file; .

https://reviewboard.mozilla.org/r/218778/#review225708

::: commit-message-0ac95:1
(Diff revision 2)
> +Bug 1436690 - Add a dedicated webconsole tests dedicated package.json file; r=bgrins.

Nit: 'dedicated' is here twice in the commit message
Attachment #8949459 - Flags: review?(bgrinstead) → review+
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/04f9bdfbff97
Add a webconsole tests dedicated package.json file; r=bgrins.
https://hg.mozilla.org/integration/autoland/rev/b7a08fdfe4e0
Switch react (et al.) to version 16.2, and update dependencies; r=bgrins.
https://hg.mozilla.org/integration/autoland/rev/2685a80e5182
Remove unecessary amd load for Reps in GripMessageBody; r=bgrins.
https://hg.mozilla.org/integration/autoland/rev/bb5aaa6697ec
Fix React 16 warning in NetworkMessage component; r=bgrins.
https://hg.mozilla.org/integration/autoland/rev/e2e875344af8
Fix mocha tests due to React and Enzyme updates; r=bgrins.
Blocks: 1438476
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: