Closed
Bug 1072467
Opened 10 years ago
Closed 10 years ago
Add tests for e10s add-on shims
Categories
(Firefox :: Extension Compatibility, defect)
Tracking
()
RESOLVED
FIXED
Firefox 35
People
(Reporter: billm, Assigned: billm)
Details
Attachments
(1 file, 2 obsolete files)
15.33 KB,
patch
|
ted
:
review+
mconley
:
review+
|
Details | Diff | Splinter Review |
This includes tests for a few issues from the past as well as some stuff I'm going to file soon. Ted, I'm asking for your review for the build changes. We need to package up an add-on and there doesn't seem to be any way to do that with moz.build. Do I have to touch CLOBBER because of these changes?
Attachment #8494687 -
Flags: review?(ted)
Attachment #8494687 -
Flags: review?(mconley)
Comment 1•10 years ago
|
||
Comment on attachment 8494687 [details] [diff] [review] shim-tests Review of attachment 8494687 [details] [diff] [review]: ----------------------------------------------------------------- This looks really good - and I'm so glad to have tests for these things! ::: toolkit/components/addoncompat/tests/addon/bootstrap.js @@ +2,5 @@ > +var Ci = Components.interfaces; > +var Cu = Components.utils; > + > +Cu.import("resource://gre/modules/Services.jsm"); > +Cu.import("resource://gre/modules/Promise.jsm"); There seems to be a growing trend to move away from Promise.jsm, and to use DOM Promises instead. Can we also do that here? @@ +22,5 @@ > +let gWin; > +let gBrowser; > +let ok, is, info; > + > +function testContentWindow() A brief description of what each of these tests actually exercises would be good. @@ +28,5 @@ > + let deferred = Promise.defer(); > + > + const url = baseURL + "browser_addonShims_testpage.html"; > + let tab = gBrowser.addTab(url); > + gBrowser.selectedTab = tab; You use this pattern several times: let tab = gBrowser.addTab(url); ... tab.linkedBrowser.addEventListener("load", ... tab.linkedBrowser.removeEventListener("load"... Seems ripe for a helper function that returns a Promise that resolves once a particular tab finishes loading to some URL. Not everywhere, mind you - just the places where you remove the event listener immediately. @@ +66,5 @@ > + > + function dummyHandler() {} > + > + // Test that a removed listener stays removed. > + for (let i = 0; i < 5; i++) { It's not clear to me what this loop is trying to test. If this didn't work, would we expect it to throw? Perhaps we can ensure that dummyHandler never fires by having it set a value, and ensure that value has never been set by the time we load either url1 or url2 (or both). @@ +233,5 @@ > + > +function startup(aData, aReason) > +{ > + forEachWindow(win => { > + win.shimAddon = (funcs) => runTests(win, funcs); I think "shimAddon" doesn't describe everything that this function does. Perhaps "runAddonShimTests" is clearer. ::: toolkit/components/addoncompat/tests/browser/browser.ini @@ +5,5 @@ > + browser_addonShims_testpage2.html > +generated-files = > + addon.xpi > + > +[browser_addonShims.js] This is going to execute with both the shims enabled and disabled. Do we want to only ever run this with e10s? ::: toolkit/components/addoncompat/tests/browser/browser_addonShims.js @@ +1,2 @@ > +let {AddonManager} = Cu.import("resource://gre/modules/AddonManager.jsm", {}); > +let {Promise} = Cu.import("resource://gre/modules/Promise.jsm", {}); As before, let's try to use DOM Promises instead. @@ +2,5 @@ > +let {Promise} = Cu.import("resource://gre/modules/Promise.jsm", {}); > + > +const ADDON_URL = "http://example.com/browser/toolkit/components/addoncompat/tests/browser/addon.xpi"; > + > +function addAddon(url) Let's get some docstrings in here while we're at it. @@ +47,5 @@ > + > + return deferred.promise; > +} > + > +function test() Better yet: add_task(function* test_addon_shims() { let addon = yield addAddon(ADDON_URL); yield window.shimAddon({...}); yield removeAddon(addon); }); Also, we should direct the reader to where the "real" tests are, within the add-on itself.
Attachment #8494687 -
Flags: review?(mconley)
Assignee | ||
Comment 2•10 years ago
|
||
Thanks for the quick review. I want to run the tests even without e10s to make sure that the shims have the same semantics as non-e10s code.
Attachment #8494687 -
Attachment is obsolete: true
Attachment #8494687 -
Flags: review?(ted)
Attachment #8494795 -
Flags: review?(ted)
Attachment #8494795 -
Flags: review?(mconley)
Comment 3•10 years ago
|
||
Comment on attachment 8494795 [details] [diff] [review] shim-tests v2 Review of attachment 8494795 [details] [diff] [review]: ----------------------------------------------------------------- A few small things, overall this looks really good though. I'll be really happy to get some tests for this stuff. :) ::: toolkit/components/addoncompat/tests/addon/bootstrap.js @@ +46,5 @@ > + is(gWin.content, browser.contentWindow, "content === contentWindow"); > + > + ok(browser.contentDocument.getElementById("link"), "link present in document"); > + > + // FIXME: Waiting on bug 1065811. Bug 1065811 is marked fixed now, so let's uncomment this. @@ +80,5 @@ > + > + // We also want to make sure that this listener doesn't fire > + // after it's removed. > + let loadWithRemoveCount = 0; > + addLoadListener(browser, function handler1() { Where is this handler removed? You used to remove it in the function itself, but that's missing now. @@ +230,5 @@ > + > + gWin = win; > + gBrowser = win.gBrowser; > + > + return testContentWindow(). Out of curiosity, what is the behaviour of this overall test when one of these subtests fails? Do we just wait until timeout?
Attachment #8494795 -
Flags: review?(mconley)
Assignee | ||
Comment 4•10 years ago
|
||
> Where is this handler removed? You used to remove it in the function itself, but that's
> missing now.
It's added with addLoadListener, which removes it automatically.
Assignee | ||
Comment 5•10 years ago
|
||
> Out of curiosity, what is the behaviour of this overall test when one of these subtests
> fails? Do we just wait until timeout?
Yeah, if one of the individual tests times out, then they all time out. I guess I could add a timer myself, but that seems pretty error-prone.
Comment 6•10 years ago
|
||
(In reply to Bill McCloskey (:billm) from comment #4) > > Where is this handler removed? You used to remove it in the function itself, but that's > > missing now. > > It's added with addLoadListener, which removes it automatically. Of course, my mistake - I read that as addEventListener by accident. I understand now.
Assignee | ||
Comment 7•10 years ago
|
||
I re-ran the test with the patches for bug 1065811 applied. Unfortunately, that just exposed bug 1073631. So I just updated the comment :-).
Attachment #8494795 -
Attachment is obsolete: true
Attachment #8494795 -
Flags: review?(ted)
Attachment #8496148 -
Flags: review?(mconley)
Assignee | ||
Comment 8•10 years ago
|
||
Comment on attachment 8496148 [details] [diff] [review] shim-tests v3 Still need Ted's review too.
Attachment #8496148 -
Flags: review?(ted)
Comment 9•10 years ago
|
||
Comment on attachment 8496148 [details] [diff] [review] shim-tests v3 Review of attachment 8496148 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Thanks for the tests, billm! ::: toolkit/components/addoncompat/tests/addon/bootstrap.js @@ +46,5 @@ > + is(gWin.content, browser.contentWindow, "content === contentWindow"); > + > + ok(browser.contentDocument.getElementById("link"), "link present in document"); > + > + // FIXME: Waiting on bug 1073631. Maybe we should file a bug to uncomment out this block. Either a bug, or some other mechanism that'll ensure we don't forget to do that once bug 1073631 gets fixed.
Attachment #8496148 -
Flags: review?(mconley) → review+
Comment 10•10 years ago
|
||
Comment on attachment 8496148 [details] [diff] [review] shim-tests v3 Review of attachment 8496148 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/addoncompat/tests/Makefile.in @@ +3,5 @@ > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +include $(topsrcdir)/config/rules.mk > + > +# This is so hacky. Waiting on bug 988938. This is me being sad. :-(
Attachment #8496148 -
Flags: review?(ted) → review+
Assignee | ||
Comment 11•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/839f8ca256d4
Comment 12•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/839f8ca256d4
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
You need to log in
before you can comment on or make changes to this bug.
Description
•