Closed Bug 427980 Opened 12 years ago Closed 12 years ago

Re-implement the MailBloatTest on tinderbox

Categories

(Webtools Graveyard :: Tinderbox, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: standard8, Assigned: standard8)

References

()

Details

Attachments

(5 files, 10 obsolete files)

16.13 KB, patch
dmose
: review+
Details | Diff | Splinter Review
6.53 KB, patch
rhelmer
: review+
Details | Diff | Splinter Review
7.76 KB, patch
Details | Diff | Splinter Review
3.04 KB, patch
nthomas
: review+
Details | Diff | Splinter Review
25.56 KB, patch
neil
: review+
Details | Diff | Splinter Review
Attached patch Tinderbox Changes (obsolete) — Splinter Review
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.
Attached patch Bloat Test Chrome Files (obsolete) — Splinter Review
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.
This patch will remove the old smoketest files from mailnews/extensions that are no longer needed.
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.
Any particular reason not to do the compose window as well?
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.
Attached patch Bloat Test Files (obsolete) — Splinter Review
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.
Attachment #314588 - Attachment is obsolete: true
Attachment #314831 - Flags: review?(dmose)
Attachment #314589 - Flags: review?(dmose)
Blocks: 428398
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 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.
Attachment #314832 - Flags: review?(robert)
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?
(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.
Ok, this is the correct one.
Attachment #314833 - Attachment is obsolete: true
Comment on attachment 314832 [details] [diff] [review]
[normal patch checked in] Tinderbox Changes v2 (diff -w)

Ah, ok.
Attachment #314832 - Flags: review?(robert) → review+
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
Attachment #315306 - Attachment description: Tinderbox Changes v2 (correct normal patch) → [checked in] Tinderbox Changes v2 (correct normal patch)
Attachment #314832 - Attachment description: Tinderbox Changes v2 (diff -w) → [normal patch checked in] Tinderbox Changes v2 (diff -w)
(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 on attachment 314589 [details] [diff] [review]
[checked in] Remove old smoketest files

r=dmose
Attachment #314589 - Flags: review?(dmose) → review+
Attachment #314589 - Attachment description: Remove old smoketest files → [checked in] Remove old smoketest files
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
Attached patch Bloat Test Files v2 (obsolete) — Splinter Review
Use goQuitApplication() rather than window.close() so that mac exits the tests correctly.
Attachment #314831 - Attachment is obsolete: true
Attachment #316589 - Flags: review?(dmose)
Attachment #314831 - Flags: review?(dmose)
Attached patch Fix Log Comparing (obsolete) — Splinter Review
(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 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
Attached patch Fix Log Comparing v2 (obsolete) — Splinter Review
- 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.
Attachment #316591 - Attachment is obsolete: true
$binary_dir doesn't need quoting, if it did the rest of the patch would have to have $binary_dir quoted in various places.
Attachment #318586 - Attachment is obsolete: true
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).
Attachment #318594 - Flags: review?(nrthomas)
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+.
Attachment #318594 - Flags: review?(nrthomas) → review+
Comment on attachment 318594 [details] [diff] [review]
[checked in]Fix Log Comparing v3

Checked in with styles fixed.
Attachment #318594 - Attachment description: Fix Log Comparing v3 → [checked in]Fix Log Comparing v3
Attached patch Bloat Test Files v3 (obsolete) — Splinter Review
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.
Attachment #316589 - Attachment is obsolete: true
Attachment #316589 - Flags: review?(dmose)
Attachment #320526 - Flags: review?(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).
Attachment #320526 - Flags: review?(dmose)
Attached patch Bloat Test Files v4 (obsolete) — Splinter Review
Reworked files into three separate sets, added lots of comments and simplified.
Attachment #320526 - Attachment is obsolete: true
Attachment #322103 - Flags: review?(dmose)
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.
Attachment #322103 - Attachment is obsolete: true
Attachment #323856 - Flags: review?(dmose)
Attachment #322103 - Flags: review?(dmose)
This change will also be required to the tinderbox scripts.
Priority: -- → P1
Depends on: 448137
Blocks: 448137
No longer depends on: 448137
Comment on attachment 323856 [details] [diff] [review]
[checked in] Bloat Test Files v5

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

IIRC, dmose replied me that he would, but maybe not this week.
Comment on attachment 323856 [details] [diff] [review]
[checked in] Bloat Test Files v5

Switching reviewers.
Attachment #323856 - Flags: review?(dmose) → review?(bienvenu)
Attachment #323856 - Flags: review?(bienvenu) → review?(neil)
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 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", "tinderbox@invalid.com");
tinderbox@mozilla.org ;-)
Attachment #323856 - Flags: review?(neil) → review+
(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 on attachment 323856 [details] [diff] [review]
[checked in] Bloat Test Files v5

Checked in, changeset id 486:1da06fe7ba02
Attachment #323856 - Attachment description: Bloat Test Files v5 → [checked in] Bloat Test Files v5
Attachment #323857 - Attachment is obsolete: true
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.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Product: Webtools → Webtools Graveyard
You need to log in before you can comment on or make changes to this bug.