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

RESOLVED FIXED in mozilla1.9

Status

MailNews Core
Backend
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

Trunk
mozilla1.9
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

10 years ago
Created attachment 319979 [details] [diff] [review]
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.
Attachment #319979 - Flags: review?(dmose)
(Assignee)

Comment 1

10 years ago
Created attachment 319980 [details] [diff] [review]
The fix (diff -w, without new files)
(Assignee)

Comment 2

10 years ago
Created attachment 320062 [details] [diff] [review]
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.
Attachment #319979 - Attachment is obsolete: true
Attachment #319980 - Attachment is obsolete: true
Attachment #320062 - Flags: review?(dmose)
Attachment #319979 - Flags: review?(dmose)
(Assignee)

Comment 3

10 years ago
Created attachment 320063 [details] [diff] [review]
The fix (diff -w, without new files) v2

Comment 4

10 years ago
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?

Updated

10 years ago
Attachment #320062 - Flags: review?(dmose)

Updated

10 years ago
Blocks: 228675

Updated

10 years ago
Blocks: 217034
(Assignee)

Comment 5

10 years ago
Created attachment 320661 [details] [diff] [review]
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.
Attachment #320063 - Attachment is obsolete: true
Attachment #320661 - Flags: review?(dmose)

Updated

10 years ago
Blocks: 187768
(Assignee)

Comment 6

10 years ago
Created attachment 320758 [details] [diff] [review]
The fix (diff -w) v3

This time without the extra mess.
Attachment #320661 - Attachment is obsolete: true
Attachment #320661 - Flags: review?(dmose)
(Assignee)

Updated

10 years ago
Attachment #320758 - Attachment is patch: true
Attachment #320758 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Updated

10 years ago
Attachment #320758 - Flags: review?(dmose)
(Assignee)

Updated

10 years ago
Attachment #320758 - Attachment description: The fix (diff -w, without new files) v3 → The fix (diff -w) v3

Comment 7

10 years ago
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.
Attachment #320758 - Flags: review?(dmose) → review+
(Assignee)

Comment 8

10 years ago
Created attachment 320886 [details] [diff] [review]
[checked in] Final Patch

This is what I actually checked in after addressing Dan's comments.
Attachment #320062 - Attachment is obsolete: true
Attachment #320758 - Attachment is obsolete: true
(Assignee)

Comment 9

10 years ago
This is now all checked in -> fixed.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED

Updated

10 years ago
Target Milestone: --- → mozilla1.9

Comment 10

10 years ago
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.
(Assignee)

Comment 11

10 years ago
Created attachment 321032 [details]
Log file from tests
(Assignee)

Comment 12

10 years ago
(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.
(Assignee)

Comment 13

10 years ago
(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.
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.