707789 - Use Services.prefs instead of preferences-service, in Mailnews Core test files

Status: RESOLVED FIXED

(MailNews Core :: Testing Infrastructure, defect)

Description

[Serge Gautherie (:sgautherie)]

"Found 69 matching lines in 69 files"

Comment 1

[SAKURAI Kenichi]

I would like to take this in Mozilla Hackathon 2012 in Japan.

Comment 2

[SAKURAI Kenichi]

Attachment: Proposed Patch

Comment 3

[Serge Gautherie (:sgautherie)]

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]

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) (:⚙️)]

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]

Thanks for your comments and sorry for my very late response.

Please find attached new patches.

Comment 7

[SAKURAI Kenichi]

Attachment: modified patches

Comment 8

[Mike Conley (:mconley) (:⚙️)]

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) (:⚙️)]

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) (:⚙️)]

Comment on attachment 651621 [details] [diff] [review]
modified patches

r-'ing until patch is de-bitrotted.

Comment 11

[SAKURAI Kenichi]

Attachment: Modified new patches

Thanks for your review and comments. Please find attached new patches.

Comment 12

[Serge Gautherie (:sgautherie)]

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]

Attachment: Modified new patches

Thanks for your comments.

I fixed.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

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]

Attachment: Updated patches

Thanks. I updated patches.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 654665 [details] [diff] [review]
Updated patches

Beautiful! Thanks!

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Thanks for the patch!

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