Closed Bug 1073247 Opened 6 years ago Closed 4 years ago

e10s'ify UITour tests

Categories

(Firefox :: Tours, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
e10s + ---
firefox46 --- fixed
firefox47 --- fixed

People

(Reporter: adw, Assigned: MattN)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

They live here: http://mxr.mozilla.org/mozilla-central/source/browser/modules/test/

This doesn't seem too hard...  As far as I can tell, when the tests touch content, most of the time they're doing gContentAPI.foo(), which would be pretty straightforward to convert into sending a message to a test content script that then makes the API call.  The hard part might rather be converting all those calls sites to be async.  It might help to convert these tests to add_task.
Flags: qe-verify-
Flags: firefox-backlog+
Blocks: e10s-tests
tracking-e10s: --- → +
Attached patch uitour-e10s-fixing-tests.patch (obsolete) — Splinter Review
After bug 1073238 is solved I have some thoughts to share. You may find it useful, dear bug solver:

0. Use Task! See http://hg.mozilla.org/mozilla-central/file/d380166816dd/browser/modules/test/browser_UITour3.js.
1. Make a separate directory for UITour.
2. Make sure that security tests are working properly (`test_untrusted_code`). My understanding is that they are now pretty much broken since we're checking synchronously that the UITour is not shown. I'd suggest calling sendPageCallback("UITour:untrusted") in [here](http://hg.mozilla.org/mozilla-central/file/d380166816dd/browser/base/content/content-UITour.js#l12). It's not perfect because it adds a test-specific code. (Maybe it's somehow possible to mock it. I tried but I couldn't make it work.)
3. It would be so much easier to test if all the methods that are asynchronous (which now means: all) had callbacks. We could then automatically convert them to promises. To workaround this problem I created some helpers like [`showInfoPromise`](http://hg.mozilla.org/mozilla-central/file/d380166816dd/browser/modules/test/head.js#l131). It does two things: it calls showInfo and returns a promise that will be resolved when info is actually shown. It makes beautiful testing:
```
  yield showInfoPromise("urlbar", "a title", "some text", "image.png");
```
  After this you know that the info is shown so run your asserts now. Make similar helpers for other methods or generalize this concept somehow (although difficult without callbacks).
4. I already worked on fixing tests and there's a patch in the attachment. It has some inter-process communication in the tests. Feel free to use it.
Moving open UITour bugs to Firefox::Tours. Filter on firefox-tours-20150121.
Component: General → Tours
Blocks: 1213241
Duplicate of this bug: 1240747
I'm taking a stab at fixing head.js and a few tests to understand the difficulty.
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Points: 5 → ---
Comment on attachment 8520306 [details] [diff] [review]
uitour-e10s-fixing-tests.patch

With my WIP patch, the whole directory still passes on OS X without e10s and browser_showMenu_controlCenter.js now passes with e10s.
Attachment #8520306 - Attachment is obsolete: true
Comment on attachment 8712519 [details]
MozReview Request: Bug 1073247 - UITour: Proxy gContentAPI method calls to the content process via ContentTask and fix tests for e10s. r=felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32573/diff/1-2/
Attachment #8712519 - Attachment description: MozReview Request: Bug 1073247 - WIP: UITour: Proxy gContentAPI method calls to the content process via ContentTask → MozReview Request: Bug 1073247 - UITour: Proxy gContentAPI method calls to the content process via ContentTask. r=felipe
Attachment #8712519 - Flags: review?(felipc)
Comment on attachment 8712519 [details]
MozReview Request: Bug 1073247 - UITour: Proxy gContentAPI method calls to the content process via ContentTask and fix tests for e10s. r=felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32573/diff/2-3/
Comment on attachment 8712519 [details]
MozReview Request: Bug 1073247 - UITour: Proxy gContentAPI method calls to the content process via ContentTask and fix tests for e10s. r=felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32573/diff/3-4/
Attachment #8712519 - Attachment description: MozReview Request: Bug 1073247 - UITour: Proxy gContentAPI method calls to the content process via ContentTask. r=felipe → MozReview Request: Bug 1073247 - UITour: Proxy gContentAPI method calls to the content process via ContentTask and fix tests for e10s. r=felipe
Comment on attachment 8712519 [details]
MozReview Request: Bug 1073247 - UITour: Proxy gContentAPI method calls to the content process via ContentTask and fix tests for e10s. r=felipe

https://reviewboard.mozilla.org/r/32573/#review29803

::: browser/components/uitour/test/browser_trackingProtection_tour.js:68
(Diff revision 4)
> -  gContentAPI.hideMenu("controlCenter");
> +  yield gContentAPI.hideMenu("controlCenter");

is this really a promise? I don't see how hideMenu returns a promise, and there's another instance where this is used without the `yield`
Attachment #8712519 - Flags: review?(felipc) → review+
https://reviewboard.mozilla.org/r/32573/#review29803

> is this really a promise? I don't see how hideMenu returns a promise, and there's another instance where this is used without the `yield`

Yes, in E10s, all property accesses of gContentAPI return a function which returns a promise using ContentTask as a sort of shim to avoid a rewrite. I fixed that one other place which was missing the yield though it wouldn't have mattered much in that case since waitForCondition ended up being used.
https://hg.mozilla.org/mozilla-central/rev/4754d1254086
https://hg.mozilla.org/mozilla-central/rev/24be2dff0c48
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Depends on: 1245363
Depends on: 1245267
You need to log in before you can comment on or make changes to this bug.