1034512 - Enable 'shared' tests with e10s

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Joe Walker [:jwalker]]

+++ This bug was initially created as a clone of Bug #1034511 +++

Comment 1

[Chris Peterson [:cpeterson]]

Moving DevTools test bugs to e10s milestone M7 (i.e. not blocking e10s merge to Aurora).

Comment 2

[Patrick Brosset <:pbro>]

Joe, stealing this bug from you. I don't think you intended it to be assigned to you in the first place since I don't see the assigned flag having changed, that probably came from cloning the bug.

There's a bunch of cubic-bezier tests in the /shared/ folder that I know how to fix for e10s, starting with these now.

Comment 3

[Patrick Brosset <:pbro>]

Attachment: bug1034512-1-enable-shared-test-e10s.patch

Comment 4

[Patrick Brosset <:pbro>]

Attachment: bug1034512-2-fix-cubicbezierwidget-tests.patch

Comment 5

[Patrick Brosset <:pbro>]

Attachment: bug1034512-3-use-add_task.patch

Comment 6

[Patrick Brosset <:pbro>]

Attachment: bug1034512-4-fix-inplace-editor.patch

Comment 7

[Patrick Brosset <:pbro>]

Attachment: bug1034512-5-fix-output-parser.patch

Comment 8

[Patrick Brosset <:pbro>]

Attachment: bug1034512-6-fix-spectrum.patch

Comment 9

[Patrick Brosset <:pbro>]

So these first 6 patches make more than 50% of the tests in this folder pass with e10s.
One of them also refactors most of the async tests to use add_task.

Next up are the telemetry tests. I'll start working on them tomorrow.

Comment 10

[Patrick Brosset <:pbro>]

A lot of these tests were loading common devtools widgets into a content page, and testing them from there.
With e10s, this breaks in various ways.
I've changed several of the tests to run instead in a toolbox host, which is something that runs in the chrome of the browser, so in the parent process. This is better because that's where they are used anyway.

Comment 11

[Patrick Brosset <:pbro>]

Attachment: bug1034512-7-fix-telemetry-tests.patch

Makes the Telemetry-related tests in browser/shared/devtools/test/ run with e10s.

Worth noting:

- I had to disable a couple of them (browser_telemetry_button_paintflashing and browser_telemetry_button_responsive) because the corresponding tool don't work with e10s.
- I removed a large amount of duplicated code from these tests.
- I rewrote them to use add_task.

Comment 12

[Patrick Brosset <:pbro>]

Attachment: bug1034512-8-templater-test-cpow.patch

The templater test already works fine with e10s, but it makes a lot of accesses to the content DOM through CPOWs. So this patch appends the rendered template into a toolbox host instead of in the content tab.
I also removed the need for the deprecated sync thenable.

Comment 13

[Patrick Brosset <:pbro>]

Attachment: bug1034512-3-use-add_task v2.patch

rebased part 3.

Comment 14

[Patrick Brosset <:pbro>]

Attachment: bug1034512-9-fix-treewidget-tests.patch

Fixed the treeWidget tests for e10s.

Comment 15

[Patrick Brosset <:pbro>]

Attachment: bug1034512-10-rewrite-browser_toolbar_webconsole_errors_count-test.patch

part 10 - The last remaining test was browser_toolbar_webconsole_errors_count.js.

This was a hard one, primarily because it's a very long test, and because it didn't even pass for me locally without e10s!! This is odd, because this test isn't disabled, so it must be passing for other people.

To make it pass again, I had to dive deep in its code, and pretty rewrite the whole thing, making extensive use a tasks/promises, flattening the code, adding info statement all over. I also added a couple of frame-script messageManager magic, because the test was accessing the content page for various stuff.

I got it passing locally without e10s, but it looks like the error counter in the gcli developertoolbar doesn't work well with e10s, so I'm going to have to disable the test for e10s anyway.

Comment 16

[Patrick Brosset <:pbro>]

Attachment: bug1034512-10-rewrite-browser_toolbar_webconsole_errors_count-test.patch

Disabling the test with e10s

Comment 17

[Patrick Brosset <:pbro>]

Attachment: bug1034512-11-cleanup-browser_options-view-01.js.patch

part 11 - Cleaned up and fixed a test that was introduced while I was working on this bug: browser_options-view-01.js

Comment 18

[Patrick Brosset <:pbro>]

Comment on attachment 8547636 [details] [diff] [review]
bug1034512-2-fix-cubicbezierwidget-tests.patch

Hey Brian, this makes the cubicbezierwidget tests pass with e10s by making them run in a toolbox host window instead of inside the content window.
Also made the tests use add_task.

Comment 19

[Patrick Brosset <:pbro>]

Comment on attachment 8551278 [details] [diff] [review]
bug1034512-3-use-add_task v2.patch

More add_task all around.

Comment 20

[Patrick Brosset <:pbro>]

Comment on attachment 8547638 [details] [diff] [review]
bug1034512-4-fix-inplace-editor.patch

Joe, this makes the inplace-editor test pass with E10S by making it run inside a toolbox host window rather than inside a content window.
All devtools widgets ultimately run inside toolbox hosts anyway, and I think it makes more sense to be running those tests there rather than re-writing them all through framescripts.

Comment 21

[Patrick Brosset <:pbro>]

Comment on attachment 8547639 [details] [diff] [review]
bug1034512-5-fix-output-parser.patch

Sorry Mike for stacking yet another review on your list :( but you're the one who knows the output parser the best.

Changes in this patch:
- disabled a couple of layouthelpers tests with e10s for now, will file a bug to migrate them over
- re-wrote a little bit the outputparser test:
  - used the async test add_task test runner
  - made the test run in a toolbox host window instead of a content window
  - move all test step function calls into the main performTest function instead of having each step call the next one (I think this is more readable this way).

Comment 22

[Patrick Brosset <:pbro>]

Comment on attachment 8547640 [details] [diff] [review]
bug1034512-6-fix-spectrum.patch

This patch makes the spectrum (color picker) tests pass with e10s.
- used add_task
- moved all test step functions in the main performTest function for easier readability
- made the test run in a toolbox host window
- also removed the synthesized event to change the color to make it simpler by calling the callback directly. I've not grown very fond of synthesizing events in tests, I found they often end up being problematic, and I don't think it's a big deal calling event handlers directly in tests because testing the event -> handler part isn't very useful.

Comment 23

[Patrick Brosset <:pbro>]

Comment on attachment 8550296 [details] [diff] [review]
bug1034512-7-fix-telemetry-tests.patch

I would have flagged Mike for review for this one but his review queue is overflowing right now.
Here's a description of the changes here:

- I disabled a couple of telemetry tests for paintflashing and responsiveview because these tools don't work with e10s yet.
- I rewrote a large part of the tests:
  - cleaned up useless imports
  - made use of add_task and promiseTab to simplify a lot the setup/tearDown phases of each test
  - moved an important amount of code to head.js as it was duplicated in pretty much all of the telemetry tests
 
This might look like a lot of changes, but once you've looked at one test, you'll see the other more or less follow the exact same pattern.

Comment 24

[Patrick Brosset <:pbro>]

Comment on attachment 8551698 [details] [diff] [review]
bug1034512-11-cleanup-browser_options-view-01.js.patch

Jordan, after a lot of changes made to the devtools/shared/test directory, I had to modify the browser_options-view-01.js test too to:

- make it run in a toolbox host window instead of content window, to make sure it works well with e10s, without having to go through CPOWs,
- wrap it in add_task for consistency and better error handling,
- remove a couple of unused functions.

Comment 25

[Patrick Brosset <:pbro>]

Comment on attachment 8547635 [details] [diff] [review]
bug1034512-1-enable-shared-test-e10s.patch

r=me for this test-enabling-only patch (just removed the skip-if).
However, I need to make sure this patch goes in last.

Comment 26

[Patrick Brosset <:pbro>]

Pending try build for all 11 parts: https://treeherder.mozilla.org/#/jobs?repo=try&revision=af551cda3239

Comment 27

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8551698 [details] [diff] [review]
bug1034512-11-cleanup-browser_options-view-01.js.patch

Review of attachment 8551698 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

::: browser/devtools/shared/test/browser_options-view-01.js
@@ +81,3 @@
>    return new OptionsView({
>      branchName: BRANCH,
> +    window: win,

nit: In a recent patch, `window` option was removed from the OptionsView constructor as we can get this from the `menupopup`

Comment 28

[Patrick Brosset <:pbro>]

Attachment: bug1034512-11-cleanup-browser_options-view-01.js v2.patch

Thanks Jordan. I removed the extra window property.

Comment 29

[Brian Grinstead [:bgrins]]

Comment on attachment 8547636 [details] [diff] [review]
bug1034512-2-fix-cubicbezierwidget-tests.patch

Review of attachment 8547636 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/shared/test/browser_cubic-bezier-01.js
@@ +7,5 @@
>  // Tests that the CubicBezierWidget generates content in a given parent node
>  
>  const TEST_URI = "chrome://browser/content/devtools/cubic-bezier-frame.xhtml";
>  const {CubicBezierWidget} = devtools.require("devtools/shared/widgets/CubicBezierWidget");
> +const {DOMHelpers} = Cu.import("resource:///modules/devtools/DOMHelpers.jsm", {});

Do you need {DOMHelpers} and {Hosts} in any of these 3 files?  I don't see any other references to them

Comment 30

[Brian Grinstead [:bgrins]]

Comment on attachment 8547640 [details] [diff] [review]
bug1034512-6-fix-spectrum.patch

Review of attachment 8547640 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/shared/test/browser_spectrum.js
@@ +5,5 @@
>  // Tests that the spectrum color picker works correctly
>  
>  const TEST_URI = "chrome://browser/content/devtools/spectrum-frame.xhtml";
>  const {Spectrum} = devtools.require("devtools/shared/widgets/Spectrum");
> +const {DOMHelpers} = Cu.import("resource:///modules/devtools/DOMHelpers.jsm", {});

Here are DomHelpers and Hosts again - should these just be moved into head.js?

@@ +91,3 @@
>  
> +    // Simulate a drag move event by calling the handler directly.
> +    s.onDraggerMove(s.dragger.offsetWidth/2, s.dragger.offsetHeight/2);

Now that this is in a host, we could stick with the eventutils calls in theory, right?  Keeping that could help possibly add test coverage to the case where onDraggerMove is inappropriately bound to the element.  I'm fairly sure that executesoon is unnecessary, so it would (in theory) be just a matter of switching this line out with:

EventUtils.synthesizeMouse(s.dragger, s.dragger.offsetWidth/2, s.dragger.offsetHeight/2, {}, doc.defaultView);

Not a huge deal, just curious why you switched to calling the handler directly.

Comment 31

[Brian Grinstead [:bgrins]]

Comment on attachment 8551278 [details] [diff] [review]
bug1034512-3-use-add_task v2.patch

Review of attachment 8551278 [details] [diff] [review]:
-----------------------------------------------------------------

Nice

Comment 32

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #29)
> Comment on attachment 8547636 [details] [diff] [review]
> bug1034512-2-fix-cubicbezierwidget-tests.patch
> 
> Review of attachment 8547636 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/shared/test/browser_cubic-bezier-01.js
> @@ +7,5 @@
> >  // Tests that the CubicBezierWidget generates content in a given parent node
> >  
> >  const TEST_URI = "chrome://browser/content/devtools/cubic-bezier-frame.xhtml";
> >  const {CubicBezierWidget} = devtools.require("devtools/shared/widgets/CubicBezierWidget");
> > +const {DOMHelpers} = Cu.import("resource:///modules/devtools/DOMHelpers.jsm", {});
> 
> Do you need {DOMHelpers} and {Hosts} in any of these 3 files?  I don't see
> any other references to them
In fact |createHost| needs them to be imported. These imports should really be in head.js in fact. I will move them there.

Comment 33

[Patrick Brosset <:pbro>]

Attachment: bug1034512-2-fix-cubicbezierwidget-tests v2.patch

Moved the 2 imports needed for createHost into head.js.

Comment 34

[Patrick Brosset <:pbro>]

Attachment: bug1034512-4-fix-inplace-editor v2.patch

Removed the 2 createHost imports now that they are in head.js.

Comment 35

[Patrick Brosset <:pbro>]

Attachment: bug1034512-5-fix-output-parser v2.patch

Removed 2 useless imports.

Comment 36

[Patrick Brosset <:pbro>]

Attachment: bug1034512-6-fix-spectrum v2.patch

(In reply to Brian Grinstead [:bgrins] from comment #30)
> Comment on attachment 8547640 [details] [diff] [review]
> bug1034512-6-fix-spectrum.patch
> 
> Review of attachment 8547640 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/shared/test/browser_spectrum.js
> @@ +5,5 @@
> >  // Tests that the spectrum color picker works correctly
> >  
> >  const TEST_URI = "chrome://browser/content/devtools/spectrum-frame.xhtml";
> >  const {Spectrum} = devtools.require("devtools/shared/widgets/Spectrum");
> > +const {DOMHelpers} = Cu.import("resource:///modules/devtools/DOMHelpers.jsm", {});
> 
> Here are DomHelpers and Hosts again - should these just be moved into
> head.js?
Done.

> @@ +91,3 @@
> >  
> > +    // Simulate a drag move event by calling the handler directly.
> > +    s.onDraggerMove(s.dragger.offsetWidth/2, s.dragger.offsetHeight/2);
> 
> Now that this is in a host, we could stick with the eventutils calls in
> theory, right?  Keeping that could help possibly add test coverage to the
> case where onDraggerMove is inappropriately bound to the element.  I'm
> fairly sure that executesoon is unnecessary, so it would (in theory) be just
> a matter of switching this line out with:
> 
> EventUtils.synthesizeMouse(s.dragger, s.dragger.offsetWidth/2,
> s.dragger.offsetHeight/2, {}, doc.defaultView);
> 
> Not a huge deal, just curious why you switched to calling the handler
> directly.
I can't explain it, but the synthesized event doesn't work at all after moving this test to the host. The event just doesn't get handled by the widget. That's why I ended up calling the onDraggerMove callback directly.

Comment 37

[Patrick Brosset <:pbro>]

Attachment: bug1034512-7-fix-telemetry-tests v2.patch

Rebased.

Comment 38

[Patrick Brosset <:pbro>]

Attachment: bug1034512-8-templater-test-cpow v2.patch

Removed 2 useless imports now that they are in head.js.
See comment 12 for more info about changes in this patch.

Comment 39

[Patrick Brosset <:pbro>]

Attachment: bug1034512-9-fix-treewidget-tests v2.patch

Removed 2 useless imports.

Comment 40

[Joe Walker [:jwalker]]

Comment on attachment 8552371 [details] [diff] [review]
bug1034512-8-templater-test-cpow v2.patch

Review of attachment 8552371 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/shared/test/browser_templater_basic.js
@@ +282,5 @@
>  
>  function delayReply(data) {
> +  return new Promise(resolve => {
> +    executeSoon(() => {
> +      resolve(data);

I don't think we need this now, since promises are async by nature, but maybe it's now worth taking out.

Comment 41

[Patrick Brosset <:pbro>]

Attachment: bug1034512-11-cleanup-browser_options-view-01.js v3.patch

Removed 2 useless imports.

Comment 42

[Patrick Brosset <:pbro>]

Attachment: bug1034512-12-removed-toolboxhost-imports.patch

Hey Victor, several new tests in this directory needed to call createHost, and so I've moved the 2 related imports (Hosts and DOMHelpers) to head.js. 
This patch is simply about removing the duplicated imports from the test files they remained in.

Comment 43

[Patrick Brosset <:pbro>]

Attachment: bug1034512-8-templater-test-cpow v3.patch

Thanks Joe. And yes, you're right, we can get rid of the executeSoon function call. I've removed it here.

Comment 44

[Patrick Brosset <:pbro>]

Attachment: bug1034512-13-cleaned-up-browser_theme-test.patch

Not directly related to e10s, but the theme test had some unneeded imports and calls to waitForExplicitFinish. So I cleaned it up a bit.

Comment 45

[Patrick Brosset <:pbro>]

Attachment: bug1034512-14-fix-css-color-test.patch

Part 14 fixes browser_css_color.js with e10s.
It was passing with e10s but was intermittently timing out. That's because it was accessing the content document to manipulate a canvas element a lot of times (iterating over colors), and with e10s, each and every access to the content page had to be handled through CPOWs, making the test take a big amount of time.
It now runs in a toolbox host window instead.
I've also made it use add_task.

Comment 46

[Patrick Brosset <:pbro>]

In my last try build (https://treeherder.mozilla.org/#/jobs?repo=try&revision=af551cda3239), browser_css_color.js was still failing intermittently with e10s, but I believe I have fixed it with attachment 8552424 [details] [diff] [review].
The other failing test is browser_outputparser.js and it also fails intermittently with e10s, but I'm not sure why.
Since this bug already has 14 parts, I'm thinking I will disable it for e10s for now and file a separate bug to fix this test instead.

Comment 47

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #46)
> In my last try build
> (https://treeherder.mozilla.org/#/jobs?repo=try&revision=af551cda3239),
> browser_css_color.js was still failing intermittently with e10s, but I
> believe I have fixed it with attachment 8552424 [details] [diff] [review].
> The other failing test is browser_outputparser.js and it also fails
> intermittently with e10s, but I'm not sure why.
> Since this bug already has 14 parts, I'm thinking I will disable it for e10s
> for now and file a separate bug to fix this test instead.
Filed bug 1124162

Comment 48

[Patrick Brosset <:pbro>]

Pending try build for all patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c22c9b1821d3

Comment 49

[Victor Porof [:vporof][:vp]]

Comment on attachment 8552387 [details] [diff] [review]
bug1034512-12-removed-toolboxhost-imports.patch

Review of attachment 8552387 [details] [diff] [review]:
-----------------------------------------------------------------

r+ if tests still pass

Comment 50

[Patrick Brosset <:pbro>]

I have filed bug 1125768 for test browser_toolbar_webconsole_errors_count.js instead of asking for a review for it here.
I have disabled the test for e10s for now anyway since the feature it is testing doesn't seem to work with e10s anyway.

Now that all other patches have been R+'d, I'll just do a little renaming, re-ordering of patches, and attach them all here again (carrying R+s over).

In the meantime, here's a final TRY push for all patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8945b244372

Comment 51

[Patrick Brosset <:pbro>]

Attachment: bug1034512-1-fix-cubicbezierwidget-tests.patch

Comment 52

[Patrick Brosset <:pbro>]

Attachment: bug1034512-2-use-add_task.patch

Comment 53

[Patrick Brosset <:pbro>]

Attachment: bug1034512-3-fix-inplace-editor.patch

Comment 54

[Patrick Brosset <:pbro>]

Attachment: bug1034512-4-fix-output-parser.patch

Comment 55

[Patrick Brosset <:pbro>]

Attachment: bug1034512-5-fix-spectrum.patch

Comment 56

[Patrick Brosset <:pbro>]

Attachment: bug1034512-6-fix-telemetry-tests.patch

Comment 57

[Patrick Brosset <:pbro>]

Attachment: bug1034512-7-templater-test-cpow.patch

Comment 58

[Patrick Brosset <:pbro>]

Attachment: bug1034512-8-fix-treewidget-tests.patch

Comment 59

[Patrick Brosset <:pbro>]

Attachment: bug1034512-9-cleanup-browser_options-view-01.js.patch

Comment 60

[Patrick Brosset <:pbro>]

Attachment: bug1034512-10-removed-toolboxhost-imports.patch

Comment 61

[Patrick Brosset <:pbro>]

Attachment: bug1034512-11-cleaned-up-browser_theme-test.patch

Comment 62

[Patrick Brosset <:pbro>]

Attachment: bug1034512-12-fix-css-color-test.patch

Comment 63

[Patrick Brosset <:pbro>]

Attachment: bug1034512-13-enable-shared-test-e10s.patch

Comment 64

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/7c79302f32de
https://hg.mozilla.org/integration/fx-team/rev/ec68d1b95880
https://hg.mozilla.org/integration/fx-team/rev/5336ba11be5d
https://hg.mozilla.org/integration/fx-team/rev/a7384e567922
https://hg.mozilla.org/integration/fx-team/rev/ce2241501510
https://hg.mozilla.org/integration/fx-team/rev/d1c55bb95e67
https://hg.mozilla.org/integration/fx-team/rev/394aac76bf61
https://hg.mozilla.org/integration/fx-team/rev/132b8377fb88
https://hg.mozilla.org/integration/fx-team/rev/14c3ad8f6146
https://hg.mozilla.org/integration/fx-team/rev/dc0d932d2ecf
https://hg.mozilla.org/integration/fx-team/rev/9ad5627c881d
https://hg.mozilla.org/integration/fx-team/rev/9201029f4d48
https://hg.mozilla.org/integration/fx-team/rev/252f2c47f271

Comment 65

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/7c79302f32de
https://hg.mozilla.org/mozilla-central/rev/ec68d1b95880
https://hg.mozilla.org/mozilla-central/rev/5336ba11be5d
https://hg.mozilla.org/mozilla-central/rev/a7384e567922
https://hg.mozilla.org/mozilla-central/rev/ce2241501510
https://hg.mozilla.org/mozilla-central/rev/d1c55bb95e67
https://hg.mozilla.org/mozilla-central/rev/394aac76bf61
https://hg.mozilla.org/mozilla-central/rev/132b8377fb88
https://hg.mozilla.org/mozilla-central/rev/14c3ad8f6146
https://hg.mozilla.org/mozilla-central/rev/dc0d932d2ecf
https://hg.mozilla.org/mozilla-central/rev/9ad5627c881d
https://hg.mozilla.org/mozilla-central/rev/9201029f4d48
https://hg.mozilla.org/mozilla-central/rev/252f2c47f271