458352 - Integrate Mail Bloat Tests with MozMill

Status: RESOLVED WONTFIX

(MailNews Core :: Testing Infrastructure, defect)

Description

[Mark Banner (:standard8)]

As I mentioned in my blog (see url above), integrating MozMill into the mail bloat tests would be very useful and make development and maintenance of the tests much easier, as well as providing some useful testing coverage. My blog already has one test script.

Currently this is blocked by bug 457265 which will hook MozMill into the automation system. I still wanted to raise it though - if you're interested in helping with this, please also consider if you can help with that bug as well.

Comment 1

[Mark Banner (:standard8)]

Attachment: WIP

Work in progress patch.

This patch moves bloat tests to using MozMill to drive the tests rather than the existing bloat* files (I'm hoping some of this may eventually help sort out some of our timing problems).

At the moment it only runs the Lk part of the test. I'm having some issues with clean shutdown (i.e. it doesn't work) that I'm going to ask the mozmill guys about.

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

Attachment: andrew's mozmill driver as it exists, based on standard8's

This patch is mainly interesting because the driver code is more aligned with the mozmill idiom than standard8's WIP ported driver.  This works with mozmill/jsbridge/mozrunner trunk and is extremely biased towards my use-case, running the driver program from a directory of mozmill tests in my source directory on linux. (The logic tries to figure out the right objdir and what not.)

Besides the dubious path detection, the main thing this adds over vanilla trunk mozmill (and is ported from standard8's driver) is the use of -CreateProfile rather than mozmills' "borrow a profile" and overlay some settings thing.  Arguably mozmill is almost close enough to duplicate what CreateProfile does, but it seems like using CreateProfile is the most realistic approach.

I'm posting this just because it seems like everyone has a nearly working Thunderbird/mozmill solution, and I figured I'd share mine too.

Comment 3

[Mark Banner (:standard8)]

Attachment: WIP v2

This patch is a totally revised one based on Andrew's idea. It needs current trunk versions of mozmill/jsbridge/mozrunner and bug 487385 and bug 487391 to run. What it currently does (including some of Andrew's features):

- Provides capability for -CreateProfile and using the existing prefs file to specify the prefs
- Automatic discovery of the binary/src dir
- No leak/bloat output
- Cleanly shut down Thunderbird
- Running "python runtest.py -t test_mozmillbloat.js" will run and complete successfully.

What I'd like it to do:

1) Put the scripts in mail/test/mozmill like Andrew's patch

I think we're at the stage where if we have mozmill, we're going to do UI testing that warrants separation from SeaMonkey, plus the mozmill run script is now getting into app specifics.

2a) Take the runtest.py make it so that it can run any generic test with minimal set of default prefs (its unclear if we'll need an account set up or not.

2b) Add a wrapper file/class around runtest.py so that bloat tests can be run automatically.

This step may need two wrappers - I'd like to see one setting XPCOM_MEM_BLOAT_LOG argument (via an extra arg on the command line), and the second passing the --trace-malloc and --shutdown-leaks arguments from the command line to the app.

The documentation here may be useful for this step:
http://mozrunner.googlecode.com/svn/trunk/docs/_build/html/index.html

3) Separate out ThunderTestProfile.preferences into their own file that gets imported as needed (replacing mailnewsTestPrefs.js) - maybe basing on the test directory.

This means we can have multiple sets of preferences for different types of tests.

So what we'd end up with:

mail/test/mozmill/bloat/ - Test scripts for bloat
mail/test/mozmill/<other>/ - Test scripts for other specific items

These directories would contain test_*.js files plus potentially one preferences.py file that can be imported, containing the set of preferences to use for those tests.

mail/test/mozmill/runtest.py - Generic run script, call with -t <folder> to run with set of tests
mail/test/mozmill/runrefcnt.py - Specific run script (wrapper around runtest.py) for reference count tests (RLk)
mail/test/mozmill/runbloattest.py - Specific run script (again a wrapper) for leak/heap/allocations tests.


If anyone could pick up some or all of this that would be really useful. It doesn't have to be implemented all at the same time, just getting item 1 (and maybe 2a) done would be useful.

Comment 4

[Robert Kaiser]

I think we should end up with a generic test harness (run scripts etc.) in mozilla-central, so that all applications can use it (just like with the other test harnesses). The tests depend on the UI of the app, so looks good if they are in mail/ or the like.

Comment 5

[Mark Banner (:standard8)]

(In reply to comment #4)
> I think we should end up with a generic test harness (run scripts etc.) in
> mozilla-central, so that all applications can use it (just like with the other
> test harnesses). The tests depend on the UI of the app, so looks good if they
> are in mail/ or the like.

As an end goal, I agree. Given FF doesn't seem to be doing anything in the MozMill in mozilla-central area I'd rather drive this from Thunderbird area in the short term (as we really need some of these test capabilities), and dealing with making the scripts application generic adds another level of complexity that I think we don't want at the moment.

Comment 6

[Ludovic Hirlimann [:Usul]]

Attachment: Move the bloat tests at the right place

Step 1) from Standard8's multi-steps

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 373645 [details] [diff] [review]
Move the bloat tests at the right place

I guess I didn't quite explain my part 1 well enough :-(

>diff -r e566900c420a mail/test/mozmill/bloat/bloatAddrOverlay.js
>diff -r e566900c420a mail/test/mozmill/bloat/bloatAddrOverlay.xul
>diff -r e566900c420a mail/test/mozmill/bloat/bloatComposeOverlay.js
>diff -r e566900c420a mail/test/mozmill/bloat/bloatComposeOverlay.xul
>diff -r e566900c420a mail/test/mozmill/bloat/bloatMainOverlay.js
>diff -r e566900c420a mail/test/mozmill/bloat/bloatMainOverlay.xul
>diff -r e566900c420a mail/test/mozmill/bloat/runtest.pl

Don't copy these, we don't need them.

>diff -r e566900c420a mail/test/mozmill/bloat/runtest.py

This looks like its mailnews/test/performance/bloat/runtest.py with my patch applied - that's good it is what we want.

You also need mailnews/test/performance/bloat/test_mozmillbloat.js from my patch.

I'm actually thinking we should make these two files to be located as follows:

mail/test/mozmill/runbloattest.py
mail/test/mozmill/bloat/test_openWindows.js

(note: I'm suggesting the bloat dir here, because then we can spread different sections of the bloat tests over different files and just pass "-t bloat" as a test directory argument for mozmill).

>diff -r e566900c420a mail/test/mozmill/bloat/setUpBloatTest.py

I'm pretty sure we don't need this in the new world.

>diff -r e566900c420a mail/test/mozmill/content-policy/test-msg-content-policy.js
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/mail/test/mozmill/content-policy/test-msg-content-policy.js	Mon Apr 20 16:02:20 2009 +0200
...
>diff -r e566900c420a mail/test/mozmill/test-msg-content-policy.js
>--- a/mail/test/mozmill/test-msg-content-policy.js	Mon Apr 20 03:37:04 2009 -0700
>+++ /dev/null	Thu Jan 01 00:00:00 1970 +0000

This looks like a cp with a hg remove and hg add. hg rename would be much better as that preserves history.

Comment 8

[Ludovic Hirlimann [:Usul]]

Attachment: Reading Mark instructions

I could not copy the test_mozmillbloat.js to mail/test/mozmill/bloat because it needs to be added to the repo before being able to be copied.

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 373802 [details] [diff] [review]
Reading Mark instructions

>diff -r e566900c420a mail/test/mozmill/content-policy/test-msg-content-policy.js
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/mail/test/mozmill/content-policy/test-msg-content-policy.js	Tue Apr 21 09:41:10 2009 +0200

>diff -r e566900c420a mail/test/mozmill/test-msg-content-policy.js
>--- a/mail/test/mozmill/test-msg-content-policy.js	Mon Apr 20 03:37:04 2009 -0700
>+++ /dev/null	Thu Jan 01 00:00:00 1970 +0000

I was just thinking these still don't look like a rename, then I realised - you need to turn on git style patches, then it will come out something like "Renamed foo to Bar".

https://developer.mozilla.org/en/Mercurial_FAQ#Configuration

>diff -r e566900c420a mailnews/test/performance/bloat/runtest.py
>diff -r e566900c420a mailnews/test/performance/bloat/test_mozmillbloat.js

Drop these changes - we'll sort out that directory later.

Comment 10

[Ludovic Hirlimann [:Usul]]

Attachment: move file v3

Comment 11

[Ludovic Hirlimann [:Usul]]

Attachment: with runtest.py modifications

Comment 12

[Mark Banner (:standard8)]

(In reply to comment #11)
> Created an attachment (id=376908) [details]
> with runtest.py modifications

We can drop the whole of the ThunderMozMill class - we don't need it now, that was fixed in a different way.

Do we still need to set the default_prefs_path to mailnewsTestPrefs.js now that we've got the prefs set up in ThunderTestProfile.preferences?

At some point we need to separate those out into separate files, but it'll do for starters to get this into hg.

Comment 13

[Ludovic Hirlimann [:Usul]]

(In reply to comment #12)
> (In reply to comment #11)
> > Created an attachment (id=376908) [details] [details]
> > with runtest.py modifications
> 
> We can drop the whole of the ThunderMozMill class - we don't need it now, that
> was fixed in a different way.

Will need to fix that.
 
> Do we still need to set the default_prefs_path to mailnewsTestPrefs.js now that
> we've got the prefs set up in ThunderTestProfile.preferences?

Probably just forgot to remove it.

> At some point we need to separate those out into separate files, but it'll do
> for starters to get this into hg.

Comment 14

[Ludovic Hirlimann [:Usul]]

Attachment: taking comments into account + using new Thunderbird classes

Mark sorry for the time you are wasting. I'm still new in python. Taking your comments into account and using the newly defined Thunderbird classes.

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 377144 [details] [diff] [review]
taking comments into account + using new Thunderbird classes

No need to worry about wasting my time, I'm getting something developed and I don't have to spend lots of time on it ;-)

This patch is a mixture of code from Andrew, Ludovic and myself, rather than get another person involved (which wouldn't have much benefit, plus its test-only code), I'm going to review it.

>+# Contributor(s):
>+#   Mark Banner <[e-mail]>

Lets add Andrew and yourself here.

>+#import optparse

Just drop that line for now if we don't need it.

>+class ThunderTestProfile(mozrunner.ThunderbirdProfile):
>+    preferences = {
...
>+        'extensions.palmsync.conduitRegistered': True,
>+        'browser.dom.window.dump.enabled' :  True,
>+		'dom.max_chrome_script_run_time' :  200,
>+		'dom.max_script_run_time' :  0,

Can we change these tabbed lines (and the ones below them) to spaces only please.

>+        self.base_test_dir = os.getcwd()
>+       

nit: whitespace on blank line.

>+            mozconfig_path = os.path.join(self.src_dir, '.mozconfig.mk')
>+        
>+        guess_path = os.path.join(self.src_dir, 'mozilla/build/autoconf/config.guess')

nit: whitespace on blank line.

>+class ThunderTestRunner(mozrunner.ThunderbirdRunner):
>+    profile_class = ThunderTestProfile
>+   

nit: ditto

>+class ThunderTestCLI(mozmill.CLI):
>+    profile_class = ThunderTestProfile
>+    runner_class = ThunderTestRunner
>+    def stop_runner(self):
>+    	self.runner.stop()

we don't need to define stop_runner, we can just drop that function.

I think with those fixed we can land this as a first WIP version and then follow up with more patches. This does work for running test with pre-defined profile within a build environment so its a good start.

r=Standard8

What I'd like to see going forward is:

- Facility to load in different profiles, from the test directory (should hopefully be able to do that via an import mechanism) or fallback to "default" set.
- Wrappers for the refcnt & leak tests.

That would bring us forward to being able to fully automate the bloat tests using mozmill, as well as giving us a generic script that we can run in a build environment.

I'd like to think about seeing if we can hook these scripts into the make package-tests mechanism as I think this could potentially be useful testing mozmill against release builds but I'll deal with that in a different bug.

Comment 16

[Ludovic Hirlimann [:Usul]]

Attachment: [checked in] patch addressing review comments

Here you go.

Comment 17

[Mark Banner (:standard8)]

Comment on attachment 377152 [details] [diff] [review]
[checked in] patch addressing review comments

This is now checked in:

http://hg.mozilla.org/comm-central/rev/fa041768f27d

The script can be run in a build environment by ensuring MOZCONFIG is set in the environment, and then going into <srcdir>/mail/test/mozmill and doing:

python runtest.py -t bloat

It won't work for the test-msg-content-policy.js at the moment because that requires a different profile set up.

There also won't be any bloat log outputs - that what we need the wrapper scripts for.

Comment 18

[Ludovic Hirlimann [:Usul]]

Taking some notes for later :
Standard9: so for instance, I would be able to run the script in one of the following two ways:
[2:31pm] Standard9: runbloattest.py --test="bloat"
[2:32pm] Standard9: runbloattest.py --test="leak"
[2:32pm] _Tsk_: ok
[2:32pm] Standard9: although it may be better to rename those "refcnt" and "malloc" respecitively
http://mxr.mozilla.org/comm-central/source/mailnews/test/performance/bloat/runtest.py#134

Comment 19

[Mark Banner (:standard8)]

(In reply to comment #18)
> Taking some notes for later :
> Standard9: so for instance, I would be able to run the script in one of the
> following two ways:
> [2:31pm] Standard9: runbloattest.py --test="bloat"
> [2:32pm] Standard9: runbloattest.py --test="leak"
> [2:32pm] Standard9: although it may be better to rename those "refcnt" and
> "malloc" respecitively
> http://mxr.mozilla.org/comm-central/source/mailnews/test/performance/bloat/runtest.py#134

Actually, having developed that thought, I think it might be better to do this all within runtest.py. So we provide two new (exclusive, I think) arguments:

--refcnt
    Runs the selected test(s) with XPCOM_MEM_BLOAT_LOG logging enabled.
--trace-malloc
    Runs the selected test(s) with --trace-malloc logging and generates trace-malloc and shutdown-leak logs (requires --enable-trace-malloc in build configuration).

This way, we can run any mozmill test with bloat & leak logging which may be useful in the future for debugging and/or automated testing.

Comment 20

[Ludovic Hirlimann [:Usul]]

Attachment: WIP adding support for profiles

If a test needs a specific profile then the directory containing the test should contain a .profile file with one name in it.

That profile would be used from mozmill/profiles/name

Comment 21

[Mark Banner (:standard8)]

Comment on attachment 384393 [details] [diff] [review]
WIP adding support for profiles

Good idea I think, however there's a few problems I see with this patch:

- I don't like the use of dot files as they get hidden in various circumstances. I'd prefer just a file called "profile", afaik mozmill looks for test_ so we won't interfere with that.

- If you provide a profile file, then the script will use the directory of the specified profile as the profile - we shouldn't be doing that, we should always create a new profile, leaving the template one untouched.

- The mozmill & jsbridge extensions aren't getting installed in the profile.

>+            dotprofile= curdir + '/' + self.options.test + ".profile"

This line doesn't work right, you're potentially missing adding a '/' after self.options.test, and it is generally better to use the cross-platform os.path.join function for building path names.

Comment 22

[Ludovic Hirlimann [:Usul]]

Attachment: Adding profile support

Addressing standard8's comments.

Comment 23

[Mark Banner (:standard8)]

Comment on attachment 387162 [details] [diff] [review]
Adding profile support

I've not tested this yet, but it is looking better - I'll test when I get the next version.

>+    def get_profile(self, default_profile=None, profile=None, create_new=None, plugins=[],
>+                    preferences={}):
>+        """Returns the profile instance for the given command line arguments."""    
>+        profile = ThunderTestProfile(default_profile, profile, create_new, plugins, preferences)
>+        profile.install_plugin(mozmill.extension_path)
>+        profile.install_plugin(jsbridge.extension_path)

As per Mikeal's comment on the mozmill list, we should just use super() here.
>+        return profile
>+        
>+    def get_runner(self, binary=None, profile=None):
>+        """Returns the runner instance for the given command line binary arguemt

nit: spelling mistake.

>+            dotprofile=os.path.join( curdir ,self.options.test, "profile")

nit: no space after ( and spaces either side of = please (several places).

Should probably rename dotprofile as well.

Comment 24

[Ludovic Hirlimann [:Usul]]

Attachment: Patch with comment addressed

sorry for being so sluggish, and bothering you all the time.

Comment 25

[Mark Banner (:standard8)]

Comment on attachment 387440 [details] [diff] [review]
Patch with comment addressed

>+                workingprofile = os.path.join(curdir,"work_profile")
>+                if os.path.exists(workingprofile):
>+                    shutil.rmtree(workingprofile)
>+                shutil.copytree(profilename, workingprofile, False)
>+                crea_new = False
>+                prof = os.path.join(workingprofile, nameinfile)

I had a couple of issues with Thunderbird wanting to use a different work profile to the created one so I reordered the profile a bit.

As I've already done that, I'll attach a patch with those fixes in.

r=Standard8, thanks for driving this through.

Comment 26

[Mark Banner (:standard8)]

Attachment: [checked in] Allow pre-existing profile data

Updated patch that has been checked in: http://hg.mozilla.org/comm-central/rev/349adc106287

Comment 27

[Joshua Cranmer [:jcranmer]]

Bloat tests cease to exist now.