Last Comment Bug 707789 - Use Services.prefs instead of preferences-service, in Mailnews Core test files
: Use Services.prefs instead of preferences-service, in Mailnews Core test files
Status: RESOLVED FIXED
[good first bug]
:
Product: MailNews Core
Classification: Components
Component: Testing Infrastructure (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: Thunderbird 17.0
Assigned To: SAKURAI Kenichi
:
Mentors:
http://mxr.mozilla.org/comm-central/s...
Depends on:
Blocks: 592653 720356
  Show dependency treegraph
 
Reported: 2011-12-05 12:35 PST by Serge Gautherie (:sgautherie)
Modified: 2012-08-27 10:05 PDT (History)
6 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed Patch (29.67 KB, patch)
2012-07-28 02:13 PDT, SAKURAI Kenichi
mconley: review-
Details | Diff | Review
modified patches (74.24 KB, patch)
2012-08-13 20:29 PDT, SAKURAI Kenichi
mconley: review-
Details | Diff | Review
Modified new patches (71.75 KB, patch)
2012-08-21 00:41 PDT, SAKURAI Kenichi
no flags Details | Diff | Review
Modified new patches (71.75 KB, patch)
2012-08-22 04:39 PDT, SAKURAI Kenichi
mconley: review-
Details | Diff | Review
Updated patches (72.64 KB, patch)
2012-08-23 09:48 PDT, SAKURAI Kenichi
mconley: review+
Details | Diff | Review

Description Serge Gautherie (:sgautherie) 2011-12-05 12:35:44 PST
"Found 69 matching lines in 69 files"
Comment 1 SAKURAI Kenichi 2012-07-27 19:42:30 PDT
I would like to take this in Mozilla Hackathon 2012 in Japan.
Comment 2 SAKURAI Kenichi 2012-07-28 02:13:02 PDT
Created attachment 646827 [details] [diff] [review]
Proposed Patch
Comment 3 Serge Gautherie (:sgautherie) 2012-07-29 15:42:45 PDT
Comment on attachment 646827 [details] [diff] [review]
Proposed Patch

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

::: mailnews/base/test/unit/test_accountMgr.js
@@ +11,4 @@
>  const am = Components.classes["@mozilla.org/messenger/account-manager;1"]
>                       .getService(Components.interfaces.nsIMsgAccountManager);
>  
> +const prefs = Services.prefs;

I think "Services.prefs" should be used directly, without the need for "const prefs" anymore. (Likewise in other files.)
Unless there is a reason not to.
Comment 4 Robert Kaiser (not working on stability any more) 2012-07-29 16:21:07 PDT
Yes, it at least *should* be used directly, I'm right now not sure of the dynamics of JavaScript - the caching effect that Services.* is there for doing *might* work with that indirect reference via the const, but it's *sure* to work when used directly - and it's short enough to be used in those cases, usually.
Comment 5 Mike Conley (:mconley) - (Away until June 29th) 2012-08-13 08:39:51 PDT
Comment on attachment 646827 [details] [diff] [review]
Proposed Patch

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

Hey Kenichi,

Thanks so much for your patch!

Just one issue - there are a few places where you're maintaining an alias of Services.prefs (like gPrefs, or prefSvc), when just using Services.prefs is simpler. Can you switch those?

Thanks again!

-Mike

::: mailnews/base/test/unit/test_accountMgr.js
@@ +11,4 @@
>  const am = Components.classes["@mozilla.org/messenger/account-manager;1"]
>                       .getService(Components.interfaces.nsIMsgAccountManager);
>  
> +const prefs = Services.prefs;

Agreed.

::: mailnews/base/test/unit/test_junkingWhenDisabled.js
@@ +161,4 @@
>    gLocalInboxFolder = configure_message_injection({mode: "local"});
>  
>    // Set option so that when messages are marked as junk, they move to the junk folder
> +  let prefSvc = Services.prefs;

Same as above - we don't need to alias Services.prefs.

::: mailnews/base/test/unit/test_nsMsgTraitService.js
@@ +4,4 @@
>  
>  const ts = Cc["@mozilla.org/msg-trait-service;1"]
>               .getService(Ci.nsIMsgTraitService);
> +var traitsBranch = Services.prefs.getBranch("mailnews.traits.");

While you're here, use let instead of var.
Comment 6 SAKURAI Kenichi 2012-08-13 20:27:36 PDT
Thanks for your comments and sorry for my very late response.

Please find attached new patches.
Comment 7 SAKURAI Kenichi 2012-08-13 20:29:23 PDT
Created attachment 651621 [details] [diff] [review]
modified patches
Comment 8 Mike Conley (:mconley) - (Away until June 29th) 2012-08-20 08:18:02 PDT
Comment on attachment 651621 [details] [diff] [review]
modified patches

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

This looks good!

I'm going to push to try to make sure nothing breaks. If that comes back green, you've got my r+.

Thanks!

-Mike

::: mailnews/base/test/unit/test_acctRepair.js
@@ +37,4 @@
>    // This will force the load of the accounts setup above.
>    // We should have created an account for the local folders.
>    do_check_eq(am.accounts.Count(), 2);
> +  dump(Services.prefs.getCharPref("mail.accountmanager.accounts") + "\n");

While we're here, we should remove this dump statement.
Comment 9 Mike Conley (:mconley) - (Away until June 29th) 2012-08-20 08:20:50 PDT
Hm - this patch does not appear to apply cleanly - test_winmail.js appears to be out of date.

Would you mind updating the patch so I can push it to try?

Thanks!
Comment 10 Mike Conley (:mconley) - (Away until June 29th) 2012-08-20 10:32:01 PDT
Comment on attachment 651621 [details] [diff] [review]
modified patches

r-'ing until patch is de-bitrotted.
Comment 11 SAKURAI Kenichi 2012-08-21 00:41:00 PDT
Created attachment 653680 [details] [diff] [review]
Modified new patches

Thanks for your review and comments. Please find attached new patches.
Comment 12 Serge Gautherie (:sgautherie) 2012-08-21 10:07:20 PDT
Comment on attachment 653680 [details] [diff] [review]
Modified new patches

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

::: mailnews/base/test/unit/test_acctRepair.js
@@ -33,5 @@
> -  prefs.setCharPref("mail.accountmanager.defaultaccount", "account6");
> -  prefs.setCharPref("mail.accountmanager.localfoldersserver", "server1");
> -  // This will force the load of the accounts setup above.
> -  // We should have created an account for the local folders.
> -  do_check_eq(am.accounts.Count(), 2);

These 3 lines are not supposed to be removed, are they?
Comment 13 SAKURAI Kenichi 2012-08-22 04:39:01 PDT
Created attachment 654175 [details] [diff] [review]
Modified new patches

Thanks for your comments.

I fixed.
Comment 14 Mike Conley (:mconley) - (Away until June 29th) 2012-08-23 08:37:21 PDT
Comment on attachment 654175 [details] [diff] [review]
Modified new patches

Hm, so this looks pretty good, but I ran it through a try build, and we've got a permanent orange in our XPCShell tests now, with:

/Users/cltbld/talos-slave/test/build/xpcshell/tests/mailnews/db/gloda/test/unit/test_corrupt_database.js:34: ReferenceError: gPrefs is not defined

Any chance you could look into that?
Comment 15 SAKURAI Kenichi 2012-08-23 09:48:46 PDT
Created attachment 654665 [details] [diff] [review]
Updated patches

Thanks. I updated patches.
Comment 16 Mike Conley (:mconley) - (Away until June 29th) 2012-08-27 09:12:19 PDT
Comment on attachment 654665 [details] [diff] [review]
Updated patches

Beautiful! Thanks!
Comment 17 Ryan VanderMeulen [:RyanVM] 2012-08-27 10:05:36 PDT
Thanks for the patch!

https://hg.mozilla.org/comm-central/rev/c085b0f2711e

Note You need to log in before you can comment on or make changes to this bug.