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)

defect
Not set
normal

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.
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)
Attachment #8572402 - Flags: review?(gps)
Attachment #8572403 - Flags: review?(gps)
Attachment #8572406 - Flags: review?(gps)
Attachment #8572405 - Attachment description: use MockRegistrar in netwerk. → Part 5: use MockRegistrar in netwerk.
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+
Attachment #8572404 - Flags: review?(gps) → review+
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 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 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)
Attachment #8572406 - Flags: review?(gps) → review+
Attachment #8572407 - Flags: review?(gps) → review+
Attachment #8572405 - Flags: review?(jduell.mcbugs) → review+
Attachment #8572402 - Flags: review?(bmcbride) → review+
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+
(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)
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)
Unbitrotted.
Carrying over review+
Attachment #8582833 - Flags: review+
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 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+
Attachment #8572402 - Attachment is obsolete: true
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
Thanks all!
Keywords: checkin-needed
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)
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+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: