Import module of Outlook Express should be rewritten with nsIWindowsRegKey

RESOLVED FIXED in Thunderbird 16.0

Status

MailNews Core
Import
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: hiro, Assigned: hiro)

Tracking

Trunk
Thunderbird 16.0
All
Windows XP
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

5 years ago
We can test the module of settings import with the mock of nsIWindowsRegKey
(Assignee)

Comment 1

5 years ago
Created attachment 601830 [details] [diff] [review]
Proposed fix
Attachment #601830 - Flags: review?(dbienvenu)
(Assignee)

Comment 2

5 years ago
Created attachment 601831 [details] [diff] [review]
Tests

These test covers only normal case, but I suppose it is sufficient for now.
Attachment #601831 - Flags: review?(mconley)

Updated

5 years ago
Assignee: nobody → hiikezoe
Status: NEW → ASSIGNED

Comment 3

5 years ago
Comment on attachment 601830 [details] [diff] [review]
Proposed fix

thx, this looks fine - I don't have an OE profile on my machine, but the unit test works fine.
Attachment #601830 - Flags: review?(dbienvenu) → review+
Comment on attachment 601831 [details] [diff] [review]
Tests

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

Wow, nice tests! :)

This looks really good - just a few nits - see below.

Thanks,

-Mike

::: mailnews/import/test/unit/test_oe_settings.js
@@ +1,3 @@
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +const am = Cc["@mozilla.org/messenger/account-manager;1"]

am and smtpService can be pulled from MailServices (http://mxr.mozilla.org/comm-central/source/mailnews/base/util/mailServices.js).

@@ +10,5 @@
> +let gOriginalCID = Components.manager.contractIDToCID(CONTRACT_ID);
> +let factory;
> +let uuid;
> +
> +function POP3Account() {

This can be a one-liner, like:

function POP3Account() {}

@@ +12,5 @@
> +let uuid;
> +
> +function POP3Account() {
> +}
> +POP3Account.prototype = {

Newline before this.

@@ +35,5 @@
> +  'SMTP Use Sicily': 1,
> +  'SMTP User Name': 'smtpuser'
> +};
> +
> +function IMAPAccount() {

One-liner.

@@ +37,5 @@
> +};
> +
> +function IMAPAccount() {
> +}
> +IMAPAccount.prototype = {

Newline before this.

@@ +56,5 @@
> +  'SMTP Use Sicily': 2,
> +  'SMTP User Name': 'smtpuser'
> +};
> +
> +function NNTPAccount() {

One-liner.

@@ +58,5 @@
> +};
> +
> +function NNTPAccount() {
> +}
> +NNTPAccount.prototype = {

Newline before this.

@@ +73,5 @@
> +/* Outlook Express 4.0 */
> +function OE4Registry(defaultAccount) {
> +  this._defaultAccount = defaultAccount;
> +}
> +OE4Registry.prototype = {

Newline before this.

@@ +83,5 @@
> +    'Mail': {
> +      'Poll For Mail': 1234 * 60000
> +    },
> +  },
> +  get 'Identities\\{DEFAULT_ID}\\Software\\Microsoft\\Internet Account Manager\\Accounts'() {

Wow - today I learned you can define getters using a string like this.  Neat. :)

@@ +92,5 @@
> +/* Outlook Express 5.0 */
> +function OE5Registry(defaultAccount) {
> +  this._defaultAccount = defaultAccount;
> +}
> +OE5Registry.prototype = {

Newline before this.

@@ +190,5 @@
> +
> +function MockWindowsRegKey(registryData) {
> +  this._registryData = registryData;
> +}
> +MockWindowsRegKey.prototype = {

Newline before this.

@@ +198,5 @@
> +    if (!this._registryData[aRelPath])
> +      throw Cr.NS_ERROR_FAILURE;
> +    this._keyPath = aRelPath;
> +  },
> +  close: function() {

Let's separate all of these functions, for readabilities sake.

@@ +331,5 @@
> +    let account = am.accounts.QueryElementAt(i, Ci.nsIMsgAccount);
> +    am.removeAccount(account);
> +  }
> +  let smtpServers = smtpService.smtpServers;
> +  while (smtpServers.hasMoreElements()) {

Newline before this while block.
Attachment #601831 - Flags: review?(mconley) → review-
(Assignee)

Comment 5

5 years ago
Created attachment 603067 [details] [diff] [review]
Revised test

Addressed mconley's comments.
Attachment #601831 - Attachment is obsolete: true
Attachment #603067 - Flags: review?(mconley)
Comment on attachment 603067 [details] [diff] [review]
Revised test

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

Hey hiro - sorry it took me so long to get to this.

Just a few more tiny nits. After those are fixed, and assuming these tests actually pass, r=me.

::: mailnews/import/test/unit/test_oe_settings.js
@@ +1,1 @@
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");

I think it'd be good to have a comment at the start of this file saying what exactly we're testing for.

@@ +1,4 @@
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +Components.utils.import("resource:///modules/mailServices.js");
> +
> +const registrar = Components.manager.QueryInterface(Ci.nsIComponentRegistrar);

why is registrar lowercase and CONTRACT_ID in upper?  I think you should choose a consistent style.  Either all caps, or prefix with k, like kRegistrar, kContractID.

@@ +339,5 @@
> +  for (let i = 0; i < accounts.Count(); i++) {
> +    let account = accounts.QueryElementAt(i, Ci.nsIMsgAccount);
> +    MailServices.accounts.removeAccount(account);
> +  }
> +  let smtpServers = MailServices.smtp.smtpServers;

I'd put a newline after this for block finishes.
Attachment #603067 - Flags: review?(mconley) → review+
(Assignee)

Comment 7

5 years ago
(In reply to Mike Conley (:mconley) from comment #6)
> Comment on attachment 603067 [details] [diff] [review]
> @@ +1,4 @@
> > +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> > +Components.utils.import("resource:///modules/mailServices.js");
> > +
> > +const registrar = Components.manager.QueryInterface(Ci.nsIComponentRegistrar);
> 
> why is registrar lowercase and CONTRACT_ID in upper?

I actually prefer uppercase name for constant variable, but in this case its appearance seems weird for me.

 REGISTRAR.registerFactory(...);

So I'd propose that 'let' is used for the registrar like this:

let registrar = Components.manager.QueryInterface(Ci.nsIComponentRegistrar);

What do you think?
(Assignee)

Comment 8

5 years ago
Created attachment 609609 [details] [diff] [review]
Updated fix

Adapted to the latest trunk.
Attachment #601830 - Attachment is obsolete: true
Attachment #609609 - Flags: review+
(Assignee)

Comment 9

5 years ago
Created attachment 609612 [details] [diff] [review]
Revised test

This patch is addressed mconley's comment.

And fixed the failure of smtp server port check on try server because the try server is not applied KB933612. See http://mxr.mozilla.org/comm-central/source/mailnews/import/oexpress/nsOESettings.cpp#873
So I've changed the expected smtp server port 465 to avoid the failure.

And also fix the run-if directive in xpcshell.ini.

I pushed these fixes to try server. The fixes does not contain the correct run-if directive and registrar name is lower-case, but I suppose it is OK.
http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=0a0cc63aeb50
Attachment #603067 - Attachment is obsolete: true
Attachment #609609 - Attachment is obsolete: true
Attachment #609612 - Flags: review+
(Assignee)

Comment 10

5 years ago
Comment on attachment 609609 [details] [diff] [review]
Updated fix

Oops! I did make this obsolete..
Attachment #609609 - Attachment is obsolete: false
(Assignee)

Comment 11

5 years ago
Created attachment 630419 [details] [diff] [review]
Adapt to the latest trunk
Attachment #609609 - Attachment is obsolete: true
Attachment #630419 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
This has bitrotted again. Please rebase and I'll get it checked in.
Keywords: checkin-needed
Hiro ?
(Assignee)

Comment 14

5 years ago
Created attachment 635511 [details] [diff] [review]
de-bitrotted
Attachment #630419 - Attachment is obsolete: true
Attachment #635511 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/73bc66a4817e
https://hg.mozilla.org/comm-central/rev/9f8c975b8d30
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 16.0

Updated

5 years ago
Depends on: 770330
You need to log in before you can comment on or make changes to this bug.