432812 - Provide "global" setup and cleanup scripts for mailnews tests and tidy up

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Mark Banner (:standard8)]

Attachment: The fix (full patch)

Now we're starting to get a bit more in place, it seems obvious we need set up and clean up procedures that are easy to apply globally to mailnews.

I think the best way I've found of doing this so far is to provide head_ and tail_ files in all test directories which import global scripts.

tail_ also has an advantage as it makes it easier to cleanup after each test.

The patch I'm attaching does the necessary changes, corrects various comments and drops the old testnum stuff that I had been using from the remaining files that it was in.

I'd like to get this in asap as it is going to affect other tests that are being written.

Comment 1

[Mark Banner (:standard8)]

Attachment: The fix (diff -w, without new files)

Comment 2

[Mark Banner (:standard8)]

Attachment: The fix (full patch) v2

New patch:

- Dropped the redundant definition in mailCleanup.js that was requiring abSetup.js to be imported in all head files.
- Dropped importing abSetup.js from all head files apart from the address book one (I decided that this was going to be needed in the majority of ab test cases, so its probably worth adding there).
- Imported abSetup.js into nsMsgCompose1.js as that needs to know the address book default info.

Comment 3

[Mark Banner (:standard8)]

Attachment: The fix (diff -w, without new files) v2

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

This isn't really reviewable as-is, because, even looking at the raw diffs for the added files, you can't tell which file is which.  Could you regenerate the "diff -w" version _with_ the added files (and meaningful filenames for them) and request review on that?

Comment 5

[Mark Banner (:standard8)]

Attachment: The fix (diff -w, without new files) v2 regenerated

Regenerated the patch. I'm assuming the problems you were getting with review were partially caused by bugzilla not working well with hg patches. Therefore I'm assuming your meaningful file names comment was relating to that and not the filenames I'm using.

If it does relate to the filenames I'm using, then please suggest an improvement, as I think that head_addrbook.js is quite clear that its the head file (i.e. gets run first), for the address book unit tests.

Comment 6

[Mark Banner (:standard8)]

Attachment: The fix (diff -w) v3

This time without the extra mess.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 320758 [details] [diff] [review]
The fix (diff -w) v3

Good stuff!  Just a few nits to address; r=dmose with them addressed.

>Index: mailnews/addrbook/test/resources/abCleanup.js
>===================================================================
>RCS file: mailnews/addrbook/test/resources/abCleanup.js
>diff -N mailnews/addrbook/test/resources/abCleanup.js

Looks like the reference to abCleanup.js in addrbook/test/resources/readme.txt wants to be replaced by some text describing how the cleanup is now done in mailCleanup.js.

>Index: mailnews/addrbook/test/unit/test_nsAbManager2.js
>
> [...]
>
>-
>-  // Test 6 - Clear everything down
>-
>-  gAbManager.removeAddressBookListener(gAblAll);
>-  gAblAll = null;
>-
>-  for (i = 0; i < numListenerOptions; ++i) {
>-    gAbManager.removeAddressBookListener(gAblSingle[i]);
>-    gAblSingle[i] = null;
>-  }

Isn't removing the listeners manually still necessary here, even though nulling out the addressbooks isn't?

>Index: mailnews/base/test/unit/test_nsMsgMailSession_Listeners.js
>===================================================================
>
>-    ++testnum; // Test 1 - Add a listener
>+  // Test 1 - Add a listener

Here and elsewhere, I suggest getting rid of the test numbers entirely.  They'll just get out of sync when somebody adds a test in the middle and forgets to update them.

>Index: mailnews/compose/resources/content/MsgComposeCommands.js
>===================================================================

I'm guessing this wasn't intended to be part of this patch.

>Index: mailnews/test/resources/mailCleanup.js
>===================================================================
>RCS file: mailnews/test/resources/mailCleanup.js
>diff -N mailnews/test/resources/mailCleanup.js
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ mailnews/test/resources/mailCleanup.js	13 May 2008 18:37:03 -0000
>@@ -0,0 +1,47 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/**
>+ * In this file we clean up the address books created during the tests.
>+ *
>+ * Note: This file relies on abSetup.js being loaded.
>+ */

Is the above note actually still true?

>+if (cleanupAcctMgr instanceof Components.interfaces.nsIObserver)
>+  cleanupAcctMgr.observe(null, "xpcom-shutdown", "");

Is the instanceof test here ever going to return false?  If so, how about adding a comment explaining in what eventuality that might happen.

Comment 8

[Mark Banner (:standard8)]

Attachment: [checked in] Final Patch

This is what I actually checked in after addressing Dan's comments.

Comment 9

[Mark Banner (:standard8)]

This is now all checked in -> fixed.

Comment 10

[Kent James (:rkent)]

I tried to use this after checkin, and I've got some problems.

I think the issue is the intepretation of the tail section. This patch interprets it as code that is executed after all of the tests have been completed. However, I think the correct interpretation is that it is code that is loaded after the test code - but before the actual test execution.

My symptom: when running tests with this patch, the profile directory does not exist. I trace this to the fact that tail_mailbase.js (and therefore mailCleanup.js) tears down the profiles when loaded. This occurs BEFORE _execute_test() is called.

If you go to the instructions at http://developer.mozilla.org/en/docs/Writing_xpcshell-based_unit_tests and check how debug examples show that tail.js is loaded before execute_test, then I think you will see that the framework will not work if the tail tears down the setup. The tail needs to load functions that are called at completion to tear down the framework, not run automatically.

Comment 11

[Mark Banner (:standard8)]

Attachment: Log file from tests

Comment 12

[Mark Banner (:standard8)]

(In reply to comment #10)
> My symptom: when running tests with this patch, the profile directory does not
> exist. I trace this to the fact that tail_mailbase.js (and therefore
> mailCleanup.js) tears down the profiles when loaded. This occurs BEFORE
> _execute_test() is called.

This is interesting, it certainly is not what I am seeing. Please see attachment 321032 [details] that I just added. This is the output of a run of test_mailist1.js where I added debug at the end of mailDirService.js, start and end of mailCleanup.js and the start and end of test_mailist1.js.

As you'll see mailCleanup.js is clearly happening after the main test. If it wasn't I'd be expecting more problems (especially on Windows) due to the fact that when the test ended it would be holding onto the files (especially the address book ones) but we seem to be working with that fine on Windows - the SeaMonkey tinderboxes now cover Windows and Linux.

Comment 13

[Mark Banner (:standard8)]

(In reply to comment #10)
> My symptom: when running tests with this patch, the profile directory does not
> exist. I trace this to the fact that tail_mailbase.js (and therefore
> mailCleanup.js) tears down the profiles when loaded. This occurs BEFORE
> _execute_test() is called.

Ok, this problem is bug 384339 which just affects check-interactive. I've already comment on that bug and I think we should fight for that being fixed rather than change what we're doing.

Kent, note that there is also the check-one option. I'm not sure if you need to debug or not, but if you just want to run one of the tests, that would be a better option for you.