Enable devtools/webconsole tests with e10s

RESOLVED FIXED in Firefox 37

Status

defect
P2
normal
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: msucan, Assigned: harth)

Tracking

(Blocks 2 bugs)

Trunk
Firefox 37
Dependency tree / graph

Firefox Tracking Flags

(e10s+)

Details

(Whiteboard: [e10s-m7][status:inflight])

Attachments

(1 attachment, 23 obsolete attachments)

445.05 KB, patch
Details | Diff | Splinter Review
Reporter

Description

5 years ago
No description provided.
Whiteboard: [e10s][status:inflight]
Reporter

Comment 1

5 years ago
Posted patch bug1042253-wip1.diff (obsolete) — Splinter Review
Reporter

Updated

5 years ago
Depends on: 1049103
Reporter

Comment 2

5 years ago
Posted patch bug1042253-wip2.diff (obsolete) — Splinter Review
More tests fixed.
Attachment #8466440 - Attachment is obsolete: true
Reporter

Updated

5 years ago
Depends on: 1050243
Reporter

Comment 3

5 years ago
Posted patch bug1042253-wip3.diff (obsolete) — Splinter Review
More tests running in e10s mode.
Attachment #8467998 - Attachment is obsolete: true
Assignee: mihai.sucan → nobody
Status: ASSIGNED → NEW
Posted patch webconsole-e10s-WIP4.patch (obsolete) — Splinter Review
Rebased, fixed a couple more tests, and enabled a few that were fine
Attachment #8470448 - Attachment is obsolete: true
Posted patch webconsole-e10s-WIP5.patch (obsolete) — Splinter Review
Fixed a couple of more, found a weird one that passes when run alone but fails when run in the group, so I left it semicolon'd out for now with a comment in the browser.ini file
Attachment #8474895 - Attachment is obsolete: true
Posted patch webconsole-e10s-WIP6.patch (obsolete) — Splinter Review
Fixed 18 additional console tests.
Attachment #8475601 - Attachment is obsolete: true
Posted patch webconsole-e10s-WIP7.patch (obsolete) — Splinter Review
More tests enabled
Attachment #8476029 - Attachment is obsolete: true
Taking a break from working on these tests.
Assignee

Comment 9

5 years ago
Posted patch webconsole-e10s-WIP8.patch (obsolete) — Splinter Review
Fix one more test, fire events in content and not from the test.
Attachment #8476144 - Attachment is obsolete: true
Assignee

Comment 10

5 years ago
Posted patch webconsole-e10s-WIP9.patch (obsolete) — Splinter Review
About 10 more tests.
Attachment #8476398 - Attachment is obsolete: true
Assignee

Comment 11

5 years ago
Posted patch webconsole-e10s-WIP10.patch (obsolete) — Splinter Review
Fixes a few more. Getting an error from a previously-enabled test:

737 INFO Full message: TypeError: toolbox.getPanel(...) is undefined
3738 INFO Full stack: runner@chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_webconsole_split_persist.js:41:21

Someone else was having problems in bug 992679 with using toolbox.getPanel("webconsole") to get a reference to the split console, when it's definitely open. Brian, any ideas there?

78 down, 118 to go
Flags: needinfo?(bgrinstead)
(In reply to Heather Arthur  [:harth] from comment #11)
> Created attachment 8477784 [details] [diff] [review]
> webconsole-e10s-WIP10.patch
> 
> Fixes a few more. Getting an error from a previously-enabled test:
> 
> 737 INFO Full message: TypeError: toolbox.getPanel(...) is undefined
> 3738 INFO Full stack:
> runner@chrome://mochitests/content/browser/browser/devtools/webconsole/test/
> browser_webconsole_split_persist.js:41:21
> 
> Someone else was having problems in bug 992679 with using
> toolbox.getPanel("webconsole") to get a reference to the split console, when
> it's definitely open. Brian, any ideas there?
> 
> 78 down, 118 to go

This is weird, buttonsPromise is throwing an exception (from the _buildButtons call), which is maybe causing the promise.all call to not wait for the split console or something?  If I comment the buttonsPromise line out here it passes: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox.js#282.

There seem to be two relevant errors from buildButtons (I'll file a new bug for it)

106 INFO console.error:
107 INFO   Message: TypeError: window is null
108 INFO   Stack:
109 INFO     exports.items<.state.isChecked@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/commands/paintflashing.js:106:15
110 INFO CommandUtils.createButtons/</</onChange@resource:///modules/devtools/DeveloperToolbar.jsm:156:27
111 INFO CommandUtils.createButtons/</<@resource:///modules/devtools/DeveloperToolbar.jsm:167:1
112 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:40:40
113 INFO then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:20:43
114 INFO then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:58:9
115 INFO CommandUtils.createButtons/<@resource:///modules/devtools/DeveloperToolbar.jsm:99:14
116 INFO exports.promiseEach/</callNext@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/util/util.js:277:19
117 INFO exports.promiseEach/</callNext/onSuccess@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/util/util.js:269:11
118 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:40:40
119 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:40:40
120 INFO then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:20:43
121 INFO then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:58:9
122 INFO then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:58:9
123 INFO exports.promiseEach/</callNext@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/util/util.js:278:7
124 INFO exports.promiseEach/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/util/util.js:281:5
125 INFO Promise@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/util/promise.js:48:5
126 INFO exports.promiseEach@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/util/util.js:258:1
127 INFO CommandUtils.createButtons@resource:///modules/devtools/DeveloperToolbar.jsm:97:1
128 INFO Toolbox.prototype._buildButtons/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox.js:601:14
129 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:40:40
130 INFO then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:20:43
131 INFO then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:58:9
132 INFO Toolbox.prototype._buildButtons@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox.js:599:12
133 INFO Toolbox.prototype.open/</domReady@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox.js:267:30

and

9 INFO console.error:
10 INFO   Message: TypeError: window is null
11 INFO   Stack:
12 INFO     exports.items<.state.isChecked@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/commands/paintflashing.js:106:15
13 INFO CommandUtils.createButtons/</</onChange@resource:///modules/devtools/DeveloperToolbar.jsm:156:27
14 INFO CommandUtils.createButtons/</<@resource:///modules/devtools/DeveloperToolbar.jsm:167:1
15 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:40:40
16 INFO then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:20:43
17 INFO then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:58:9
18 INFO CommandUtils.createButtons/<@resource:///modules/devtools/DeveloperToolbar.jsm:99:14
19 INFO exports.promiseEach/</callNext@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/util/util.js:277:19
20 INFO exports.promiseEach/</callNext/onSuccess@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/util/util.js:269:11
21 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:40:40
22 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:40:40
23 INFO then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:20:43
24 INFO then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:58:9
25 INFO then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:58:9
26 INFO exports.promiseEach/</callNext@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/util/util.js:278:7
27 INFO exports.promiseEach/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/util/util.js:281:5
28 INFO Promise@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/util/promise.js:48:5
29 INFO exports.promiseEach@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/gcli/util/util.js:258:1
30 INFO CommandUtils.createButtons@resource:///modules/devtools/DeveloperToolbar.jsm:97:1
31 INFO Toolbox.prototype._buildButtons/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/toolbox.js:601:14
32 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:40:40
33 INFO then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:20:43
34 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:72:11
35 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:40:11
36 INFO then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:20:43
37 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:72:11
38 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:40:11
39 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:40:40
40 INFO then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:20:43
41 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:72:11
42 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:40:11
43 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:40:40
44 INFO then@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:20:43
45 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:72:11
46 INFO resolve@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:40:11
47 INFO Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:865:23
48 INFO this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:7
Flags: needinfo?(bgrinstead)
Depends on: 1058033
Assignee

Comment 13

5 years ago
Posted patch webconsole-e10s-WIP11.patch (obsolete) — Splinter Review
10 more.

88 down, 108 to go
Attachment #8477025 - Attachment is obsolete: true
Attachment #8477784 - Attachment is obsolete: true
Assignee

Comment 14

5 years ago
Posted patch webconsole-e10s-WIP12.patch (obsolete) — Splinter Review
101 down, 95 to go
Attachment #8478805 - Attachment is obsolete: true
Assignee

Comment 15

5 years ago
Posted patch webconsole-e10s-WIP13.patch (obsolete) — Splinter Review
128 down, 68 to go
Attachment #8479548 - Attachment is obsolete: true
Assignee

Comment 16

5 years ago
Posted patch webconsole-e10s-WIP14.patch (obsolete) — Splinter Review
Rebased.
Attachment #8480294 - Attachment is obsolete: true
Assignee

Comment 17

5 years ago
Posted patch webconsole-e10s-WIP15.patch (obsolete) — Splinter Review
146 down, 50 to go
Attachment #8481745 - Attachment is obsolete: true
Assignee

Comment 18

5 years ago
Posted patch webconsole-e10s-WIP16.patch (obsolete) — Splinter Review
I've done about all I can do. There are about 40 left. Many of them are tough ones. We could knock a bunch out if we figured out how to get around some common patterns though.

I had to comment out a bunch that need expectUncaughtException() in regular mode, but fail with that in e10s mode as the errors thrown from content don't reach the test or something like that.

A bunch of the unconverted tests are trying to send mouse events to a content button, which doesn't work under e10s.

Others I left are just trying to access variables from the content window.

Additionally, console.log() from content may not be showing up in the browser console, according to the test browser_console.js.
Attachment #8483220 - Attachment is obsolete: true
Assignee

Comment 19

5 years ago
Posted patch webconsole-e10s-WIP17.patch (obsolete) — Splinter Review
Rebased. Comments above stand.
Attachment #8483893 - Attachment is obsolete: true
(In reply to Heather Arthur  [:harth] from comment #18)
> Created attachment 8483893 [details] [diff] [review]
> webconsole-e10s-WIP16.patch
> 
> I've done about all I can do. There are about 40 left. Many of them are
> tough ones. We could knock a bunch out if we figured out how to get around
> some common patterns though.
> 
> I had to comment out a bunch that need expectUncaughtException() in regular
> mode, but fail with that in e10s mode as the errors thrown from content
> don't reach the test or something like that.
> 
> A bunch of the unconverted tests are trying to send mouse events to a
> content button, which doesn't work under e10s.
> 
> Others I left are just trying to access variables from the content window.
> 
> Additionally, console.log() from content may not be showing up in the
> browser console, according to the test browser_console.js.

Great!  I'd say it's ready for review if browser.ini is updated to remove the semicolons and instead add skip-if = e10s for the still affected tests.
(In reply to Heather Arthur  [:harth] from comment #18)
> I've done about all I can do. There are about 40 left. Many of them are
> tough ones. We could knock a bunch out if we figured out how to get around
> some common patterns though.
> 
> I had to comment out a bunch that need expectUncaughtException() in regular
> mode, but fail with that in e10s mode as the errors thrown from content
> don't reach the test or something like that.
> 
> A bunch of the unconverted tests are trying to send mouse events to a
> content button, which doesn't work under e10s.

Heather, are these e10s issues with ours test frameworks? Do you need any guidance from e10s developers on fixing the common problem patterns?


> Additionally, console.log() from content may not be showing up in the
> browser console, according to the test browser_console.js.

I think this is bug 1060093.
Depends on: 1060093
Flags: needinfo?(fayearthur)
Assignee

Comment 22

5 years ago
(In reply to Chris Peterson (:cpeterson) from comment #21)
> (In reply to Heather Arthur  [:harth] from comment #18)
> > I had to comment out a bunch that need expectUncaughtException() in regular
> > mode, but fail with that in e10s mode as the errors thrown from content
> > don't reach the test or something like that.
> > 
> > A bunch of the unconverted tests are trying to send mouse events to a
> > content button, which doesn't work under e10s.
> 
> Heather, are these e10s issues with ours test frameworks? Do you need any
> guidance from e10s developers on fixing the common problem patterns?

These issues I think would be common to all tests. expectUncaughtException() is a SimpleTest thing, and the mouse events are sent using EventUtils.

the expectUncaughtException() issue would be fine if we completely switched to running e10s only, as we could just take them all out.

> > Additionally, console.log() from content may not be showing up in the
> > browser console, according to the test browser_console.js.
> 
> I think this is bug 1060093.

What's odd is that the logs show up in the content console, but don't show up in the browser console. At least in the test. So I'm not sure it's the same.
Flags: needinfo?(fayearthur)
Assignee

Comment 23

5 years ago
Update: I tried converting all the commented-out tests to skip-if, and curiously got a crash in e10s mode when running the lot.
Assignee

Comment 24

5 years ago
Posted patch webconsole-e10s-WIP18.patch (obsolete) — Splinter Review
Here's the patch with skip-ifs instead of comments. I just ran them again. Maybe what I was seeing was a tab crash. In one test, a test crashes and the test times out.
Attachment #8483895 - Attachment is obsolete: true
(In reply to Heather Arthur  [:harth] from comment #24)
> Created attachment 8486092 [details] [diff] [review]
> webconsole-e10s-WIP18.patch
> 
> Here's the patch with skip-ifs instead of comments. I just ran them again.
> Maybe what I was seeing was a tab crash. In one test, a test crashes and the
> test times out.

Went ahead and pushed to try with e10s enabled to see what happens there: https://tbpl.mozilla.org/?tree=Try&rev=1f368856f6c9
(In reply to Heather Arthur  [:harth] from comment #24)
> Created attachment 8486092 [details] [diff] [review]
> webconsole-e10s-WIP18.patch
> 
> Here's the patch with skip-ifs instead of comments. I just ran them again.
> Maybe what I was seeing was a tab crash. In one test, a test crashes and the
> test times out.

Looks like there is a crash on browser_webconsole_split_persist.js [0] - https://tbpl.mozilla.org/php/getParsedLog.php?id=47641876&full=1&branch=try.

I'd suggest just skipping that test with a comment (and bucketing it with the other difficult ones) if it gets the suite running again.
I see a few more errors and leaks on the linux debug build: https://tbpl.mozilla.org/php/getParsedLog.php?id=47640037&full=1&branch=try

INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_bug664688_sandbox_update_after_navigation.js | uncaught exception - TypeError: contentWindow is null at chrome://browser/content/browser.js:4509
INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_webconsole_bug_658368_time_methods.js | Test timed out - expected PASS
INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_webconsole_clickable_urls.js | Test timed out - expected PASS

Leaks:

TEST-UNEXPECTED-FAIL | browser/devtools/webconsole/test/browser_webconsole_split_escape_key.js | leaked 2 window(s) until shutdown [url = about:blank]
TEST-UNEXPECTED-FAIL | browser/devtools/webconsole/test/browser_webconsole_split_focus.js | leaked 2 window(s) until shutdown [url = about:blank]
TEST-UNEXPECTED-FAIL | browser/devtools/webconsole/test/browser_webconsole_output_table.js | leaked 1 window(s) until shutdown [url = http://example.com/browser/browser/devtools/webconsole/test/test-console-table.html]
I wouldn't worry about the leaks yet, failed tests usually don't clean up after themselves.
Depends on: 1067576
Assignee

Comment 29

5 years ago
Posted patch webconsole-e10s-WIP19.patch (obsolete) — Splinter Review
Rebased. Now we need to convert the non-e10s tests to loadTab() and openConsole()-as-a-promise. I've started on that.
Assignee

Comment 30

5 years ago
Posted patch webconsole-e10s-WIP20.patch (obsolete) — Splinter Review
Sorry, huge patch. This rewrites all the webconsole tests to use new promise-y helper methods (loadTab and openConsole). It also changes some tests to work on e10s. Some are disabled with e10s as they are intermittently failing or would take to long to fix this round.

Still getting leaks from various tests on debug builds (https://tbpl.mozilla.org/?tree=Try&rev=65f822e21b1c), does it still stand that that doesn't matter?
Attachment #8486092 - Attachment is obsolete: true
Attachment #8498071 - Attachment is obsolete: true
Attachment #8502045 - Flags: review?(past)
Comment on attachment 8502045 [details] [diff] [review]
webconsole-e10s-WIP20.patch

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

What I meant above was that we should only start working on fixing the leaks after we've made sure that no other test failures occur. This seems to be the case in some of the oranges I looked at in the last try run, but if you prefer to just disable the leaky tests for now, I'm also fine with that. It's always good to make concrete steps forward.

Related to that, there are a lot of oranges in unrelated areas, like the style inspector that are obscuring the results of the try run. Didn't we disable all the failing tests/suites or is it that your try push just wasn't rebased on the latest fx-team tip?
Attachment #8502045 - Flags: review?(past)
Assignee

Comment 32

5 years ago
Posted patch webconsole-e10s-WIP21.patch (obsolete) — Splinter Review
As much as I'd like to get this in the bag, I'm going to focus on fixing some e10s functionality in other places.

These pass locally for me, but there are several crashes and memory leaks on try. Whenever I disable the leaks, leaks pop up in other tests and whenever I disable a crash, a crash pops up somewhere else.

This probably means the crashes are due to running out of memory, I think.

Latest run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=934899582a7d
Attachment #8502045 - Attachment is obsolete: true
Moving DevTools test bugs to e10s milestone M7 (i.e. not blocking e10s merge to Aurora).
Whiteboard: [e10s][status:inflight] → [e10s-m7]
Whiteboard: [e10s-m7] → [e10s-m7][status:inflight]
Assignee

Updated

5 years ago
Assignee: nobody → fayearthur
Assignee

Comment 34

5 years ago
Okay, this is it.

head.js now defines loadTab() and removes addTab(), as well as makes openConsole() promise-y. A lot of changes are because of that. A lot of tests were also cleaned up to use an asyncTest() helper and yields.

Passes plain try: https://tbpl.mozilla.org/?tree=Try&rev=e51bf4cc7904
I've disabled the tests that time out here: https://tbpl.mozilla.org/?tree=Try&rev=da4892fceaac

There are still some leaks, but they exist throughout our code and this is bug 1094749.

This leaves about 40 tests disabled. 10 of these are due to the expectUncaughtException() calls I mentioned in comment 18, which I'll file a followup for. about 8 are disabled just for Linux debug timeouts.
Attachment #8504110 - Attachment is obsolete: true
Attachment #8534511 - Flags: review?(past)
Comment on attachment 8534511 [details] [diff] [review]
webconsole-e10s-tests-v22.patch

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

My eyes are bleeding after reviewing all of this, but I'm so excited about the task-based rewrite! W00t!

::: browser/devtools/webconsole/test/browser_warn_user_about_replaced_api.js
@@ +22,5 @@
> +  let hud2 = yield openConsole();
> +
> +  yield testWarningPresent(hud2);
> +
> +  Services.prefs.clearUserPref(PREF)

Nit: missing semicolon.

::: browser/devtools/webconsole/test/browser_webconsole_autocomplete_in_debugger_stackframe.js
@@ +137,5 @@
>    openDebugger().then(() => {
>      gStackframes.selectFrame(1);
>  
>      info("openConsole");
> +    executeSoon(() => openConsole().then(testDriver.next()));

then() should have a lambda that calls next() (not use its return value as a callback):

then(() => testDriver.next())

::: browser/devtools/webconsole/test/browser_webconsole_autocomplete_popup_close_on_tab_switch.js
@@ +27,5 @@
> +  panel.addEventListener("popupshown", function popupOpened() {
> +    panel.removeEventListener("popupshown", popupOpened, false);
> +    loadTab("data:text/html;charset=utf-8,<p>testing autocomplete closes").then(() => {
> +      finished.resolve();
> +    });

Nit: then(() => finished.resolve()) is shorter.

::: browser/devtools/webconsole/test/browser_webconsole_bug_595934_message_categories.js
@@ +1,1 @@
> +

Hmm?

::: browser/devtools/webconsole/test/browser_webconsole_bug_597103_deactivateHUDForContext_unfocused_window.js
@@ +1,1 @@
> +

Look, there it is again!

::: browser/devtools/webconsole/test/head.js
@@ +17,5 @@
> +
> +/*
> +
> +  loadTab(TEST_URI).then(() => {
> +    openConsole().then(() => {

Did you mean to add a comment here, like "common test setup code"?

@@ +399,4 @@
>      }
>    }
>  
> +  setTimeout(wait, 100);

Wouldn't executeSoon() be better in this case? It doesn't seem like we actually need the 100 msec delay (although I don't know if we ever succeed or fail in this first call either; if not, a timeout is better).

::: browser/devtools/webconsole/test/test-console-output-events.html
@@ +19,5 @@
>    document.addEventListener("mousemove", eventLogger);
>    document.addEventListener("keypress", eventLogger);
> +
> +  synthesizeKeyPress("a", {shiftKey: true});
> +  synthesizeMouseMove();

The test expects these events in the reverse order, is this order intentional?
Attachment #8534511 - Flags: review?(past) → review+
Assignee

Comment 36

5 years ago
Posted patch For check in (obsolete) — Splinter Review
Fixed to comments. Thanks for doing the huge review Panos.

Plain try:
https://tbpl.mozilla.org/?tree=Try&rev=67337d785dda

e10s:
https://tbpl.mozilla.org/?tree=Try&rev=56e43e6ed63d
(super orange, but they are known or leaks probably due to bug 1073352)
Attachment #8534511 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Keywords: checkin-needed
Needs rebasing, sorry :(
Keywords: checkin-needed
Assignee

Updated

5 years ago
Keywords: checkin-needed
Status: NEW → ASSIGNED
Keywords: checkin-needed
Assignee

Comment 40

5 years ago
Leaving the rest of the tests to someone else.
Assignee: fayearthur → nobody
Status: ASSIGNED → NEW
https://hg.mozilla.org/mozilla-central/rev/84880f27bab3
Assignee: nobody → fayearthur
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Filed bug 1112599 to track enabling the rest of the web console tests.

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.