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

RESOLVED FIXED in Thunderbird 17.0

Status

MailNews Core
Testing Infrastructure
--
trivial
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: sgautherie, Assigned: SAKURAI Kenichi)

Tracking

(Blocks: 2 bugs)

Trunk
Thunderbird 17.0
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug], URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

6 years ago
"Found 69 matching lines in 69 files"

Updated

6 years ago
Blocks: 720356
No longer blocks: 592653

Updated

6 years ago
Blocks: 592653

Updated

5 years ago
Summary: Use Services.prefs instead of preferences-service, in Mailnews Core → Use Services.prefs instead of preferences-service, in Mailnews Core test files
(Assignee)

Comment 1

5 years ago
I would like to take this in Mozilla Hackathon 2012 in Japan.
Assignee: nobody → kenichi
(Assignee)

Comment 2

5 years ago
Created attachment 646827 [details] [diff] [review]
Proposed Patch
Attachment #646827 - Flags: review?(mconley)
(Reporter)

Comment 3

5 years ago
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

5 years ago
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 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.
Attachment #646827 - Flags: review?(mconley) → review-
(Assignee)

Comment 6

5 years ago
Thanks for your comments and sorry for my very late response.

Please find attached new patches.
(Assignee)

Comment 7

5 years ago
Created attachment 651621 [details] [diff] [review]
modified patches
Attachment #646827 - Attachment is obsolete: true
Attachment #651621 - Flags: review?(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.
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 on attachment 651621 [details] [diff] [review]
modified patches

r-'ing until patch is de-bitrotted.
Attachment #651621 - Flags: review?(mconley) → review-
(Assignee)

Comment 11

5 years ago
Created attachment 653680 [details] [diff] [review]
Modified new patches

Thanks for your review and comments. Please find attached new patches.
Attachment #651621 - Attachment is obsolete: true
(Reporter)

Comment 12

5 years ago
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?
(Assignee)

Comment 13

5 years ago
Created attachment 654175 [details] [diff] [review]
Modified new patches

Thanks for your comments.

I fixed.
Attachment #653680 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #654175 - Flags: review?(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?
Attachment #654175 - Flags: review?(mconley) → review-
(Assignee)

Comment 15

5 years ago
Created attachment 654665 [details] [diff] [review]
Updated patches

Thanks. I updated patches.
Attachment #654175 - Attachment is obsolete: true
Attachment #654665 - Flags: review?(mconley)
Comment on attachment 654665 [details] [diff] [review]
Updated patches

Beautiful! Thanks!
Attachment #654665 - Flags: review?(mconley) → review+

Updated

5 years ago
Keywords: checkin-needed
Thanks for the patch!

https://hg.mozilla.org/comm-central/rev/c085b0f2711e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 17.0
You need to log in before you can comment on or make changes to this bug.