In bug 432812, a basic unit test environment for mailnews was created. In that environment, the head creates a basic profile directory, while in the tail that directory is removed, plus additional cleanup is done of components that might have been created (account manager, ab directories) during the testing. Here I propose that the head and tail implement complementary functions that setup and tear down different aspects of the mailnews environment. These functions would be called at the beginning and end of run_test(). The advantage of this would be: 1) The check-interactive problems of bug 384339 would no longer affect debugging. 2) Tear down could be easily turned off if necessary, for example to examine files created during the tests. 3) Each test author would have more control of what setup and tear down is done, so that for example address book tear down is not done for tests that are unrelated to the address book.

(In reply to comment #0) > 1) The check-interactive problems of bug 384339 would no longer affect > debugging. Let's ignore that for the purposes of this discussion and assume it works fine. That bug seem to stagnate but its potentially moving forward now, and if it doesn't get fixed it'll make some core tests difficult, not just ours. > 2) Tear down could be easily turned off if necessary, for example to examine > files created during the tests. This is where I think a function wouldn't give you that much of an advantage. If you didn't close down properly, then I'm a bit concerned that either you'd get more problems, or I'm wondering if everything would get committed all the time. In theory it would do its just a thought. Anyway, I have a patch in mind for this in the existing system. I'll attach it in a while. > 3) Each test author would have more control of what setup and tear down is > done, so that for example address book tear down is not done for tests that are > unrelated to the address book. Set-up wise it is just the profile directory. That is something that could arguably have been implemented at core level. I understand some of your point about tear down, but I don't see what is causing your problem. If its not needed but run anyway, it will still work. From the discussion on irc it sounded more like you had a problem in that the current tear-down didn't do enough for mailnews, rather than it needing to be selective. My general point of view on tests are that: 1) each test should tidy up after itself, cleanly. 2) the author of new tests should have to write as little as possible to get the test working i.e. not have to call setup/tear down functions.

This patch once applied would only require the author to put a line into a test file saying: LEAVE_PROFILE_FILES=1 and this would leave the profile files alone in tail (i.e. not delete them) but still shutdown cleanly.

For now, I'll use this bug to document problems I am having. We'll decide later what changes, if any, to do to the setup. Currently I'm trying to debug setting up local folders in bug 217034. I'm running in a debugger where it dies. Each time it dies, the old profile is left intact, and so more and more local folders are being created. That is, my current local folder is located at mailtest/Mail/Local Folders-4 Recommended solution: remove profileDir if it exists at the beginning of setup. Your patch does this. However, you don't do it if the LEAVE_PROFILE_FILES is set. I think you should delete the profile whether LEAVE_PROFILE_FILES is set or not.

Oops, on second glance your patch does not do this. You would need to add profileDir.remove() in the head as well.

(In reply to comment #3) > Recommended solution: remove profileDir if it exists at the beginning of setup. So, is there likely to be a case where someone wants to manually place files in the profile directory whilst setting up the test and hence wouldn't want this to happen?

This shows an example (not debugged) of an organization scheme using symmetric load/unload functions for setup of different aspects of the mailnews environment. In the existing mailCleanup.js, some items are added to cleanup (like the ab stuff) but there is no clear path to add additional cleanup requirements (such as I need for local mail folders). With the proposed structure, the plan is clear: add load and unload functions for each required setup. The example .js is incomplete, setup of other possible environments (newsreader, address book) would need to be added to this. You could still, if you want, use head_ and tail_ files to manage this. But I would not setup and tear down by default the addressbook in the mailnews/base directories. But as an example, in my tests for bug 187768 I will need the address book. So in run_test I would simply need to add loadAddressBook and unloadAddressBook.

(In reply to comment #5) > (In reply to comment #3) > > Recommended solution: remove profileDir if it exists at the beginning of setup. > > So, is there likely to be a case where someone wants to manually place files in > the profile directory whilst setting up the test and hence wouldn't want this > to happen? > I don't really have enough experience with unit testing to answer this definitively. But it seems to me that unit tests are supposed to start with a clean environment. Any required setup, such as copying files, should be done in the test itself, and not in some program that presumably runs before the tests.

Here's another specific issue: I'm trying to understand why I need to call "prettiestName" on the Inbox or it fails. I'm all setup with the debugger - but unfortunately the setup of the Inbox is being done before run_test. So I can't use my batch file which simulates check-interactive to debug. What I wish I had done is include as little setup as possible prior to run_test. That is, I wish I had a routine such as: loadLocalMailFolders(); which I run at the beginning of run_test. The point I am trying to make is that most of my time in writing tests has been spent debugging setup issues. Those are much more difficult to debug if they are done automatically or outside of the run_test routine.

(In reply to comment #8) > The point I am trying to make is that most of my time in writing tests has been > spent debugging setup issues. Those are much more difficult to debug if they > are done automatically or outside of the run_test routine. Maybe this is part of our misunderstanding each other. The *only* thing I would do be default in the setup (head) would be to set up the profile directory service, this only makes sure NS_GetSpecialDirectory works. Account Manager, and other initialisations are the responsibility of run_test because each situation/test will want different methods. On the tear down side I would want to do more, even if it is unnecessary. There's several reasons for this (which I remembered last night after having gone to bed): 1) Linux lets the profile directory removal succeed even though processes still have files. Therefore authors on Linux won't necessarily realise tests fail on Windows & Mac because they haven't called the right shutdown functions. 2a) Provide a consistent clean shutdown of mailnews where the author of the test doesn't have to work out/remember what functions he has to call to shutdown, e.g. I've tested some of the compose code, should I be shutting down the address book? 2b) If we were to enable leak detection on unit tests we'd want to know we're shutting down cleanly and consistently in all instances. 3) If we are calling provided functions to tear down, and you are debugging a test that requires a specific setup then you'll either have to wrap all your tests in try {} catch{} and probably a finally{}, or do the calls in the tail, which is what we do now.

It's not easy for me to separate problems due to real needs to change the unit test environment, from transient issues due to my own learning curve/moving target due to bug 432812/bug 384389/need to develop new setup & tear down for local accounts. But I'll try. Here's my updated list of things we can try to agree to: 1) The unit test environment philosophy is to only setup the profile directory in the head, but add to the tail a robust tear down. 2) Based on 1), I need to add a tear down of the local mail accounts to the standard tail, since they are needed in most of the tests I am developing. 3) I would prefer to see the setup and tear down organized into a series of load and unload functions in a single .js file. The unload functions would not require the load functions, but would successfully tear down the setup of the load function if called. This accomplishes several goals. First, test writers can easily call a function like loadLocalMailAccounts to get a relatively standard setup if that is appropriate. Second, in debugging setup and tear down issues, it will be very clear which tear down operations are associated with which part of the setup. Third, in manual runs of xpcshell in debugging, it will be easier to simulate the head and tail operations through calls to specific functions. Fourth, in adding new test environment features (such as the local mail accounts) it will be clear that the task is to prepare appropriate load and unload functions, and add the unload function to the standard tail. Multiple variants of the load functions could also be created for different standard situations. 4) The profile setup should succeed even if the previous tear down failed. Deleting any previous profile directory at setup is one way to do this. 5) It should be easy to leave the profile directory intact after test to assist in debugging. Your LEAVE_PROFILE_FILES would do this.

Here I add the functionality to support local mail directories in mailnews test. This demonstrates my proposal on use of load and unload functions. We could accept this patch as a way to move forward for now. If you like the approach, then perhaps later we could add load and unload functions for the other mailnews setup features.

I'd be interested in hearing thoughts from Jeff & Rob, since they have an awful lot of experience with our testing infrastructure..

Having seen Kent's patch (attachment 322052 [details] [diff] [review]) my current thoughts are: * a set of functions for setting things up (e.g. local mail folders) is a good idea ** but they shouldn't leave variables in global scope (this could get messy once we get possibilities for different protocols, setups etc) * I'm still not sure about the tear-down functions. I still think one common consistent script for correctly shutting down mailnews would be best. The argument that there are different "unload" functions providing greater control goes away when patches in folders start testing things cross-mailnews (e.g. compose tests also use address book). * We should just define Ci/Cc globally for mailnews tests (at least) and be done with it. Thoughts of others would be much appreciated.

(In reply to comment #13) > ** but they shouldn't leave variables in global scope I might reword this to say "they should be very conservative about leaving variables in global scope." They have to leave SOMETHING in global scope or they can't be used, even if only a function definition. The existing mailDirService.js leaves a lot of variables in global scope: nsIFile, NS_APP_USER_PROFILE_50_DIR, MailTestDirServer, dirSvc, profileDir. I thought I was being MORE conservative about global scope than seems to be common practice. What is the global object in XPCSHELL? Another option would be to add the global variables programmatically in the loadXxxx.js functions if there is a global object (which I assume there is?)

(In reply to comment #13) > * I'm still not sure about the tear-down functions. I still think one common > consistent script for correctly shutting down mailnews would be best. The > argument that there are different "unload" functions providing greater control > goes away when patches in folders start testing things cross-mailnews (e.g. > compose tests also use address book). IMO, having the ability to register a tear-down function would be really useful for other unit tests outside of mailnews too.

(In reply to comment #14) > (In reply to comment #13) > > > ** but they shouldn't leave variables in global scope > > I might reword this to say "they should be very conservative about leaving > variables in global scope." Yes I'd agree with that. My general thought is that if you've just used a function to set something up, then that function should give you back what you've just set up, otherwise you've got two things to think about, the first being what's the function called, the second being which objects you then can use. I realise the profile service is different. (In reply to comment #15) > (In reply to comment #13) > > * I'm still not sure about the tear-down functions. I still think one common > > consistent script for correctly shutting down mailnews would be best. The > > argument that there are different "unload" functions providing greater control > > goes away when patches in folders start testing things cross-mailnews (e.g. > > compose tests also use address book). > IMO, having the ability to register a tear-down function would be really useful > for other unit tests outside of mailnews too. Shawn, could you expand on this please? Do you mean register from the test file rather than calling it in the tail? If that is what you mean, why not just do it in the tail anyway?

As a nit, I'll use the name (un)loadLocalMailAccount() if the patch is accepted, since there is only one account. Perhaps also, as suggested indirectly in comment 16, loadLocalMailAccount should return inboxFolder.

(In reply to comment #16) > Shawn, could you expand on this please? Do you mean register from the test file > rather than calling it in the tail? If that is what you mean, why not just do > it in the tail anyway? Well, the tail file runs immediately after the test file runs, correct? I hit an issue yesterday where things were running before all calls to do_test_finished were done, so I couldn't actually clean up my test environment because tests were still running. Any type of async tests are going to have this problem.

Some thoughts and observations clicked together today, and I came up with this patch. I'm hoping this is much more to everyone's liking. What it does: - Tidies the mailDirService.js implementation, removes things out of global scope. - Globally defined Ci/Cc/Cr/CC - Provides the init function for local mail folder accounts (note, I've kept slightly different globals for now, but provided what are hopefully clearer names as to what they actually are/where they come from). - Removed the current tail_*.js system of tear down. - Implement a new tidy_*.js system. The new tidy_*.js system is xpcshell based, but runs in a separate process *after* the test_*.js process has run. What this means is that each test_foo.js script is run, and shutdown cleanly via an xpcom shutdown (xpcshell does this for us [1]). We then will run any tidy_*.js file in the directory, and hence mailnews can use it for removing the "profile" directory. Now I've found it, and assuming it works on all platforms, I think this is the best solution. Implementers don't have to worry about correct shutdown, if they don't want the file(s) to be removed, they can comment out the line in tail_*.js (I may have to redo the comment there for mailnews). So, thoughts/comments? [1] http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/js/src/xpconnect/shell/xpcshell.cpp&rev=1.110&mark=1477#1477

Oh and assuming this works, I'll move the xpcshell test extension parts to a separate core bug to get them submitted.

I've tried this patch, and it seems to work for me. However, my preference would be that the mailtest profile directory be left in place after the end of each unit test, and deleted as part of the head_ setup to insure a clean environment for each test. I have two main reasons for this: 1) During preparation of unit tests, I frequently want to examine the actual files in the profile area to see the results of setup operations I have done. If the profile is left, this is easy. If it is removed, then I have to enable some sort of feature to leave it alone. 2) During debugging of tests, frequently they hang and have to be killed. Depending on how this is done, the tail_ or tidy_ functions will not run - leaving the profile directory in place. This sometimes causes a spurious failure of the unit test on the next run. As a bonus, this will eliminate the need implement these tidy_ functions, plus let check-interactive work even if bug 384339 is not fixed. If the mailtest profile directory is left intact, perhaps it should be placed in the _tests or _profile directory rather than dist/bin The other changes in Standard8's patch are good, including removing some unneeded globals and reorganizing the globals in my patch.

This patch will remove the profile directory before the test, and places it in (hopefully) <objdir>/_tests. Hence we'll no longer need awkward clean-up etc.

(In reply to comment #22) > Created an attachment (id=322743) [details] > Better Fix v2 > > This patch will remove the profile directory before the test, and places it in > (hopefully) <objdir>/_tests. > > Hence we'll no longer need awkward clean-up etc. > I ran this patch, and it didn't do what you say. The profile is still in dist/bin. Also, I don't see that mailtest is removed before the test. After running make -C mailnews/base/test I have both "Local Folders" and "Local Folders-1" directories.

Let's try attaching the correct version of the patch this time.

(In reply to comment #24) > Created an attachment (id=322748) [details] > Better Fix v2a > I have some small nits with this, but generally I'm very happy with this. 1) As I said in comment 17, please use loadLocalMailAccount() instead of the plural. This was my error, I was mixing folders with accounts. There is only one local mail account. 2) When I run check-interactive, it now works, but the opening line looks like this: test_bug434810.js: e:\434810-M20080528\tb-debug\_tests\mailtestjs> That is, somewhere there is a missing \n in a dump, so the mailtest collides with the js prompt.

Comment on attachment 322925 [details] [diff] [review] Better Fix v3 Fixes the latest comments (removed an unneeded dump statement and fixed function name).

Comment on attachment 322925 [details] [diff] [review] Better Fix v3 Looks great; very clean. r=dmose with the following nits considered or addressed (at your discretion). >+function run_test() >+{ >+ loadLocalMailAccount(); >+ >+ var rootFolder = gLocalMailAccount.rootFolder; >+ >+ do_check_eq(rootFolder.numSubFolders, 3); >+ do_check_true(rootFolder.containsChildNamed("Inbox")); It's not obvious to the reader where the 3 comes from. This would be clearer if we just had an array something like ["Inbox", "Drafts", "Trash"], checked for all them, and checked numSubFolders against the .length property of the array. >+// Left as a global to make things like copying files easy. >+var profileDir = initializeDirServer(); gProfileDir, perhaps? >+// Local Mail Folders. Requires prior setup of profile directory >+ >+var gLocalMailAccount; >+var gLocalInboxFolder; >+ >+function loadLocalMailAccount() >+{ >+ var acctMgr = Cc["@mozilla.org/messenger/account-manager;1"] >+ .getService(Ci.nsIMsgAccountManager); >+ acctMgr.createLocalMailAccount(); >+ >+ gLocalMailAccount = acctMgr.localFoldersServer; Given that this is not an nsIMsgAccount, would gLocalIncomingServer be a more intuitive name?

I've checked this in with the nits address with one minor modification (after a bit of experimentation) in the unit test test_bug434810.js we have to get the string service and bundle for messenger.properties so we can get the right names for the unsent messages and trash folders - TB & SM are different, hence requiring us to get the string bundle.