Closed
Bug 1139254
Opened 9 years ago
Closed 9 years ago
Need a way to register mock easily in test code
Categories
(Testing :: XPCShell Harness, defect)
Testing
XPCShell Harness
Tracking
(firefox40 fixed)
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: hiro, Assigned: hiro)
Details
Attachments
(7 files, 4 obsolete files)
6.80 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
2.31 KB,
patch
|
jduell.mcbugs
:
review+
|
Details | Diff | Splinter Review |
2.38 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
4.29 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
6.84 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
17.92 KB,
patch
|
hiro
:
review+
|
Details | Diff | Splinter Review |
75.62 KB,
patch
|
hiro
:
review+
|
Details | Diff | Splinter Review |
Many tests implement their own factory to register mock. But those factories basically just create the instance of the mock. So the factory for the mock can be confined in a utility method to register the mock.
Assignee | ||
Comment 1•9 years ago
|
||
This module has the utility method to register mock and unregiser the mock. The utility method additionally has _genuine property to access the original interface of the mock. It will be useful to create a test double. For example, in a certain condition the mock method is invoked, the original method is invoked in other cases.
Attachment #8572401 -
Flags: review?(gps)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8572402 -
Flags: review?(gps)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8572403 -
Flags: review?(gps)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8572404 -
Flags: review?(gps)
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8572405 -
Flags: review?(gps)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8572406 -
Flags: review?(gps)
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8572407 -
Flags: review?(gps)
Assignee | ||
Updated•9 years ago
|
Attachment #8572405 -
Attachment description: use MockRegistrar in netwerk. → Part 5: use MockRegistrar in netwerk.
Assignee | ||
Comment 8•9 years ago
|
||
Try with all the above patch sets: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc07ef6f4d9c
Comment 9•9 years ago
|
||
Comment on attachment 8572401 [details] [diff] [review] Introduce a new js module to register mock easily Review of attachment 8572401 [details] [diff] [review]: ----------------------------------------------------------------- This is an awesome idea! The JSM is pretty solid. Only the tests need some minor cleanup. You may also want to consider throwing some docs into toolkit/modules/docs. ::: testing/modules/MockRegistrar.jsm @@ +1,2 @@ > +"use strict"; > + Needs MPL @@ +29,5 @@ > + * @param args An array which is passed in the constructor of mock. > + * > + * @return The CID of the mock. > + */ > + register(contractID, mock, args) { I haven't touched JS in a while. Is this new ES6 syntax? @@ +56,5 @@ > + try { > + let genuine = originalFactory.createInstance(outer, iid); > + wrappedMock._genuine = genuine; > + } catch(ex) { > + dump(ex); Log.jsm has some nice APIs for formatting errors. You should consider using them so this output is nicer. ::: testing/modules/tests/xpcshell/test_mockRegistrar.js @@ +20,5 @@ > +function run_test () { > + run_next_test(); > +} > + > +add_task(function test_register() { Nit: This isn't using generators, so add_test() is sufficient. @@ +30,5 @@ > + QueryInterface: XPCOMUtils.generateQI([Ci.nsIUserInfo]), > + }; > + > + let userInfoCID = MockRegistrar.register("@mozilla.org/userinfo;1", localUserInfo); > + do_check_eq(Cc["@mozilla.org/userinfo;1"].createInstance(Ci.nsIUserInfo).username, "localusername"); Assert.equal() should be used for new tests. @@ +32,5 @@ > + > + let userInfoCID = MockRegistrar.register("@mozilla.org/userinfo;1", localUserInfo); > + do_check_eq(Cc["@mozilla.org/userinfo;1"].createInstance(Ci.nsIUserInfo).username, "localusername"); > + do_register_cleanup(() => { > + MockRegistrar.unregister(userInfoCID); I don't see what value do_register_cleanup has here. These functions run after all tests have finished. xpcshell tests are process isolated. So this is just busywork.
Attachment #8572401 -
Flags: review?(gps) → feedback+
Updated•9 years ago
|
Attachment #8572404 -
Flags: review?(gps) → review+
Comment 10•9 years ago
|
||
Comment on attachment 8572405 [details] [diff] [review] Part 5: use MockRegistrar in netwerk. Review of attachment 8572405 [details] [diff] [review]: ----------------------------------------------------------------- Redirecting to a Necko peer.
Attachment #8572405 -
Flags: review?(gps) → review?(jduell.mcbugs)
Comment 11•9 years ago
|
||
Comment on attachment 8572402 [details] [diff] [review] Part 2: use MockRegistrar in toolkit/components. Review of attachment 8572402 [details] [diff] [review]: ----------------------------------------------------------------- Redirecting to someone more involved in JS these days.
Attachment #8572402 -
Flags: review?(gps) → review?(bmcbride)
Comment 12•9 years ago
|
||
Comment on attachment 8572403 [details] [diff] [review] Part 3: use MockRegistrar in toolkit/mozapps. Review of attachment 8572403 [details] [diff] [review]: ----------------------------------------------------------------- Redirecting review.
Attachment #8572403 -
Flags: review?(gps) → review?(bmcbride)
Updated•9 years ago
|
Attachment #8572406 -
Flags: review?(gps) → review+
Updated•9 years ago
|
Attachment #8572407 -
Flags: review?(gps) → review+
Updated•9 years ago
|
Attachment #8572405 -
Flags: review?(jduell.mcbugs) → review+
Updated•9 years ago
|
Attachment #8572402 -
Flags: review?(bmcbride) → review+
Comment 13•9 years ago
|
||
Comment on attachment 8572403 [details] [diff] [review] Part 3: use MockRegistrar in toolkit/mozapps. Review of attachment 8572403 [details] [diff] [review]: ----------------------------------------------------------------- This is much nicer :) (Sorry for the delay)
Attachment #8572403 -
Flags: review?(bmcbride) → review+
Assignee | ||
Comment 14•9 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #9) > ::: testing/modules/MockRegistrar.jsm > @@ +1,2 @@ > > +"use strict"; > > + > > Needs MPL Done. > @@ +29,5 @@ > > + * @param args An array which is passed in the constructor of mock. > > + * > > + * @return The CID of the mock. > > + */ > > + register(contractID, mock, args) { > > I haven't touched JS in a while. Is this new ES6 syntax? Yes. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Object_initializer#Method_definitions > @@ +56,5 @@ > > + try { > > + let genuine = originalFactory.createInstance(outer, iid); > > + wrappedMock._genuine = genuine; > > + } catch(ex) { > > + dump(ex); > > Log.jsm has some nice APIs for formatting errors. You should consider using > them so this output is nicer. Used Log.jsm. > ::: testing/modules/tests/xpcshell/test_mockRegistrar.js > @@ +20,5 @@ > > +function run_test () { > > + run_next_test(); > > +} > > + > > +add_task(function test_register() { > > Nit: This isn't using generators, so add_test() is sufficient. Fixed. > @@ +30,5 @@ > > + QueryInterface: XPCOMUtils.generateQI([Ci.nsIUserInfo]), > > + }; > > + > > + let userInfoCID = MockRegistrar.register("@mozilla.org/userinfo;1", localUserInfo); > > + do_check_eq(Cc["@mozilla.org/userinfo;1"].createInstance(Ci.nsIUserInfo).username, "localusername"); > > Assert.equal() should be used for new tests. Replaced all do_check_eq() with Assert.equal(). > @@ +32,5 @@ > > + > > + let userInfoCID = MockRegistrar.register("@mozilla.org/userinfo;1", localUserInfo); > > + do_check_eq(Cc["@mozilla.org/userinfo;1"].createInstance(Ci.nsIUserInfo).username, "localusername"); > > + do_register_cleanup(() => { > > + MockRegistrar.unregister(userInfoCID); > > I don't see what value do_register_cleanup has here. These functions run > after all tests have finished. xpcshell tests are process isolated. So this > is just busywork. Removed. Thank you!
Attachment #8572401 -
Attachment is obsolete: true
Attachment #8582824 -
Flags: review?(gps)
Assignee | ||
Comment 15•9 years ago
|
||
Unfortunately the fix for bug 1139765 conflicted with the previous patch. This new patch makes xhr.channel nsIChannel, so QueryInterface against the channel works fine. Other parts of the new patch is the same as the previous one. :Unfocused, I am sorry for taking your time twice.
Attachment #8572403 -
Attachment is obsolete: true
Attachment #8582830 -
Flags: review?(bmcbride)
Assignee | ||
Comment 16•9 years ago
|
||
Unbitrotted. Carrying over review+
Attachment #8582833 -
Flags: review+
Assignee | ||
Comment 17•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=feaf53c95f7e
Comment 18•9 years ago
|
||
Comment on attachment 8582830 [details] [diff] [review] Part 3: use MockRegistrar in toolkit/mozapps v2 Review of attachment 8582830 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Hiroyuki Ikezoe (:hiro) from comment #15) > :Unfocused, I am sorry for taking your time twice. No problem! Thank you for taking the time to do all this :)
Attachment #8582830 -
Flags: review?(bmcbride) → review+
Comment 19•9 years ago
|
||
Comment on attachment 8582824 [details] [diff] [review] Part 1: Introduce a new js module to register mock easily v2 Review of attachment 8582824 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.
Attachment #8582824 -
Flags: review?(gps) → review+
Assignee | ||
Updated•9 years ago
|
Attachment #8572402 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8582824 -
Attachment description: Introduce a new js module to register mock easily v2 → Part 1: Introduce a new js module to register mock easily v2
Comment 21•9 years ago
|
||
seems part 3 failed to apply: renamed 1139254 -> use_MockRegistrar_in_toolkit_mozapps.patch applying use_MockRegistrar_in_toolkit_mozapps.patch patching file toolkit/mozapps/update/tests/data/xpcshellUtilsAUS.js Hunk #3 FAILED at 2964 1 out of 3 hunks FAILED -- saving rejects to file toolkit/mozapps/update/tests/data/xpcshellUtilsAUS.js.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh use_MockRegistrar_in_toolkit_mozapps.patch could you take a look, thanks!
Flags: needinfo?(hiikezoe)
Assignee | ||
Comment 22•9 years ago
|
||
Just resolved conflict against the fix for bug 1148070, nothing has been changed in its logic. Now this patch can be applied to current tip. Confirmed all tests in toolkit/mozapps work fine locally.
Assignee: nobody → hiikezoe
Attachment #8582830 -
Attachment is obsolete: true
Flags: needinfo?(hiikezoe)
Attachment #8585363 -
Flags: review+
Comment 23•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2f70dc09ad0 https://hg.mozilla.org/integration/mozilla-inbound/rev/4938efa0a6cc https://hg.mozilla.org/integration/mozilla-inbound/rev/a9436c210dca https://hg.mozilla.org/integration/mozilla-inbound/rev/d30cd8f36a66 https://hg.mozilla.org/integration/mozilla-inbound/rev/aa90ef70bf28 https://hg.mozilla.org/integration/mozilla-inbound/rev/d0c8bfdb7cd9 https://hg.mozilla.org/integration/mozilla-inbound/rev/65591e625b43
Flags: in-testsuite+
Keywords: checkin-needed
Comment 24•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d2f70dc09ad0 https://hg.mozilla.org/mozilla-central/rev/4938efa0a6cc https://hg.mozilla.org/mozilla-central/rev/a9436c210dca https://hg.mozilla.org/mozilla-central/rev/d30cd8f36a66 https://hg.mozilla.org/mozilla-central/rev/aa90ef70bf28 https://hg.mozilla.org/mozilla-central/rev/d0c8bfdb7cd9 https://hg.mozilla.org/mozilla-central/rev/65591e625b43
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•