Closed Bug 1253233 Opened 8 years ago Closed 8 years ago

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

Categories

(Toolkit :: Find Toolbar, defect)

defect
Not set
normal
Points:
5

Tracking

()

RESOLVED FIXED
mozilla48
Iteration:
47.3 - Mar 7
Tracking Status
e10s + ---
firefox48 --- fixed

People

(Reporter: mikedeboer, Assigned: mikedeboer)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

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+
Summary: [e10s] Re-enable mochitest-chrome tests for find toolbar → [e10s] Add remote browser coverage in mochitest-chrome tests for find toolbar
Up-ing the patches whilst pondering who I should ask to review them...
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)
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
See Also: → 1261584
You need to log in before you can comment on or make changes to this bug.