Closed Bug 786936 Opened 12 years ago Closed 10 years ago

Need a utility method to register test double

Categories

(MailNews Core :: Testing Infrastructure, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 29.0

People

(Reporter: hiro, Assigned: hiro)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 6 obsolete files)

Attached patch The utitlify (obsolete) — Splinter Review
There are some tests which implementing test double. ex.  mailnews/imap/test/unit/test_imapID.js, mailnews/import/test/unit/resources/mock_windows_reg_factory.js and tests (the test for bug 786487 etc.) which are not checked in yet.

So we should create a utility method to register those test doubles easily.
Attachment #656758 - Flags: review?(mconley)
Attached patch Rewrite tests with the utility (obsolete) — Splinter Review
Attachment #656760 - Flags: review?(mconley)
I just wanted to give my ++ to the concept here of overriding core services in tests to introduce behavior to test.

Looking at https://bugzilla.mozilla.org/attachment.cgi?id=656760, you end up loading your resource into every test that uses alertTestUtils. Isn't there some way to modify alertTestUtils itself to load in your resource instead?
Attachment #656758 - Flags: review?(mconley) → review?(mbanner)
Attachment #656760 - Flags: review?(mconley) → review?(mbanner)
(In reply to Kent James (:rkent) from comment #2)

> Looking at https://bugzilla.mozilla.org/attachment.cgi?id=656760, you end up
> loading your resource into every test that uses alertTestUtils. Isn't there
> some way to modify alertTestUtils itself to load in your resource instead?

I have been considering such way but can't find any good way. Just an idea to make testDoubleUtils js module.
Attached patch Modulerized test double (obsolete) — Splinter Review
This is an attempt to avoid loading testDoubleUtils.js in each tests.

Basically I am fond of this modulerized test double but unfortunately I can't not find a way to use do_register_cleanup in the module. So if something goes wrong in test, registered doubles may not be unregistered cleanly.

Kent, what do you think?
Attachment #657147 - Flags: feedback?(kent)
With the modulerized test double, the diff for asyncTestUtils.js is just:

diff --git a/mailnews/test/resources/asyncTestUtils.js b/mailnews/test/resources/asyncTestUtils.js
--- a/mailnews/test/resources/asyncTestUtils.js
+++ b/mailnews/test/resources/asyncTestUtils.js
@@ -15,16 +15,17 @@
  *   yield async_run({func: something_async});
  *   yield async_run({func: something_that_maybe_is_async});
  *   something_that_is_definitely_not_async();
  *   do_test_finished();
  * }
  */

 Components.utils.import("resource:///modules/errUtils.js");
+Components.utils.import("resource://testing-common/testDouble.js");

 /**
  * Url listener that can wrap another listener and trigger a callback, but
  *  definitely calls async_driver to resume asynchronous processing.  Use
  *  |asyncUrlListener| if you just need a url listener that resumes processing
  *  without any additional legwork.
  *
  * @param [aWrapped] The nsIUrlListener to pass all notifications through to.
Comment on attachment 657147 [details] [diff] [review]
Modulerized test double

I tried to make this work, but I could not. That is, after compiling with the patch loaded, I tried adding a line to an existing test (I used base/test/unit/test_accountMgr.js) but every time I did it, I get this error message and it fails:

Directory request for: resource:testing-common that we (mailDirService.js) are not handling, leaving it to another handler.

This also happens if I ask for the other test module in use (httpd.js) so it is an issue with the underlying setup. How do I get this to work?

Otherwise, comments looking at the code:

+const EXPORTED_SYMBOLS = ["registerTestDouble", "unregisterTestDouble"];

Personally I would prefer if you exported a single global TestDouble, and the functions were then TestDouble.register and TestDouble.unregister

Concerning "find a way to use do_register_cleanup in the module", how about if you added to mailShutdown.js something like this (using my proposed module globals):

if (TestDouble)
  TestDouble.unregister();

I'm not 100% sure that will work, that's what I was hoping to test but could not get past the earlier error.

Otherwise I think that a module is fine, since the infrastructure already partly exists to support httpd.js  Avoiding having to pull in multiple items is a win IMHO.
Attachment #657147 - Flags: feedback?(kent) → feedback-
Attached patch Modulerized test double (obsolete) — Splinter Review
Kent, Thank you for your suggestion. This patch takes your suggestion to unregister test doubles in mailShutdown.js. It works well! 

(In reply to Kent James (:rkent) from comment #6)
> Comment on attachment 657147 [details] [diff] [review]
> Modulerized test double
> 
> I tried to make this work, but I could not. That is, after compiling with
> the patch loaded, I tried adding a line to an existing test (I used
> base/test/unit/test_accountMgr.js) but every time I did it, I get this error
> message and it fails:
> 
> Directory request for: resource:testing-common that we (mailDirService.js)
> are not handling, leaving it to another handler.

This was my mistake. I forgot to add --testing-modules-dir option in config/rules.mk. make xpcshell-tests works magically well without the option but check-one or check-interactive does not work... Yes, I usually use xpcshell-tests.
Assignee: nobody → hiikezoe
Attachment #657147 - Attachment is obsolete: true
Attachment #657763 - Flags: review?(kent)
I really won't be able to look at this for at least another week. I'm also changing this to a feedback request.
Attachment #657763 - Flags: review?(kent) → feedback?(kent)
Comment on attachment 656758 [details] [diff] [review]
The utitlify

Review of attachment 656758 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/test/resources/testDoubleUtils.js
@@ +71,5 @@
> + * Register the test double.
> + *
> + * @param id The UUID of the test double.
> + */
> +function unregisterTestDouble(id) {

nit: I think you should use uuid here to be consistent.
Comment on attachment 656760 [details] [diff] [review]
Rewrite tests with the utility

Review of attachment 656760 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay, I think the general idea is good, but we should make the inclusion a bit simpler.

::: mailnews/compose/test/unit/test_bug155172.js
@@ +2,5 @@
>  /**
>   * Authentication tests for SMTP.
>   */
>  
> +load("../../../resources/testDoubleUtils.js");

Rather than adding this to individual files, I think we could just add it to the head files, or maybe even mailDirService.js.
Attachment #656760 - Flags: review?(mbanner) → review-
Comment on attachment 657763 [details] [diff] [review]
Modulerized test double

I've looked at this quite a bit, but I can't really review it without also trying it out in context, which requires the other patches as well.

As feedback, I like the overall idea, but like Standard8 I think that you should not have to include an extra js file in tests that make particular use of a feature in a global file.
Attachment #657763 - Flags: feedback?(kent) → feedback+
Comment on attachment 656758 [details] [diff] [review]
The utitlify

Cancelling the review on this until I get an updated patch for the rewrite. I think its the right thing to do though.
Attachment #656758 - Flags: review?(mbanner) → feedback+
Blocks: 406851
Attached patch Modularized test double (obsolete) — Splinter Review
Attachment #824941 - Flags: review?
Attached patch Rewrite tests with test double (obsolete) — Splinter Review
This got better.
Attachment #656758 - Attachment is obsolete: true
Attachment #656760 - Attachment is obsolete: true
Attachment #657763 - Attachment is obsolete: true
Attachment #824942 - Flags: review?(mbanner)
Try server results with these patches:
https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=246186578529
Attachment #824941 - Flags: review? → review?(mbanner)
Comment on attachment 824942 [details] [diff] [review]
Rewrite tests with test double

This looks good. The thing that comes to mind at the moment, is that "TestDouble" is a bit confusing. When I came back to this it sounded to me like we were creating a test to test a "double" value or something.

What would you think about making the name something like "MockFactory"? So then you'd be doing MockFactory.register and MockFactory.unregister, which I think makes it a bit clearly that we're Mocking something.
Attachment #824942 - Flags: review?(mbanner) → feedback+
Attachment #824941 - Flags: review?(mbanner) → feedback+
I can agree that "TestDouble" is confusing. Actually I cloud not understand the differences between mock and stub and so on. But once I understand the differences, I realized TestDouble.js is not only for generating mock.
For the reference: http://en.wikipedia.org/wiki/Test_double

Nevertheless, I do not object to rename to MockFactory if there is no guys loving xUnit.
aceman, I would like to hear your opinion.
Flags: needinfo?(acelists)
Blocks: 784888
Comment on attachment 824941 [details] [diff] [review]
Modularized test double

Review of attachment 824941 [details] [diff] [review]:
-----------------------------------------------------------------

I am not sure what this all does, what factories are, etc. But it looks like you merge some duplicated code into a shared module, then I am all for it.

::: mailnews/test/resources/TestDouble.js
@@ +9,5 @@
> +
> +let TestDouble = {
> +  _registeredComponents: {},
> +  /**
> +   * Register a test double overrides target interfaces.

This sentence is strange. Can you explain here what a "test double" is?

@@ +10,5 @@
> +let TestDouble = {
> +  _registeredComponents: {},
> +  /**
> +   * Register a test double overrides target interfaces.
> +   * The target interface may be accessed though _genuine propety of the test double.

... property ...
Attachment #824941 - Flags: feedback+
Status: NEW → ASSIGNED
Flags: needinfo?(acelists)
Attached patch The MockFactorySplinter Review
I've decided to change the name to MockFactory.

(In reply to :aceman from comment #18)
> Review of attachment 824941 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I am not sure what this all does, what factories are, etc. But it looks like
> you merge some duplicated code into a shared module, then I am all for it.
> 
> ::: mailnews/test/resources/TestDouble.js
> @@ +9,5 @@
> > +
> > +let TestDouble = {
> > +  _registeredComponents: {},
> > +  /**
> > +   * Register a test double overrides target interfaces.
> 
> This sentence is strange. Can you explain here what a "test double" is?

Changed to "Register a mock to override target interfaces.".

> @@ +10,5 @@
> > +let TestDouble = {
> > +  _registeredComponents: {},
> > +  /**
> > +   * Register a test double overrides target interfaces.
> > +   * The target interface may be accessed though _genuine propety of the test double.
> 
> ... property ...
Fixed.

try server results:
https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=3265622d1f31
Attachment #824941 - Attachment is obsolete: true
Attachment #832033 - Flags: review?(mbanner)
Attachment #824942 - Attachment is obsolete: true
Attachment #832034 - Flags: review?(mbanner)
Attachment #832033 - Attachment description: bug786936.patch → The MockFactory
Attachment #832034 - Flags: review?(mbanner) → review+
Comment on attachment 832033 [details] [diff] [review]
The MockFactory

Review of attachment 832033 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay, looks good. r=Standard8
Attachment #832033 - Flags: review?(mbanner) → review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/a8155385a420
https://hg.mozilla.org/comm-central/rev/79082bf84f95
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 29.0
You need to log in before you can comment on or make changes to this bug.