[meta] Enable remaining devtools/webconsole tests with e10s

RESOLVED FIXED

Status

defect
RESOLVED FIXED
5 years ago
10 months ago

People

(Reporter: past, Unassigned)

Tracking

(Blocks 2 bugs, {meta})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [e10s-m8])

Attachments

(2 attachments, 3 obsolete attachments)

Bug 1042253 enabled most of the web console tests with e10s, but a few still remain disabled:

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/test/browser.ini
Whiteboard: [e10s-m8]
Summary: Enable remaining devtools/webconsole tests with e10s → [e10s] Enable remaining devtools/webconsole tests with e10s
Assignee: nobody → past
Status: NEW → ASSIGNED
All tests pass locally on linux64 opt.
(In reply to Panos Astithas [:past] from comment #3)
> All tests pass locally on linux64 opt.

Scratch that, I didn't use --e10s.
Posted patch Part 1 v2Splinter Review
This version is less ambitious, but passes in both opt and debug modes in e10s locally.
Attachment #8648921 - Attachment is obsolete: true
Attachment #8651225 - Attachment description: v2 → Part 1 v2
Try is hopeless, but this works well on my system. Let's see what happens.
Backed out for causing frequent Linux32 browser_webconsole_trackingprotection_errors.js shutdown hangs.
https://treeherder.mozilla.org/logviewer.html#?job_id=4348500&repo=fx-team

https://hg.mozilla.org/integration/fx-team/rev/0c36b5a077ee
Weird, this patch didn't actually touch browser_webconsole_trackingprotection_errors.js. It did enable the one before that in browser.ini, but in the above mentioned log that didn't run immediately before browser_webconsole_trackingprotection_errors.js either.
While working on bug 1184172, I realized many webconsole tests
are working correctly on e10s. We just have to prevent calling exceptUncaughtException.
The exception are always thrown in the child process and the test harness
doesn't care about exception happenning in child processes.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc8e237ff746
Attachment #8652369 - Flags: review?(past)
Comment on attachment 8652369 [details] [diff] [review]
Enable expectUncaughtException tests v1

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

::: browser/devtools/webconsole/test/browser.ini
@@ +232,5 @@
>  [browser_webconsole_bug_595223_file_uri.js]
>  [browser_webconsole_bug_595350_multiple_windows_and_tabs.js]
>  skip-if = e10s # Bug 1042253 - webconsole tests disabled with e10s
>  [browser_webconsole_bug_595934_message_categories.js]
> +skip-if = e10s # Bug 1042253 - webconsole tests disabled with e10s

Why isn't this removed? You modified the test as far as I can see.
Attachment #8652369 - Flags: review?(past) → review+
(In reply to Panos Astithas [:past] from comment #12)

> ::: browser/devtools/webconsole/test/browser.ini
> >  [browser_webconsole_bug_595934_message_categories.js]
> > +skip-if = e10s # Bug 1042253 - webconsole tests disabled with e10s
> 
> Why isn't this removed? You modified the test as far as I can see.

This test still fails even with this fix, so there is some more work to enable it.
(In reply to Panos Astithas [:past] from comment #10)
> Weird, this patch didn't actually touch
> browser_webconsole_trackingprotection_errors.js. It did enable the one
> before that in browser.ini, but in the above mentioned log that didn't run
> immediately before browser_webconsole_trackingprotection_errors.js either.

The same patch in a new try run causes intermittent shutdown hangs in browser_webconsole_view_source.js, which immediately follows browser_webconsole_trackingprotection_errors.js:

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

Neither test is enabled (or otherwise directly affected) by this patch.
I'm deleting 2 of the disabled tests in bug 861335.
Assignee: past → nobody
Status: ASSIGNED → NEW
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Depends on: 1241707
Depends on: 1241735
Depends on: 1242716
Depends on: 1242943
stashing a WIP that converts a bunch of the tests
Posted patch webconsole-e10s-more.patch (obsolete) — Splinter Review
Lin, this is the state of my current work.  Can you take a look when you get a chance?  Here's a try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=178dd3a0ca1f.

Also, tests that aren't part of this patch are fair game to work on.  Maybe we should start moving each test into it's own bug to make it easier to track though.
Attachment #8712160 - Attachment is obsolete: true
Attachment #8713028 - Flags: review?(lclark)
Comment on attachment 8713028 [details] [diff] [review]
webconsole-e10s-more.patch

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

I'll be finishing the review later today, but just wanted to get these comments up for discussion.

::: devtools/client/webconsole/test/browser.ini
@@ -162,5 @@
>  [browser_console_dead_objects.js]
>  skip-if = e10s # Bug 1042253 - webconsole tests disabled with e10s
>  [browser_console_copy_entire_message_context_menu.js]
>  [browser_console_error_source_click.js]
> -skip-if = buildapp == 'mulet' || e10s # Bug 1042253 - webconsole e10s tests

I don't know what "mulet" is. Should we leave the skip statement for "mulet"?

@@ -167,3 @@
>  [browser_console_filters.js]
>  [browser_console_iframe_messages.js]
> -skip-if = buildapp == 'mulet' || e10s # Bug 1042253 - webconsole e10s tests

Ditto.

::: devtools/client/webconsole/test/browser_console_error_source_click.js
@@ +24,5 @@
>      ok(hud, "browser console opened");
>  
> +    // On e10s, the exception is triggered in child process
> +    // and is ignored by test harness
> +    if (!Services.appinfo.browserTabsRemoteAutostart) {

From looking around in the code base, it looks like Services.appinfo.browserTabsRemoteAutostart means "e10s is enabled". That isn't obvious to me from the property name. That is a problem outside the scope of this issue. But in this issue, maybe it's worth adding a helper to head.js which makes it explicit (like e10sIsEnabled())? Or mentioning in the comment?

::: devtools/client/webconsole/test/browser_console_iframe_messages.js
@@ +70,5 @@
> +  yield closeConsole();
> +  info("web console closed");
> +
> +  // The browser console doesn't show page's console.log statements in
> +  // e10s windows. See Bug 1241289.

Nit. Should this be a TODO that is dependent on Bug 1241289?

@@ +79,5 @@
> +  }
> +});
> +
> +function* testWebConsole(hud) {
> +  ok(hud, "web console opened");

Nit. Should ok statements live next to their corresponding yield? e.g. let hud = yield openConsole();
I had a convo with Alex about mulet and have created this bug to address it: Bug 1243777.
(In reply to Brian Grinstead [:bgrins] from comment #20)
> Here's a try push
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=178dd3a0ca1f.

Grr looks like this is perma-oranging browser_perf-overview-render-02.js somehow.. Maybe Bug 1240509?  Or Bug 1204174
Depends on: 1240509, 1204174
Comment on attachment 8713028 [details] [diff] [review]
webconsole-e10s-more.patch

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

Here are a couple more questions. I'm setting back to needs work due to the perma-orange that you pointed out.

::: devtools/client/webconsole/test/browser_console_variables_view_while_debugging.js
@@ +25,5 @@
> +
> +function* waitForFrameAdded(dbgPanel) {
> +  let thread = dbgPanel.panelWin.DebuggerController.activeThread;
> +
> +  info("Waiting for onFramesAdded");

Nit. onFramesAdded is the name of a function that's being removed. Maybe change to "Waiting for framesadded"?

::: devtools/client/webconsole/test/browser_console_variables_view_while_debugging_and_inspecting.js
@@ +26,5 @@
> +
> +function* waitForFrameAdded(dbgPanel) {
> +  let thread = dbgPanel.panelWin.DebuggerController.activeThread;
> +
> +  info("Waiting for onFramesAdded");

See above.

::: devtools/client/webconsole/test/browser_webconsole_bug_588342_document_focus.js
@@ +17,3 @@
>  
> +  // This is skipped in e10s due to Bug 1241707 not properly restoring
> +  // focus.

Nit. Should this be a TODO?

::: devtools/client/webconsole/test/browser_webconsole_bug_593003_iframe_wrong_hud.js
@@ +20,3 @@
>  
> +  let tab1 = (yield loadTab(TEST_URI)).tab;
> +  content.console.log("FOO");

Should this be in a ContentTask.spawn call?

@@ +32,5 @@
> +  yield checkMessages(tab1, tab2);
> +
> +  info("Cleaning up");
> +  yield closeConsole(tab1);
> +  yield closeConsole(tab2);

Should this go in a registerCleanupFunction?
Attachment #8713028 - Flags: review?(lclark) → review-
I've added issues for all of the tests in https://docs.google.com/spreadsheets/d/10UeyRoiWV2HjkWwAU51HXyXAV7YLi4BjDm55mr5Xv6c/edit#gid=1777180571

Here are the numbers for the tests in the WIP patch. I'm guessing moving them each to their own bug would have the extra advantage of making it easier to land the ones that don't cause perma-orange? But if we don't want to split up this patch, we could close these ones out.

Bug 1243963 - browser_console_error_source_click.js
Bug 1243983 - browser_console_iframe_messages.js
Bug 1243995 - browser_console_optimized_out_vars.js
Bug 1243977 - browser_console_variables_view.js
Bug 1243968 - browser_console_variables_view_while_debugging.js 
Bug 1243962 - browser_console_variables_view_while_debugging_and_inspecting.js 
Bug 1243959 - browser_webconsole_autocomplete_in_debugger_stackframe.js
Bug 1243992 - browser_webconsole_bug_585991_autocomplete_keys.js
Bug 1243958 - browser_webconsole_bug_588342_document_focus.js
Bug 1243984 - browser_webconsole_bug_593003_iframe_wrong_hud.js
Bug 1243970 - browser_webconsole_bug_597103_deactivateHUDForContext_unfocused_window.js
New try push that looks better (still a lot of perf errors but not perma-oranging them: https://treeherder.mozilla.org/#/jobs?repo=try&revision=98c182a1c579&selectedJob=16073959
(In reply to Lin Clark [:linclark] from comment #21)
> ::: devtools/client/webconsole/test/browser_console_error_source_click.js
> @@ +24,5 @@
> >      ok(hud, "browser console opened");
> >  
> > +    // On e10s, the exception is triggered in child process
> > +    // and is ignored by test harness
> > +    if (!Services.appinfo.browserTabsRemoteAutostart) {
> 
> From looking around in the code base, it looks like
> Services.appinfo.browserTabsRemoteAutostart means "e10s is enabled". That
> isn't obvious to me from the property name. That is a problem outside the
> scope of this issue. But in this issue, maybe it's worth adding a helper to
> head.js which makes it explicit (like e10sIsEnabled())? Or mentioning in the
> comment?

Probably makes sense as a constant in shared-head (or to revisit and see if there's a better named constant available now on Services).  Let's move that into a follow up since these calls are all over our tests (not just in webconsole).

> ::: devtools/client/webconsole/test/browser_console_iframe_messages.js
> @@ +70,5 @@
> > +  yield closeConsole();
> > +  info("web console closed");
> > +
> > +  // The browser console doesn't show page's console.log statements in
> > +  // e10s windows. See Bug 1241289.
> 
> Nit. Should this be a TODO that is dependent on Bug 1241289?

Yes thanks, I've updated that.

> @@ +79,5 @@
> > +  }
> > +});
> > +
> > +function* testWebConsole(hud) {
> > +  ok(hud, "web console opened");
> 
> Nit. Should ok statements live next to their corresponding yield? e.g. let
> hud = yield openConsole();

Moved it
Comment on attachment 8713028 [details] [diff] [review]
webconsole-e10s-more.patch

Moved this mega patch into the bugs in comment 25
Attachment #8713028 - Attachment is obsolete: true
Converting this to a meta bug
Assignee: bgrinstead → nobody
Status: ASSIGNED → NEW
Keywords: leave-openmeta
Summary: [e10s] Enable remaining devtools/webconsole tests with e10s → [meta] Enable remaining devtools/webconsole tests with e10s
Depends on: 1247962
Depends on: 1255871
Depends on: 1310593

Updated

11 months ago
Product: Firefox → DevTools
No longer depends on: 1240509
All blockers are fixed
Status: NEW → RESOLVED
Last Resolved: 10 months ago
tracking-e10s: + → ---
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.