Closed
Bug 1255066
Opened 8 years ago
Closed 8 years ago
[e10s] convert test_bug_461710_perwindowpb.html to mochitest-browser
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: mak, Assigned: nhnt11)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 7 obsolete files)
2.44 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
17.38 KB,
patch
|
nhnt11
:
review+
|
Details | Diff | Splinter Review |
Since this test depends on private browsing and multiple windows it's easier to convert it to a mochitest-browser test.
Reporter | ||
Comment 1•8 years ago
|
||
will likely change owner, but for now I'm tracking it
Assignee: nobody → mak77
Updated•8 years ago
|
tracking-e10s:
--- → +
Assignee | ||
Comment 2•8 years ago
|
||
Panos asked me to pick this up. The patch converts the test to a mochitest-browser one, and the test passes for me locally with |mach mochitest --e10s|. I'll push to try and post a link here when the results come in. Marco, I'm not sure who the right reviewer is for this, could you please set the r? flag?
Assignee: mak77 → nhnt11
Status: NEW → ASSIGNED
Flags: needinfo?(mak77)
Reporter | ||
Updated•8 years ago
|
Attachment #8729326 -
Flags: review?(mak77)
Reporter | ||
Comment 4•8 years ago
|
||
Comment on attachment 8729326 [details] [diff] [review] Patch Review of attachment 8729326 [details] [diff] [review]: ----------------------------------------------------------------- you may need to unbitrot changes to moz.build, chrome.ini and browser.ini (I just landed another test conversion) ::: toolkit/components/places/tests/browser/browser.ini @@ +21,5 @@ > [browser_visited_notfound.js] > [browser_visituri.js] > [browser_visituri_nohistory.js] > [browser_visituri_privatebrowsing_perwindowpb.js] > +[browser_bug461710.js] \ No newline at end of file please retain alphabetical order in tests manifests. ::: toolkit/components/places/tests/browser/browser_bug461710.js @@ +1,1 @@ > +waitForExplicitFinish(); please move this into test(), so it will be more visible when we convert the test to add_task. @@ +1,3 @@ > +waitForExplicitFinish(); > + > +Cu.import("resource://gre/modules/NetUtil.jsm"); No more needed, this is now already imported by head.js @@ +17,5 @@ > + * > + * onWaitComplete: a callback which will be notified when fn() returns > + * true. > + */ > +function waitForTrue(fn, onWaitComplete) { please, instead of this, use BrowserTestUtils.waitForCondition, so we remove this duped function. @@ +154,5 @@ > +} > + > +function testOnWindow(aIsPrivate, callback) { > + var win = OpenBrowserWindow({private: aIsPrivate}); > + whenDelayedStartupFinished(win, function() { callback(win); }); And this should be replaced with BrowserTestUtils.openNewBrowserWindow that will also wait for delayed-startup. @@ +189,5 @@ > + testOnWindow(false, function(aWin) { > + var selectedBrowser = aWin.gBrowser.selectedBrowser; > + > + normalWindow = aWin; > + normalWindowIframe = selectedBrowser.contentDocument.getElementById("iframe"); This is not OK in e10s, contentDocument is a cpow, we should not use it... You need ContentTask.spawn to use content, that will require some refactoring @@ +195,5 @@ > + testOnWindow(true, function(aPrivateWin) { > + selectedBrowser = aPrivateWin.gBrowser.selectedBrowser; > + > + privateWindow = aPrivateWin; > + privateWindowIframe = selectedBrowser.contentDocument.getElementById("iframe"); ditto
Attachment #8729326 -
Flags: review?(mak77) → review-
Assignee | ||
Comment 5•8 years ago
|
||
Thanks for the review! (In reply to Marco Bonardo [::mak] from comment #4) > Comment on attachment 8729326 [details] [diff] [review] > Patch > > Review of attachment 8729326 [details] [diff] [review]: > ----------------------------------------------------------------- > @@ +189,5 @@ > > + testOnWindow(false, function(aWin) { > > + var selectedBrowser = aWin.gBrowser.selectedBrowser; > > + > > + normalWindow = aWin; > > + normalWindowIframe = selectedBrowser.contentDocument.getElementById("iframe"); > > This is not OK in e10s, contentDocument is a cpow, we should not use it... D'oh! I've converted most of the test locally to use ContentTask and BrowserTestUtils, but I can't figure out a way to call getVisitedDependentComputedStyle from a ContentTask. I tried this: > yield ContentTask.spawn(gTestWindow.gBrowser.selectedBrowser, null, function* () { > var doc = content.document; > var win = doc.defaultView; > function getColor(doc, win, id) { > var elem = doc.getElementById(id); > var utils = win.QueryInterface(Components.interfaces.nsIInterfaceRequestor) > .getInterface(Components.interfaces.nsIDOMWindowUtils); > return utils.getVisitedDependentComputedStyle(elem, "", "color"); > } > is(getColor(doc, win, "link"), kBlue, "Visited link coloring should not work inside of private mode"); > }); This throws at the getVisitedDependentComputedStyle call, with "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED)". I can't figure out another way to do this. Do you have any ideas? Am I missing something? Thanks.
Flags: needinfo?(mak77)
Assignee | ||
Comment 6•8 years ago
|
||
Figured it out, turns out I was just being silly. This patch async-ifies stuff with BrowserTestUtils and ContentTasks.
Attachment #8729326 -
Attachment is obsolete: true
Flags: needinfo?(mak77)
Attachment #8730646 -
Flags: review?(mak77)
Reporter | ||
Comment 7•8 years ago
|
||
Comment on attachment 8730646 [details] [diff] [review] Patch v2 Review of attachment 8730646 [details] [diff] [review]: ----------------------------------------------------------------- if this lands after bug 1254592, you may need to still do some changes, I think at that point chrome.ini will be empty and could be removed (and removed from moz.build) I'm pretty sure this test can be refactored in half the size, using a simple for/of loop, and avoiding the testNum "dance". If you wish to look into that, it would be great, but I won't block on it, so do what you feel like. I was thinking to something like: let tests = [ {win: normalWindow, topic: "uri-visit-saved" }, {win: normalWindow, topic: URI_VISITED_RESOLUTION_TOPIC, color: kRed, message: "Visited link coloring should work outside of private mode"}, ... ] for (let test of tests) { yield ... if (test.color) { yield ... } } ::: toolkit/components/places/tests/browser/browser_bug461710.js @@ +41,5 @@ > + > + if (testNum == 1) > + observer.expectURL(prefix + subtests[0], "uri-visit-saved"); > + else > + observer.expectURL(prefix + subtests[0]); While here, please brace this if/else @@ +43,5 @@ > + observer.expectURL(prefix + subtests[0], "uri-visit-saved"); > + else > + observer.expectURL(prefix + subtests[0]); > + > + let promise = BrowserTestUtils.waitForCondition(() => observer.resolved); This can be improved a lot using promises. This is a poor implementation of a promise indeed. The simpler way would be to have expectURL set observer.deferred = PromiseUtils.defer(); and here just yield observer.deferred.promise; A more sophisticated way could be to completely remove observer and here do something like: let promise = new Promise(resolve => { let uri = NetUtil.newURI(url); let topic = testNum == 1 ? "uri-visit-saved" : URI_VISITED_RESOLUTION_TOPIC; Services.obs.addObserver(function observe(subject) { if (uri.equals(subject.QueryInterface(Ci.nsIURI))) { Services.obs.removeObserver(observe, topic); resolve(); } }, topic, false); }); That is far more compact and readable. In general, don't be shy of refactoring testing code when it's poorly written. @@ +54,5 @@ > + > + yield handleLoad(); > +}); > + > +var doColorTest = Task.async(function* (aShouldWork) { aShouldWork is not a great name, I really hope the test works :) Let's just pass (color, message) as arguments and move their definitions to the caller, that will make the tests more explicit. @@ +136,5 @@ > +var normalWindow; > +var privateWindow; > + > +add_task(function* () { > + waitForExplicitFinish(); you should never mixup add_task and waitForExplicitFinish/finish, it is a good recipe for intermittent failures. add_task by itself already provides a way to asynchronously wait for something, and we should use that.
Attachment #8730646 -
Flags: review?(mak77)
Assignee | ||
Comment 8•8 years ago
|
||
This patch does not refactor the test, but only adds the braces you asked for and gets rid of waitForExplicitFinish.
Attachment #8730646 -
Attachment is obsolete: true
Assignee | ||
Comment 9•8 years ago
|
||
This patch refactors the test as you suggested, but there's a problem. After setting the src of the iframe, it appears that the color of the link changes to match the rules for the :visited state *after* the iframe's contentDocument's load event. I can't find any way to detect or wait for this change. The other patch uses waitForCondition, which checks the observer's resolved condition every 100ms. This interval seems enough for the color to get updated. The existing code (without either of these patches applied) does the same thing but with an interval of 20ms, and that works too. I arrived at this conclusion after spending way too much time trying to get to the bottom of why the test was failing (but inconsistently), and the only solution I could think of was manually waiting for a fixed interval of time before doing the color test. Please let me know if you have a better solution.
Assignee | ||
Comment 10•8 years ago
|
||
Marco, I haven't been able to get further than what I said above. If you know a way to detect that color change, that's great. Otherwise, I would prefer leaving the test un-refactored for now, and filing a bug to refactor it and get to the bottom of the issue in that one. FWIW, I realize that this test can easily start failing intermittently with either of the patches (or in its current state in the tree for that matter) for example if the waiting interval changes or something.
Flags: needinfo?(mak77)
Reporter | ||
Comment 11•8 years ago
|
||
So far, waiting for visited-status-resolution was enough, cause we were notifying it just after updating links, in the same process. I fear on e10s that doesn't work, cause we notify that topic after sending an async message that WILL update the links. We notify it here http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/History.cpp#589 the call updating links is history->NotifyVisited(mURI); some rows above. I'm not sure there's a clean way to wait for all of the messages to be received by the content process, so we'll still have to do with waitForCondition... you could replace the: is(color, test.color, test.message); with // In e10s waiting visited-status-resolution is not enough to ensure links // have been updated, cause it only tells us messages to update links have // been dispatched. We must still wait for the actual links update. info(test.message); waitForCondition(... and here poll for the color. In case of failure the test will start timing out, but that's ok. The test looks MUCH better.
Flags: needinfo?(mak77)
Assignee | ||
Comment 12•8 years ago
|
||
The color is obtained from within a ContentTask, so polling for it needs to be async. This patch modifies BrowserTestUtils.waitForCondition to allow that in a way that does not break any existing consumers of waitForCondition.
Attachment #8731945 -
Attachment is obsolete: true
Attachment #8731949 -
Attachment is obsolete: true
Attachment #8732405 -
Flags: review?(mak77)
Assignee | ||
Comment 13•8 years ago
|
||
Brilliant, thanks for the suggestion. This polls for the color.
Attachment #8732406 -
Flags: review?(mak77)
Reporter | ||
Comment 14•8 years ago
|
||
Comment on attachment 8732405 [details] [diff] [review] Part 1 - Allow async generator functions in BrowserTestUtils.waitForCondition Review of attachment 8732405 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm @@ +994,5 @@ > } > > let conditionPassed = false; > try { > + conditionPassed = yield Task.spawn(condition); why do you need a Task.spawn here? wouldn't just yielding on the function or generator work the same? It would be different if you'd not be in Task.async, and then you'd need to Task.spawn(condition).then(success, fail);
Reporter | ||
Comment 15•8 years ago
|
||
Comment on attachment 8732406 [details] [diff] [review] Part 2 - refactor test Review of attachment 8732406 [details] [diff] [review]: ----------------------------------------------------------------- Looks great ::: toolkit/components/places/tests/browser/browser_bug461710.js @@ +1,3 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ nit: feel free to remove the license header if you're fine sharing this with a PD license. Since it's a rewrite, it may be a good time to relicense. @@ +62,5 @@ > + yield promise; > + > + if (test.color) { > + // In e10s waiting for visited-status-resolution is not enough to ensure links > + // have been updated, because it only tells us that messages to update links trailing space
Attachment #8732406 -
Flags: review?(mak77) → review+
Assignee | ||
Comment 17•8 years ago
|
||
Yielding on the function directly works fine.
Attachment #8732405 -
Attachment is obsolete: true
Attachment #8732405 -
Flags: review?(mak77)
Flags: needinfo?(nhnt11)
Attachment #8734113 -
Flags: review?(mak77)
Assignee | ||
Comment 18•8 years ago
|
||
Forgot to qref, bah. Sorry.
Attachment #8734113 -
Attachment is obsolete: true
Attachment #8734113 -
Flags: review?(mak77)
Attachment #8734143 -
Flags: review?(mak77)
Assignee | ||
Comment 19•8 years ago
|
||
Removed license and trailing space.
Attachment #8732406 -
Attachment is obsolete: true
Attachment #8734152 -
Flags: review+
Reporter | ||
Comment 20•8 years ago
|
||
Comment on attachment 8734143 [details] [diff] [review] Part 1 v2 - Allow async generator functions in BrowserTestUtils.waitForCondition Review of attachment 8734143 [details] [diff] [review]: ----------------------------------------------------------------- Thank you!
Attachment #8734143 -
Flags: review?(mak77) → review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 21•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/31249c604813 https://hg.mozilla.org/integration/fx-team/rev/0d8ca812e2fa
Keywords: checkin-needed
Comment 23•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/31249c604813 https://hg.mozilla.org/mozilla-central/rev/0d8ca812e2fa https://hg.mozilla.org/mozilla-central/rev/fcfc45f49ab4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•