[e10s] Add remote browser coverage in mochitest-chrome tests for find toolbar

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mikedeboer, Assigned: mikedeboer)

Tracking

(Blocks: 1 bug)

unspecified
mozilla48
Points:
5
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(e10s+, firefox48 fixed)

Details

Attachments

(5 attachments)

test_findbar.xul, test_findbar_events.xul, test_bug263683.xul, test_bug304188.xul and test_bug331215.xul need work to work in e10s mode as well.
Flags: qe-verify-
Flags: firefox-backlog+

Updated

3 years ago
Blocks: 984139
tracking-e10s: ? → +
Summary: [e10s] Re-enable mochitest-chrome tests for find toolbar → [e10s] Add remote browser coverage in mochitest-chrome tests for find toolbar
Created attachment 8729653 [details] [diff] [review]
Patch 1: make preparations to allow testing of remote browser elements

Up-ing the patches whilst pondering who I should ask to review them...
Created attachment 8729655 [details] [diff] [review]
Patch 2: refactor bug263683_window.xul to run the test on remote browsers as well
Created attachment 8729657 [details] [diff] [review]
Patch 3: refactor bug304188_window.xul to run the test on remote browsers as well
Created attachment 8729658 [details] [diff] [review]
Patch 4: refactor bug331215_window.xul to run the test on remote browsers as well
Created attachment 8729659 [details] [diff] [review]
Patch 5: refactor findbar_window.xul to run the test on remote browsers as well
Comment on attachment 8729653 [details] [diff] [review]
Patch 1: make preparations to allow testing of remote browser elements

Felipe, I'm asking you to review all the patches here, if you don't mind!

Each test conversion is done with the same pattern, so that duplication should make things appear less bulky to review... I hope!
Attachment #8729653 - Flags: review?(felipc)
Attachment #8729655 - Flags: review?(felipc)
Attachment #8729657 - Flags: review?(felipc)
Attachment #8729658 - Flags: review?(felipc)
Comment on attachment 8729659 [details] [diff] [review]
Patch 5: refactor findbar_window.xul to run the test on remote browsers as well

This is one badass test. The 'TODO' parts I added are meant to be filed as follow-up bugs. Especially to figure out why `testFindAgainNotFound()` is failing no a remote browser element, because that's likely a bug in the findbar code.
Attachment #8729659 - Flags: review?(felipc)
Comment on attachment 8729655 [details] [diff] [review]
Patch 2: refactor bug263683_window.xul to run the test on remote browsers as well

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

::: toolkit/content/tests/chrome/bug263683_window.xul
@@ +19,5 @@
>    <script type="application/javascript"><![CDATA[
> +    const {interfaces: Ci, classes: Cc, results: Cr, utils: Cu} = Components;
> +    Cu.import("resource://gre/modules/Task.jsm");
> +    Cu.import("resource://testing-common/ContentTask.jsm");
> +    ContentTask.setTestScope(window.opener.wrappedJSObject);

I don't understand what is this for, can you explain?
Comment on attachment 8729655 [details] [diff] [review]
Patch 2: refactor bug263683_window.xul to run the test on remote browsers as well

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

::: toolkit/content/tests/chrome/bug263683_window.xul
@@ +19,5 @@
>    <script type="application/javascript"><![CDATA[
> +    const {interfaces: Ci, classes: Cc, results: Cr, utils: Cu} = Components;
> +    Cu.import("resource://gre/modules/Task.jsm");
> +    Cu.import("resource://testing-common/ContentTask.jsm");
> +    ContentTask.setTestScope(window.opener.wrappedJSObject);

I assume all three lines?

The two imports are for convenience objects I'm using below. (Task and ContentTask, which usually make the code flow in unit tests easier to follow and reason about - as you know).

`ContentTask.setTestScope` is invoked to set the scope where it can find the `ok()` and `info()` methods. This makes it possible to use the whole range of Assert.jsm assertion methods _inside_ a ContentTask. Super convenient.

Does that answer your question?
Attachment #8729653 - Flags: review?(felipc) → review+
Attachment #8729655 - Flags: review?(felipc) → review+
Attachment #8729657 - Flags: review?(felipc) → review+
Attachment #8729658 - Flags: review?(felipc) → review+
Attachment #8729659 - Flags: review?(felipc) → review+
w00t! Thanks, Felipe!
Keywords: checkin-needed
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8193581&repo=fx-team
Flags: needinfo?(mdeboer)
Keywords: leave-open
Well, this is fun; test_bug263683.xul permafails on OSX 10.6 opt ONLY(!?!), which caused the backout. Hrmpf. (The other two patches were part of the same patch, thus were backed out as well - understandably.)
Flags: needinfo?(mdeboer)
(In reply to Mike de Boer [:mikedeboer] from comment #17)
*part of the same push
https://hg.mozilla.org/integration/fx-team/rev/fe2112d79fc1178e0b34f7674e61309f7de082ab
Bug 1253233: refactor bug263683_window.xul to run the test on remote browsers as well. r=felipe

https://hg.mozilla.org/integration/fx-team/rev/912ee4b6a03385ac8e5bd80f06cbb57cc9b594c1
Bug 1253233: refactor bug304188_window.xul to run the test on remote browsers as well. r=felipe

https://hg.mozilla.org/integration/fx-team/rev/8f944086e8a3cd97549e815e7fbb463d76717e83
Bug 1253233: refactor bug331215_window.xul to run the test on remote browsers as well. r=felipe
Keywords: leave-open

Comment 21

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fe2112d79fc1
https://hg.mozilla.org/mozilla-central/rev/912ee4b6a033
https://hg.mozilla.org/mozilla-central/rev/8f944086e8a3
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.