Closed
Bug 1112599
Opened 9 years ago
Closed 5 years ago
[meta] Enable remaining devtools/webconsole tests with e10s
Categories
(DevTools :: Console, defect)
DevTools
Console
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: past, Unassigned)
References
(Blocks 1 open bug)
Details
(Keywords: meta, Whiteboard: [e10s-m8])
Attachments
(2 files, 3 obsolete files)
7.91 KB,
patch
|
Details | Diff | Splinter Review | |
20.47 KB,
patch
|
past
:
review+
ochameau
:
checkin+
|
Details | Diff | Splinter Review |
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
Updated•9 years ago
|
tracking-e10s:
--- → +
Updated•9 years ago
|
Whiteboard: [e10s-m8]
Updated•9 years ago
|
Summary: Enable remaining devtools/webconsole tests with e10s → [e10s] Enable remaining devtools/webconsole tests with e10s
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → past
Status: NEW → ASSIGNED
Reporter | ||
Comment 1•8 years ago
|
||
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=65cc1b187738
Reporter | ||
Comment 2•8 years ago
|
||
Tentative patch.
Reporter | ||
Comment 3•8 years ago
|
||
All tests pass locally on linux64 opt.
Reporter | ||
Comment 4•8 years ago
|
||
(In reply to Panos Astithas [:past] from comment #3) > All tests pass locally on linux64 opt. Scratch that, I didn't use --e10s.
Reporter | ||
Comment 5•8 years ago
|
||
This version is less ambitious, but passes in both opt and debug modes in e10s locally.
Attachment #8648921 -
Attachment is obsolete: true
Reporter | ||
Comment 6•8 years ago
|
||
Let's see if try has recovered: https://treeherder.mozilla.org/#/jobs?repo=try&revision=56895c5184f8
Reporter | ||
Updated•8 years ago
|
Attachment #8651225 -
Attachment description: v2 → Part 1 v2
Reporter | ||
Comment 8•8 years ago
|
||
Try is hopeless, but this works well on my system. Let's see what happens.
Reporter | ||
Updated•8 years ago
|
Keywords: leave-open
Comment 9•8 years ago
|
||
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
Reporter | ||
Comment 10•8 years ago
|
||
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.
Comment 11•8 years ago
|
||
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
Updated•8 years ago
|
Attachment #8652369 -
Flags: review?(past)
Reporter | ||
Comment 12•8 years ago
|
||
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+
Comment 13•8 years ago
|
||
(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.
Updated•8 years ago
|
Attachment #8652369 -
Flags: checkin+
Comment 15•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/11676835290d
Flags: in-testsuite+
Reporter | ||
Comment 16•8 years ago
|
||
(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.
Reporter | ||
Comment 17•8 years ago
|
||
Some of the try runs I did lately to test some theories: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b26a11dcb39 https://treeherder.mozilla.org/#/jobs?repo=try&revision=e458f7c96ad0 https://treeherder.mozilla.org/#/jobs?repo=try&revision=4990406ed58b https://treeherder.mozilla.org/#/jobs?repo=try&revision=e178ea4d5e66 https://treeherder.mozilla.org/#/jobs?repo=try&revision=a53ec41da571 I think we have to make the harness wait a bit longer in some of these tests. linux32 with e10s is very slow.
Reporter | ||
Comment 18•8 years ago
|
||
I'm deleting 2 of the disabled tests in bug 861335.
Reporter | ||
Updated•8 years ago
|
Assignee: past → nobody
Status: ASSIGNED → NEW
Updated•8 years ago
|
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Comment 19•8 years ago
|
||
stashing a WIP that converts a bunch of the tests
Comment 20•8 years ago
|
||
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 21•8 years ago
|
||
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();
Comment 22•8 years ago
|
||
I had a convo with Alex about mulet and have created this bug to address it: Bug 1243777.
Comment 23•8 years ago
|
||
(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
Comment 24•8 years ago
|
||
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-
Updated•8 years ago
|
Updated•8 years ago
|
Updated•8 years ago
|
Updated•8 years ago
|
Comment 25•8 years ago
|
||
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
Comment 26•8 years ago
|
||
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
Comment 27•8 years ago
|
||
(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 28•8 years ago
|
||
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
Comment 29•8 years ago
|
||
Converting this to a meta bug
Assignee: bgrinstead → nobody
Status: ASSIGNED → NEW
Keywords: leave-open → meta
Summary: [e10s] Enable remaining devtools/webconsole tests with e10s → [meta] Enable remaining devtools/webconsole tests with e10s
Updated•5 years ago
|
Product: Firefox → DevTools
Comment 30•5 years ago
|
||
All blockers are fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•