427980 - Re-implement the MailBloatTest on tinderbox

Status: RESOLVED FIXED

(Webtools Graveyard :: Tinderbox, defect, P1)

Description

[Mark Banner (:standard8)]

Attachment: Tinderbox Changes

Thunderbird is looking to get automated test coverage for various areas, and I'm sure SeaMonkey wants this as well.

This bug is for starting to re-implement the MailBloatTest on tinderbox - the ones we currently have there don't work (and hence no-one runs them). See also url above.

I'll cover the basic changes to tinderbox and the current testing structure in this bug, we'll want to extend the MailBloatTest later, but that can be dealt with in separate bugs.

The initial version I am going to post will:

1) Start up the main mail window, followed by the address book window, then close them down in reverse order
2) Measure RLk, Lk, MH and A in the same way as the BloatTest and BloatTest2 tests that Firefox runs.

The first patch I'm attaching is the patch for the tinderbox files (under tools/tinderbox).

Currently the mail profile set-up and chrome extensions are included in the main tinderbox script, I'm thinking of moving them out to alongside the chrome files that I'll attach in a moment.

Comment 1

[Mark Banner (:standard8)]

Attachment: Bloat Test Chrome Files

These are the bloat test files that drive the actual UI. They will live in mozilla/testing/mailnews and currently get copied into the chrome directory of the test build by the tinderbox scripts.

Comment 2

[Mark Banner (:standard8)]

Attachment: [checked in] Remove old smoketest files

This patch will remove the old smoketest files from mailnews/extensions that are no longer needed.

Comment 3

[Mark Banner (:standard8)]

For reference, the required changes to an existing debug tinderbox that is already building with --enable-tracemalloc would be to enable:

$MailBloatTest            = 1;

in tinder-config.pl and also pull the mozilla/testing/mailnews directory.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Any particular reason not to do the compose window as well?

Comment 5

[Andrew Schultz]

Comment on attachment 314587 [details] [diff] [review]
Tinderbox Changes

you might also run an "Alive" test to just open mail and let it sit for a bit (45 secs) and then kill the app.  For TB, you could probably just set MozillaAliveTest to 1.  Not sure how to do that with SM (without adding another test with more code).

>+        system("cp $Settings::TopsrcdirFull/testing/mailnews/bloat/bloatTestOverlay* $binary_dir/chrome/");

you might consider perl's copy (File::Copy).  It looks like glob copy like this is harder, but just copying a single file (you do this a few times) would work fine.

>         if ($talkback_installed) {
>             setTestnameForTalkbackReport($talkback_ini_path,"MailBloattest");
>         }

I guess this can go away.

>+    my $binary_log = "$build_dir/bloattest2.log";
>+    my $malloc_log = "$build_dir/malloc.log";
>+    my $sdleak_log = "$build_dir/sdleak.log";
>+    my $old_sdleak_log = "$build_dir/sdleak.log.old";
>+    my $leakstats_log = "$build_dir/leakstats.log";
>+    my $old_leakstats_log = "$build_dir/leakstats.log.old";
>+    my $sdleak_diff_log = "$build_dir/sdleak.diff.log";

these need new names for seamonkey (mail_foo.log)

>+    my $leaks_testname       = "trace_malloc_leaks";

>+    my $maxheap_testname       = "trace_malloc_maxheap";

>+    my $allocs_testname       = "trace_malloc_allocs";

>+        send_results_to_server($newstats->{'leaks'},  "--", $leaks_testname);
>+        send_results_to_server($newstats->{'mhs'},    "--", $maxheap_testname);
>+        send_results_to_server($newstats->{'allocs'}, "--", $allocs_testname);

for seamonkey, these need different names.

I ran a cycle (with this + fixups) manually on nye and it seemed to work.  I've enabled it now for the normal cycles.

Comment 6

[Mark Banner (:standard8)]

Attachment: Bloat Test Files

I've have now:

- Added opening and closing of the compose window
- Extracted the profile set up parts of the tinderbox script into a script and file in testing/mailnews/bloat
- Re-used the BloatTest2 function in the tinderbox script and extended it to cope with the Mail test (which is basically allowing some prefixes to be added in a nice way.
- Fixed the BloatTest function, so that what it sends to the graph server is all in lower case (to match the rest, plus I'm not sure if its causing problems with the Lk graphs on nye).

The last two issues are changes in the tinderbox scripts that I'll attach in a moment.

Dan, can you review the mailnews files (no sr in this component, so I'll have to take r as r+sr ;-) )?

Once you're reasonably happy, I'll request review on the tinderbox changes.

Comment 7

[Mark Banner (:standard8)]

Attachment: [normal patch checked in] Tinderbox Changes v2 (diff -w)

Comment 8

[Mark Banner (:standard8)]

Attachment: Tinderbox Changes v2 (normal patch)

Comment 9

[Andrew Schultz]

Comment on attachment 314833 [details] [diff] [review]
Tinderbox Changes v2 (normal patch)

>+    if ($bloat_prefix ne '') {
>     unless (-e "$binary_dir/bloaturls.txt") {

indentation

I have this running OK on nye now.

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 314832 [details] [diff] [review]
[normal patch checked in] Tinderbox Changes v2 (diff -w)

This patch is now working on Andrew's SeaMonkey trunk tinderbox (nye). I'm happy that this part is ready to go into the core tinderbox code so we can get it controlled and onto the Thunderbird debug boxes when we get them.

Comment 11

[Robert Helmer [:rhelmer]]

Comment on attachment 314832 [details] [diff] [review]
[normal patch checked in] Tinderbox Changes v2 (diff -w)

>+    if ($bloat_prefix ne '') {
>     unless (-e "$binary_dir/bloaturls.txt") {
>         print_log "Error: bloaturls.txt does not exist.\n";
>         return 'testfailed';
>     }
> 
>-    my $platform = is_windows() ? 'windows' : 'unix';
>     # If on Windows, make sure the urls file has dos lineendings, or
>     # mozilla won't parse the file correctly
>     if ($platform eq 'windows') {
>@@ -3672,11 +3657,16 @@ sub BloatTest2 {
>         close(OUT);
>         File::Copy::move("$bu.new", "$bu") or die("move: $!\n");
>     }
>+    }

Why isn't everything in the "if" block be indented?

Comment 12

[Mark Banner (:standard8)]

(In reply to comment #11)
> Why isn't everything in the "if" block be indented?

My apologies. I've managed to attach the diff -w twice to this bug. I'll attach the correct "normal" patch now.

Comment 13

[Mark Banner (:standard8)]

Attachment: [checked in] Tinderbox Changes v2 (correct normal patch)

Ok, this is the correct one.

Comment 14

[Robert Helmer [:rhelmer]]

Comment on attachment 314832 [details] [diff] [review]
[normal patch checked in] Tinderbox Changes v2 (diff -w)

Ah, ok.

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 315306 [details] [diff] [review]
[checked in] Tinderbox Changes v2 (correct normal patch)

I've checked in the tinderbox change:

http://mxr.mozilla.org/seamonkey/source/mailnews/addrbook/src/nsDirectoryDataSource.cpp#100

Comment 16

[Mark Banner (:standard8)]

(In reply to comment #15)
> (From update of attachment 315306 [details] [diff] [review])
> I've checked in the tinderbox change:
> 
> http://mxr.mozilla.org/seamonkey/source/mailnews/addrbook/src/nsDirectoryDataSource.cpp#100
> 
Make that:
2008-04-13 05:34	bugzilla%standard8.plus.com 	mozilla/tools/tinderbox/build-seamonkey-util.pl 	1.386 	56/57 

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 314589 [details] [diff] [review]
[checked in] Remove old smoketest files

r=dmose

Comment 18

[Andrew Schultz]

Comment on attachment 315306 [details] [diff] [review]
[checked in] Tinderbox Changes v2 (correct normal patch)

>+    if ($bloat_prefix ne '') {
>+        $embed_prefix = $bloat_prefix." ".$embed_prefix;
>+        $leaks_testname = lc($bloat_prefix)."_".$leaks_testname;
>+        $maxheap_testname = lc($bloat_prefix)."_".$maxheap_testname;
>+        $allocs_testname = lc($bloat_prefix)."_".$allocs_testname;

I noticed that the filenames at the top of the method ($binary_log, etc) didn't get fixed up.  The raw numbers are OK, but for the mail test, it diffs against the browser logs.

BloatTest needs a similar fixup for bloat-cur.log and bloat-prev.log

Comment 19

[Mark Banner (:standard8)]

Attachment: Bloat Test Files v2

Use goQuitApplication() rather than window.close() so that mac exits the tests correctly.

Comment 20

[Mark Banner (:standard8)]

Attachment: Fix Log Comparing

(In reply to comment #18)
> (From update of attachment 315306 [details] [diff] [review])
> >+    if ($bloat_prefix ne '') {
> >+        $embed_prefix = $bloat_prefix." ".$embed_prefix;
> >+        $leaks_testname = lc($bloat_prefix)."_".$leaks_testname;
> >+        $maxheap_testname = lc($bloat_prefix)."_".$maxheap_testname;
> >+        $allocs_testname = lc($bloat_prefix)."_".$allocs_testname;
> 
> I noticed that the filenames at the top of the method ($binary_log, etc) didn't
> get fixed up.  The raw numbers are OK, but for the mail test, it diffs against
> the browser logs.
> 
> BloatTest needs a similar fixup for bloat-cur.log and bloat-prev.log
> 

Andrew, I think this fixes the problem. Could you check it please?

Comment 21

[Andrew Schultz]

Comment on attachment 316591 [details] [diff] [review]
Fix Log Comparing

>@@ -3625,13 +3626,14 @@ sub BloatTest2 {

this looks good.

BloatTest needs the same fixup for bloat-cur.log and bloat-prev.log

Comment 22

[Mark Banner (:standard8)]

Attachment: Fix Log Comparing v2

- Fixes both BloatTest and BloatTest2 functions to generate logs with the test prefix if applicable
- Quotes the directories in the system call in BloatTest2 in case they contain spaces.

Comment 23

[Mark Banner (:standard8)]

Attachment: [checked in]Fix Log Comparing v3

$binary_dir doesn't need quoting, if it did the rest of the patch would have to have $binary_dir quoted in various places.

Comment 24

[Mark Banner (:standard8)]

Comment on attachment 318594 [details] [diff] [review]
[checked in]Fix Log Comparing v3

This patch corrects the log file names so that BloatTest(2) for Mail and Browser have different names.

Also quotes profile directory for the MailBloatTest system call so that the call works platforms with spaces in their profile locations (i.e. mac).

Comment 25

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 318594 [details] [diff] [review]
[checked in]Fix Log Comparing v3

>Index: tools/tinderbox/build-seamonkey-util.pl
>===================================================================
>@@ -3356,8 +3360,8 @@ sub BloatTest {
...
>+    my $binary_log = "$build_dir/".$bloatdiff_label."bloat-cur.log";
>+    my $old_binary_log = "$build_dir/".$bloatdiff_label."bloat-prev.log";

Style nit, please use
      my $binary_log = "${build_dir}/${bloatdiff_label}bloat-cur.log";
or 
      my $binary_log = $build_dir . "/" . $bloatdiff_label . "bloat-cur.log";

for these two and the seven at the top of BloatTest2. Otherwise r+.

Comment 26

[Mark Banner (:standard8)]

Comment on attachment 318594 [details] [diff] [review]
[checked in]Fix Log Comparing v3

Checked in with styles fixed.

Comment 27

[Mark Banner (:standard8)]

Attachment: Bloat Test Files v3

This version moves the prefs file to a common directory and also extracts some parts of the python script to a common version as well. This will make re-use easier in other mailnews tests.

Comment 28

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 320526 [details] [diff] [review]
Bloat Test Files v3

Need to add yourself as a contributor to the various license boilerplates.
>+// Various delays to allow items to fully start/run

It would be nice to have the above comment mention the units of the delays as well as how they were chosen.

>+var btPrefs = Components.classes["@mozilla.org/preferences-service;1"]
>+                      .getService(Components.interfaces.nsIPrefBranch);

The indentation of the second line looks wrong.

>+// startBloatTest is the main function that hooks the state machines in. It is
>+// called via the "load" event listener for the window.
>+//
>+// We use the kStatePref to determine which state the tests are in.
>+//
>+// If kStatePref is not set, then we're the main mail window and are driving
>+// all the tests.
>+//
>+// If KStatePref is set to "startingAddressBook", then it is (hopefully) the
        ^
        case

>+function startBloatTest() 
>+{
>+  dump("Inside startBloatTest\n\n\n");
>+
>+  // Make sure to launch the test harness in the correct state.
>+  if (btPrefs.prefHasUserValue(kStatePref)) {
>+    dump("pref has user value\n\n\n");
>+    if (btPrefs.getCharPref(kStatePref) == "startingAddressBook")
>+      setTimeout("handleABState();", kABStartup);

Any particular reason to use a string that needs to be eval()ed here (and elsewhere) rather than just passing in the function object?

After a bunch of pondering, I'm thinking that this would be significantly more readable and maintainable if there were three pairs of XUL/JS file.  This would eliminate a whole lot of state machine gunk, and state machines are notoriously painful to debug.

Thoughts?

(Removing r? for now; please re-request when you add some thoughts in to my above question).

Comment 29

[Mark Banner (:standard8)]

Attachment: Bloat Test Files v4

Reworked files into three separate sets, added lots of comments and simplified.

Comment 30

[Mark Banner (:standard8)]

Attachment: [checked in] Bloat Test Files v5

Following a discussion on mozilla.dev.planning, it seems like the best option is to put the performance test files in mailnews/test/performance for the time being, and we can decide if we want to move them somewhere else once a decision has been made on where we are heading wrt cvs/hg and branches.

Comment 31

[Mark Banner (:standard8)]

Attachment: Additional change to tinderbox script

This change will also be required to the tinderbox scripts.

Comment 32

[Serge Gautherie (:sgautherie)]

Comment on attachment 323856 [details] [diff] [review]
[checked in] Bloat Test Files v5

Any ETA for review ?

Comment 33

[Serge Gautherie (:sgautherie)]

(In reply to comment #32)
> Any ETA for review ?

IIRC, dmose replied me that he would, but maybe not this week.

Comment 34

[Mark Banner (:standard8)]

Comment on attachment 323856 [details] [diff] [review]
[checked in] Bloat Test Files v5

Switching reviewers.

Comment 35

[Mark Banner (:standard8)]

Note for review: This is a very basic setup test. It gives us something that works, so that we can get tinderbox/buildbot going. Once we have got them running, then we can look at doing more detailed tests.

Comment 36

[[e-mail]]

Comment on attachment 323856 [details] [diff] [review]
[checked in] Bloat Test Files v5

This looks fine but here are some tweaks you could consider:

>+  // load gets called before we've finished displaying/really loading, so we
>+  // have to have a bit of a timeout to allow it to get to that stage.
Some mochitests apparently use focus events instead, does that help?

>+  // clear the timeout and close the window.
>+  clearTimeout(gCurrentTimeout);
I don't know whether this was copied from somewhere but clearing a timeout that has already fired doesn't have much effect ;-)

>+  // Call the correct close compose window function, this also skips the
>+  // are you sure you want to close it prompt.
>+  MsgComposeCloseWindow(true);
Well, so should window.close(), but I guess this depends on whether you want to stress the recycling code or not.

>+// The other two windows (compose + addrbook) are closed after 3 seconds,
>+// well close after 8 to allow a little extra time - if they get delayed, then
>+// well get a prompt for closing the compose window that we don't want.
But you shouldn't get a prompt for closing a fresh compose window...

>+function handleMainTest()
>+{
>+  // First thing to do is to start the address book and compose windows
>+  toOpenWindowByType("mail:addressbook",
>+                     "chrome://messenger/content/addressbook/addressbook.xul");
>+
>+  startComposeWindow();
I don't see why you can't do this in the load handler, and only quit later.

>+user_pref("browser.chrome.site_icons", false);
>+user_pref("browser.chrome.favicons", false);
Not necessary, surely? ;-)

>+user_pref("dom.allow_scripts_to_close_windows", true);
This is a chrome test, so again you don't need this.

>+user_pref("mail.identity.id1.useremail", "[e-mail]");
[e-mail] ;-)

Comment 37

[Mark Banner (:standard8)]

(In reply to comment #36)
> (From update of attachment 323856 [details] [diff] [review])
> This looks fine but here are some tweaks you could consider:
> 
> >+  // load gets called before we've finished displaying/really loading, so we
> >+  // have to have a bit of a timeout to allow it to get to that stage.
> Some mochitests apparently use focus events instead, does that help?

I tried this locally, and I had focus problems, I'd prefer not to get into those on Tinderboxes. Additionally, I'm also considering switching to Gristmill/MozMill at some stage, which would make this go away.

> >+// The other two windows (compose + addrbook) are closed after 3 seconds,
> >+// well close after 8 to allow a little extra time - if they get delayed, then
> >+// well get a prompt for closing the compose window that we don't want.
> But you shouldn't get a prompt for closing a fresh compose window...

Bug 321783

Comment 38

[Mark Banner (:standard8)]

Comment on attachment 323856 [details] [diff] [review]
[checked in] Bloat Test Files v5

Checked in, changeset id 486:1da06fe7ba02

Comment 39

[Mark Banner (:standard8)]

Now that we have the files in, and we'll no longer intend to use tinderbox to run them this bug is fixed.

I have filed bug 458351 to extend the tests over time. Bug 428398 will create the buildbot(s) for Thunderbird, and I presume bug 448137 will cover getting coverage on SeaMonkey.