498154 - Provide a simple framework to pump messages through the POP3 fake server

Status: RESOLVED FIXED

(MailNews Core :: Networking: POP, enhancement)

Description

[Kent James (:rkent)]

In performing cross-folder copy and filter tests, I really just want to pump a list of messages through the POP3 fake server. Provide a very easy to use testing module to do that.

Comment 1

[Kent James (:rkent)]

Attachment: Fully working, but I want to use it some first.

This patch is a fully functional, but I want to use it in some tests first to make sure I am satisfied with how it works.

Comment 2

[Kent James (:rkent)]

Attachment: Just a few tweaks, ready for review

I use this to test mail filtering in bug 497622, which is a complex usage involving cross-server operations. I added a few tweaks that came up there.

Comment 3

[Kent James (:rkent)]

Attachment: load maild.js if needed

While trying to use the POP3Pump in a test in mailnews/base/tests, I see that maild.js is not by default loaded there. So I added it to POP3Pump to keep the required boilerplate in unit tests to a minimum.

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 384160 [details] [diff] [review]
load maild.js if needed

Is there a special reason not to create this as a standard javascript object/component?

IMHO it would make it clearer in what it is doing and more maintainable.

Comment 5

[Kent James (:rkent)]

I had expected a discussion of whether to implement this as a javascript module, not as a javascript object. Let me argue both.

First, I want to make sure that the idiom is understood, as it is not commonly used in mozilla code, but is elsewhere. The idiom:

(function ()
  this.moduleName = {};
  let privateVariable;
  this.moduleName.aPublicObject = localObject;
  ... more code
})();

is sometimes used in large javascript code bases to modularize their components. For example, it is the standard idiom used to wrap functionality in the Yahoo YUI library, and in dojo. The preamble makes localObject accessible from other javascript using the identifier aPublicObject, but prevents global functions from accessing privateVariable. The closure created by the creation of the global object keeps the entire anonymous function, including all of its submembers, alive for the duration of the global reference.

The use of the idiom is precisely for reasons of maintainability, as it keeps unnecessary variables out of the global namespace, and prevents callers from accessing internal variables. It makes it possible to have module-wide common variables, such as "firstFile" in POP3Pump.js, without using either a global variable, or the more verbose "this.firstFile" that would be normal practice in a javascript object.

In the current case, I partly used the idiom for reasons of convenience. I am wrapping working code from test_pop3GetNewMail.js that was originally written with lots of global variables. By wrapping that code in this idiom, I can modularize the original code with very little modification.

The option of using a javascript module is inconvenient in this case because the unit test framework is written with lots of global variables that must be accessible, which is more complex to manage with modules.

I think the issue comes down to one of familiarity with the idiom. To one who is familiar with this idiom, its use here is quite natural.

Perhaps it would help if I included in the comments a brief description of the idiom to help those who might not be familiar with it?

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 384160 [details] [diff] [review]
load maild.js if needed

I think the main objections to this that I have (as we discussed on irc) is that I think it will be difficult for maintenance because it is hard to see what is actually going on. There are a mixture of functions and variables in no apparent order, and it is hard to work out where the exported functions happen because they aren't all in one place.

I've also been thinking about suggesting the use of asuth's asyncTestUtils.js but that might not fit into this model too well.

Comment 7

[Kent James (:rkent)]

Attachment: abandon module idiom; precall folder and server setup

Well I still prefer the version with the module idiom (and it was much closer to the original code that I wrapped), but whatever. Here's the traditional version.

I also tweaked the module once more to preload the server and local mail folder, as I found myself needed to do an initial null run through the code in some use cases just to get access to the server object prior to the real test.

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 385429 [details] [diff] [review]
abandon module idiom; precall folder and server setup

>+  this.kPOP3_PORT = 1024 + 110; // will this conflict with an existing fake server?

Only if we're running two pop3 fake servers at the same time. I don't think that will be an issue generally.

>+  // Setup the daemon and server
>+  // If the debugOption is set, then it will be applied to the server.
>+POP3Pump.prototype._setupServerDaemon = function _setupServerDaemon(aDebugOption)

nit: no need for indent of the comment.

>+POP3Pump.prototype._testNext = function _testNext()
>+{
>+  let thisFiles = this._tests.shift();
>+  if (!thisFiles )

nit: no space before )


r=Standard8

Comment 9

[Kent James (:rkent)]

Attachment: Fixed Standard8's nits, patch to checkin [Checkin: Comment 10]

Carrying over review, patch to checkin.

Comment 10

[Serge Gautherie (:sgautherie)]

Comment on attachment 386049 [details] [diff] [review]
Fixed Standard8's nits, patch to checkin
[Checkin: Comment 10]


http://hg.mozilla.org/comm-central/rev/141d4eed12c4