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)
MailNews Core
Testing Infrastructure
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)
4.76 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
9.75 KB,
patch
|
standard8
:
review+
|
Details | Diff | 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)
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #656760 -
Flags: review?(mconley)
Comment 2•12 years ago
|
||
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?
Updated•12 years ago
|
Attachment #656758 -
Flags: review?(mconley) → review?(mbanner)
Updated•12 years ago
|
Attachment #656760 -
Flags: review?(mconley) → review?(mbanner)
Assignee | ||
Comment 3•12 years ago
|
||
(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.
Assignee | ||
Comment 4•12 years ago
|
||
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)
Assignee | ||
Comment 5•12 years ago
|
||
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 6•12 years ago
|
||
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-
Assignee | ||
Comment 7•12 years ago
|
||
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)
Comment 8•12 years ago
|
||
I really won't be able to look at this for at least another week. I'm also changing this to a feedback request.
Updated•12 years ago
|
Attachment #657763 -
Flags: review?(kent) → feedback?(kent)
Comment 9•12 years ago
|
||
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 10•12 years ago
|
||
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 11•12 years ago
|
||
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 12•12 years ago
|
||
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+
Assignee | ||
Comment 13•11 years ago
|
||
Attachment #824941 -
Flags: review?
Assignee | ||
Comment 14•11 years ago
|
||
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)
Assignee | ||
Comment 15•11 years ago
|
||
Try server results with these patches: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=246186578529
Assignee | ||
Updated•11 years ago
|
Attachment #824941 -
Flags: review? → review?(mbanner)
Comment 16•11 years ago
|
||
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+
Updated•11 years ago
|
Attachment #824941 -
Flags: review?(mbanner) → feedback+
Assignee | ||
Comment 17•11 years ago
|
||
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)
Comment 18•11 years ago
|
||
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+
Assignee | ||
Comment 19•11 years ago
|
||
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)
Assignee | ||
Comment 20•11 years ago
|
||
Attachment #824942 -
Attachment is obsolete: true
Attachment #832034 -
Flags: review?(mbanner)
Assignee | ||
Updated•11 years ago
|
Attachment #832033 -
Attachment description: bug786936.patch → The MockFactory
Updated•10 years ago
|
Attachment #832034 -
Flags: review?(mbanner) → review+
Comment 21•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 22•10 years ago
|
||
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.
Description
•