629069 - Create endurance tests shared-module and initial test snippet

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

We need to create a new shared-module for the Endurance Tests project. This will  use the performance shared-module to gather metrics on the current resources whilst a test snippet is run a specified number of times. The number of iterations for a test snippet will be provided as a command line parameter to an associated command line wrapper.

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Shared-module and initial test snippet v1

This patch includes a shared-module for endurance tests, and an initial test snippet for demonstration purposes. The test snippet simple opens and closes the add-ons manager. There were also necessary changes for the performance shared-module to prevent it from storing the results in memory.

Comment 2

[u279076]

Comment on attachment 507190 [details] [diff] [review]
Shared-module and initial test snippet v1

Looks fine to me.

Comment 3

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 507190 [details] [diff] [review]
Shared-module and initial test snippet v1

Looks fine. Now that you're exposing a "checkpoint" object from the PerfTracer, the object interface should probably be documented somewhere since it's public.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 507190 [details] [diff] [review]
Shared-module and initial test snippet v1

>+var Endurance = require("../../shared-modules/endurance");
>+var Addons = require("../../shared-modules/addons");

nit: please sort alphabetical.

>+var setupModule = function(module) {

function setupModule() please

>+var teardownModule = function(module) {

Same here.

>+var testOpenAndCloseAddonManager = function() {

... and here.

>+  for(var i = 0; i < persisted.iterations; i++) {
>+    enduranceManager.startIteration(i);
>+    
>+    addonsManager.open();
>+    enduranceManager.addCheckpoint("Add-ons Manager open");
>+    addonsManager.close();
>+    enduranceManager.addCheckpoint("Add-ons Manager closed");
>+    
>+    enduranceManager.endIteration();
>+  }

Most of the code will be repeated in other tests. I believe the endurance module should completely take care of the iterations, and you simply pass in a callback to the function which contains the tests.

>+/**
>+ * Constructor
>+ */

nit: please add the parameter and @contstructor entry

>+function enduranceManager(controller) {

For new classes we should immediately start to use a capital first letter.

>+    startIteration : function endurance_startIteration(iteration) {

As said in the test, it should be enough to have one single function exposed which runs all iterations.

>+        this._iteration = iteration;
>+        this._controller.sleep(persisted.delay);

Please assign both persisted values to class properties in the constructor.

>+++ b/shared-modules/performance.js
>+  getCheckpoint : function PerfTracer_getCheckpoint(aLabel) {

I don't see why this change is necessary. You should also call addCheckpoint for startIteration and endIteration. Simply add a getter to the performance class which retrieves the _log array. You will always be able to access start/end via [0] or [length-1].

Dependent on my proposals for the automation script other changes would be necessary.

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Shared-module and initial test snippet v1.1

(In reply to comment #4)
> Comment on attachment 507190 [details] [diff] [review]
> Shared-module and initial test snippet v1
> 
> >+var Endurance = require("../../shared-modules/endurance");
> >+var Addons = require("../../shared-modules/addons");
> 
> nit: please sort alphabetical.

Done.

> >+var setupModule = function(module) {
> 
> function setupModule() please

Done.

> >+var teardownModule = function(module) {
> 
> Same here.

Done.

> >+var testOpenAndCloseAddonManager = function() {
> 
> ... and here.

Done.

> >+  for(var i = 0; i < persisted.iterations; i++) {
> >+    enduranceManager.startIteration(i);
> >+    
> >+    addonsManager.open();
> >+    enduranceManager.addCheckpoint("Add-ons Manager open");
> >+    addonsManager.close();
> >+    enduranceManager.addCheckpoint("Add-ons Manager closed");
> >+    
> >+    enduranceManager.endIteration();
> >+  }
> 
> Most of the code will be repeated in other tests. I believe the endurance
> module should completely take care of the iterations, and you simply pass in a
> callback to the function which contains the tests.

Sounds ideal. Do you have any example code that I can look at, as I honestly wouldn't know where to start with this...

> >+/**
> >+ * Constructor
> >+ */
> 
> nit: please add the parameter and @contstructor entry

Done.

> >+function enduranceManager(controller) {
> 
> For new classes we should immediately start to use a capital first letter.

Done.

> >+        this._iteration = iteration;
> >+        this._controller.sleep(persisted.delay);
> 
> Please assign both persisted values to class properties in the constructor.

Done for results and delay, but I'm not sure it currently makes sense to persist the number of iterations, as the shared-module doesn't use this. The value in the code above is the current iteration.

> >+++ b/shared-modules/performance.js
> >+  getCheckpoint : function PerfTracer_getCheckpoint(aLabel) {
> 
> I don't see why this change is necessary. You should also call addCheckpoint
> for startIteration and endIteration. Simply add a getter to the performance
> class which retrieves the _log array. You will always be able to access
> start/end via [0] or [length-1].

I did this originally, but wanted to keep start/end distinct from a test's 'checkpoints'. For now I have reverted back to your suggested implementation.

> Dependent on my proposals for the automation script other changes would be
> necessary.

Done.

Comment 6

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Geo: Thanks for your point on updating the PerfTracer documentation. Where can I find this?

Henrik: Is there a way I can store the endurance results to retain a relationship with the tests? Looking at the report content, I would either imagine including the index of the test in the endurance object, or somehow adding the endurance results beneath the test results. Having this association would allow me to build a chart for an entire testrun, as well as charts for individual tests.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #5)
> > Most of the code will be repeated in other tests. I believe the endurance
> > module should completely take care of the iterations, and you simply pass in a
> > callback to the function which contains the tests.
> 
> Sounds ideal. Do you have any example code that I can look at, as I honestly
> wouldn't know where to start with this...

Still needed after our 1-1 today? 

> > >+        this._iteration = iteration;
> > >+        this._controller.sleep(persisted.delay);
> > 
> > Please assign both persisted values to class properties in the constructor.
> 
> Done for results and delay, but I'm not sure it currently makes sense to
> persist the number of iterations, as the shared-module doesn't use this. The
> value in the code above is the current iteration.

As discussed today, it makes sense once the iteration is in the endurance class itself.

> > I don't see why this change is necessary. You should also call addCheckpoint
> > for startIteration and endIteration. Simply add a getter to the performance
> > class which retrieves the _log array. You will always be able to access
> > start/end via [0] or [length-1].
> 
> I did this originally, but wanted to keep start/end distinct from a test's
> 'checkpoints'. For now I have reverted back to your suggested implementation.

Having it separated will make code more complicated which is written to analyzes samples, i.e. you can't easily get the diffs for start/end with the other timestamps.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 508376 [details] [diff] [review]
Shared-module and initial test snippet v1.1

As talked, I will remove the request for now.

Comment 9

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Shared-module and initial test snippets v1.2

(In reply to comment #7)
> (In reply to comment #5)
> > > Most of the code will be repeated in other tests. I believe the endurance
> > > module should completely take care of the iterations, and you simply pass in a
> > > callback to the function which contains the tests.

Done.

> > > >+        this._iteration = iteration;
> > > >+        this._controller.sleep(persisted.delay);
> > > 
> > > Please assign both persisted values to class properties in the constructor.
> > 

Done.

> > > I don't see why this change is necessary. You should also call addCheckpoint
> > > for startIteration and endIteration. Simply add a getter to the performance
> > > class which retrieves the _log array. You will always be able to access
> > > start/end via [0] or [length-1].
> > 
> > I did this originally, but wanted to keep start/end distinct from a test's
> > 'checkpoints'. For now I have reverted back to your suggested implementation.
> 
> Having it separated will make code more complicated which is written to
> analyzes samples, i.e. you can't easily get the diffs for start/end with the
> other timestamps.

Done.

I have also added an argument for the test name as I was unable to get this programmatically. This is for the dashboard report so we can compare resources per test. Finally, I have added a short delay after each checkpoint to simulate user think-time. I'd like to get some feedback on this change.

Comment 10

[u279076]

Comment on attachment 509462 [details] [diff] [review]
Shared-module and initial test snippets v1.2

>+    run : function endurance_run(name, callback) {
>+      this._testResults = {"name" : name, "iterations" : [ ]};
>+      for (var i=0; i < this._iterations; i++) {

This is a nit, but please put a space between i, =, and 0.

r+ otherwise.

Comment 11

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Shared-module and initial test snippets v1.2.1

(In reply to comment #10)
> Comment on attachment 509462 [details] [diff] [review]
> Shared-module and initial test snippets v1.2
> 
> >+    run : function endurance_run(name, callback) {
> >+      this._testResults = {"name" : name, "iterations" : [ ]};
> >+      for (var i=0; i < this._iterations; i++) {
> 
> This is a nit, but please put a space between i, =, and 0.

Done.

> r+ otherwise.

Thanks.

Comment 12

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Shared-module and initial test snippets v1.3

Updated to get the test method name and filename from Mozmill.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 511205 [details] [diff] [review]
Shared-module and initial test snippets v1.3

>+++ b/firefox/enduranceTests/testEnterAndLeavePrivateBrowsingModeWithMultipleTabsOpen.js

Wow, we should shorten file names. This one no-one can overview. It would probably be a good idea to use sub folders.

>+function testEnterAndLeavePrivateBrowsingModeWithMutlipleTabsOpen() {

Test module and function should have different names. That will also reduce complexity.

>+  enduranceManager.run(function () {
>+    pb.start();
>+    enduranceManager.addCheckpoint("Entered private browsing mode");
>+    pb.stop();
>+    enduranceManager.addCheckpoint("Left private browsing mode");
>+  });

I wonder if we shouldn't magically use persisted in the endurance class but better pass in the values are parameters for the constructor. The benefit of it would be that someone could run the test more easily without the automation script, simply by running Mozmill. What do you think?

>+var pbStartHandler = function(controller) {
>+  // Check to not ask anymore for entering Private Browsing mode
>+  var checkbox = new elementslib.ID(controller.window.document, 'checkbox');
>+  controller.waitThenClick(checkbox);
>+
>+  var okButton = new elementslib.Lookup(controller.window.document, 
>+                                        '/id("commonDialog")' +
>+                                        '/anon({"anonid":"buttons"})' +
>+                                        '/{"dlgtype":"accept"}');
>+  controller.click(okButton);
>+}

You don't need the dialog. I would say disable the showPrompt property.

>+function setupModule() {
>+  controller = mozmill.getBrowserController();
>+  enduranceManager = new Endurance.EnduranceManager(controller);
>+  addonsManager = new Addons.addonsManager(controller);
>+  tabBrowser = new Tabs.tabBrowser(controller);
>+  tabBrowser.closeAllTabs();
>+}

Please add two empty lines after controller and tabBrowser to separate the different actions.

>+ * @param {MozMillController} controller Mozmill controller of FF-window.

Please use the description we have in other modules. I don't think it will always be the browser window.

>+    run : function endurance_run(callback) {
>+      this._testResults = {"testMethod" : frame.events.currentTest.__name__,
>+        "testFile" : frame.events.currentModule.__file__,
>+        "iterations" : [ ]};

For better readability please move the testMethod assignment also to its own line. Otherwise good to see that this is working.

>+      for (var i = 0; i < this._iterations; i++) {
>+        this._controller.sleep(this._delay);
>+        this._iterationResults = {"checkpoints" : new Array()};

You could use the perftracer's log property to collect the checkpoints. You would only have to reset it for each iteration.

>+        this.addCheckpoint("Start iteration");

Then this._perfTracer.addCheckpoint(label) sounds more obvious to me.

>+        this._testResults.iterations.push(this._iterationResults);

And finally you can construct the result object you want to push and use this._perfTracer._log (also I'm not sure why it has an underscore because it should be public accessible).

>+      }
>+      this._results.push(this._testResults);

Also please always add some blank lines between the different blocks. This loop is hard to overlook.

>+    addCheckpoint : function endurance_addCheckpoint(label) {
>+      this._iterationResults.checkpoints.push(this._perfTracer.getCheckpoint(label));
>+      this._controller.sleep(200);

I believe we don't need that method anymore, now we have the internal method which handles the iterations. Also why was the 200ms delay necessary?

Comment 14

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Shared-module and initial test snippets v1.4

(In reply to comment #13)
> Comment on attachment 511205 [details] [diff] [review]
> Shared-module and initial test snippets v1.3
> 
> >+++ b/firefox/enduranceTests/testEnterAndLeavePrivateBrowsingModeWithMultipleTabsOpen.js
> 
> Wow, we should shorten file names. This one no-one can overview. It would
> probably be a good idea to use sub folders.

Revised folder structure and file names. Please let me know if you had something else in mind. I do have a tendency to use long test names, as I personally think it's better to be descriptive than brief.

> >+function testEnterAndLeavePrivateBrowsingModeWithMutlipleTabsOpen() {
> 
> Test module and function should have different names. That will also reduce
> complexity.

Also revised. Feedback encouraged.

> >+  enduranceManager.run(function () {
> >+    pb.start();
> >+    enduranceManager.addCheckpoint("Entered private browsing mode");
> >+    pb.stop();
> >+    enduranceManager.addCheckpoint("Left private browsing mode");
> >+  });
> 
> I wonder if we shouldn't magically use persisted in the endurance class but
> better pass in the values are parameters for the constructor. The benefit of it
> would be that someone could run the test more easily without the automation
> script, simply by running Mozmill. What do you think?

I had expected that in the future we may want to override the automation script parameters within the test, whereas your suggestion sounds like the automation script take precedence. This is probably something worth discussing. For the time being I have set suitable defaults (0 delay, 1 iteration) if not running via the automation script.

> >+var pbStartHandler = function(controller) {
> >+  // Check to not ask anymore for entering Private Browsing mode
> >+  var checkbox = new elementslib.ID(controller.window.document, 'checkbox');
> >+  controller.waitThenClick(checkbox);
> >+
> >+  var okButton = new elementslib.Lookup(controller.window.document, 
> >+                                        '/id("commonDialog")' +
> >+                                        '/anon({"anonid":"buttons"})' +
> >+                                        '/{"dlgtype":"accept"}');
> >+  controller.click(okButton);
> >+}
> 
> You don't need the dialog. I would say disable the showPrompt property.

Agreed. Done.

> >+function setupModule() {
> >+  controller = mozmill.getBrowserController();
> >+  enduranceManager = new Endurance.EnduranceManager(controller);
> >+  addonsManager = new Addons.addonsManager(controller);
> >+  tabBrowser = new Tabs.tabBrowser(controller);
> >+  tabBrowser.closeAllTabs();
> >+}
> 
> Please add two empty lines after controller and tabBrowser to separate the
> different actions.

Done.

> >+ * @param {MozMillController} controller Mozmill controller of FF-window.
> 
> Please use the description we have in other modules. I don't think it will
> always be the browser window.

Done.

> >+    run : function endurance_run(callback) {
> >+      this._testResults = {"testMethod" : frame.events.currentTest.__name__,
> >+        "testFile" : frame.events.currentModule.__file__,
> >+        "iterations" : [ ]};
> 
> For better readability please move the testMethod assignment also to its own
> line. Otherwise good to see that this is working.

Done.

> >+      for (var i = 0; i < this._iterations; i++) {
> >+        this._controller.sleep(this._delay);
> >+        this._iterationResults = {"checkpoints" : new Array()};
> 
> You could use the perftracer's log property to collect the checkpoints. You
> would only have to reset it for each iteration.

Done. The reason this wasn't done initially was to prevent storing too much in memory, however I see that this is should be just as safe.

> >+        this.addCheckpoint("Start iteration");
> 
> Then this._perfTracer.addCheckpoint(label) sounds more obvious to me.

Done.

> >+        this._testResults.iterations.push(this._iterationResults);
> 
> And finally you can construct the result object you want to push and use
> this._perfTracer._log (also I'm not sure why it has an underscore because it
> should be public accessible).

Done.

> >+      }
> >+      this._results.push(this._testResults);
> 
> Also please always add some blank lines between the different blocks. This loop
> is hard to overlook.

Done.

> >+    addCheckpoint : function endurance_addCheckpoint(label) {
> >+      this._iterationResults.checkpoints.push(this._perfTracer.getCheckpoint(label));
> >+      this._controller.sleep(200);
> 
> I believe we don't need that method anymore, now we have the internal method
> which handles the iterations. Also why was the 200ms delay necessary?

This is still needed as it exposes the addCheckpoint method to the actual endurance test. The 200ms delay is currently there because we identified that running the automation without any delays actually causes memory usage to steadily increase, as this is unrealistic we added a parameterised delay between iterations and short delay after each checkpoint to more close simulate 'think time'. I'm not against removing this, but we should probably discuss.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #14)
> > >+      for (var i = 0; i < this._iterations; i++) {
> > >+        this._controller.sleep(this._delay);
> > >+        this._iterationResults = {"checkpoints" : new Array()};
> > 
> > You could use the perftracer's log property to collect the checkpoints. You
> > would only have to reset it for each iteration.
> 
> Done. The reason this wasn't done initially was to prevent storing too much in
> memory, however I see that this is should be just as safe.

Well, before you also pushed entries to an array which resisted in memory. The assignment to the persisted object happens after each iteration.

> This is still needed as it exposes the addCheckpoint method to the actual
> endurance test. The 200ms delay is currently there because we identified that
> running the automation without any delays actually causes memory usage to
> steadily increase, as this is unrealistic we added a parameterised delay
> between iterations and short delay after each checkpoint to more close simulate
> 'think time'. I'm not against removing this, but we should probably discuss.

I would better like it if it would be a parameter for the command line or if we could even merge it with the already specified delay value via the command line. Not sure if we really need two separate delay values. More to come in my review now.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 511726 [details] [diff] [review]
Shared-module and initial test snippets v1.4

>+  // Make sure we are not in PB mode and do not show a prompt
>+  pb.enabled = false;
>+  pb.showPrompt = false;

Would be better to have that in setupModule instead of the test itself.

>+  LOCAL_TEST_PAGES.forEach(function(url) {

nit: Place a space between function and the opening bracket.

>+  enduranceManager.run(function () {
>+    pb.start();
>+    enduranceManager.addCheckpoint("Entered private browsing mode");
>+    pb.stop();
>+    enduranceManager.addCheckpoint("Left private browsing mode");
>+  });

Out of interest I would also be interested in if we have variances for starting/stopping the pb mode. Think about adding two more checkpoints?

>+EnduranceManager.prototype = {
>+    
>+    /**
>+     * Run endurance test
>+     *
>+     * @param {function} callback
>+     *        Callback function to call
>+     */
>+    run : function endurance_run(callback) {

Your indentation in the module has been mixed up. Please move back by 2 spaces.

>+      this._testResults = {
>+        "testMethod" : frame.events.currentTest.__name__,
>+        "testFile" : frame.events.currentModule.__file__,
>+        "iterations" : [ ]};

nit: for this block it would be helpful to have the closing bracket in the new line, like what we have for for loops. Oh and you can remove the quotes around the element names. Those are not necessary.

>+      for (var i = 0; i < this._iterations; i++) {
>+        this._controller.sleep(this._delay);

So we already have a sleep here?

>+        this._perfTracer.addCheckpoint("End iteration");
>+        this._iterationResults = {"checkpoints" : this._perfTracer._log};
>+        this._perfTracer.clearLog();
>
>+        this._testResults.iterations.push(this._iterationResults);

You can directly push the checkpoints. No need to store it in a separate variable first. Just call clearLog after the push command.

>+    addCheckpoint : function endurance_addCheckpoint(label) {
>+      this._perfTracer.addCheckpoint(label);
>+      this._controller.sleep(200);
>+    },

So you have said we need a timeout during each iteration. I can see that above in the for loop. Right now I don't understand this 200ms. If we really want to do an endurance tests to measure the timing we will break our results. Isn't this._delay enough?

Otherwise it looks pretty solid. I think one more round and then we can get this beast landed!

Comment 17

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Shared-module and initial test snippets v1.5

(In reply to comment #16)
> Comment on attachment 511726 [details] [diff] [review]
> Shared-module and initial test snippets v1.4
> 
> >+  // Make sure we are not in PB mode and do not show a prompt
> >+  pb.enabled = false;
> >+  pb.showPrompt = false;
> 
> Would be better to have that in setupModule instead of the test itself.

Agreed. Done.

> >+  LOCAL_TEST_PAGES.forEach(function(url) {
> 
> nit: Place a space between function and the opening bracket.

Done.

> >+  enduranceManager.run(function () {
> >+    pb.start();
> >+    enduranceManager.addCheckpoint("Entered private browsing mode");
> >+    pb.stop();
> >+    enduranceManager.addCheckpoint("Left private browsing mode");
> >+  });
> 
> Out of interest I would also be interested in if we have variances for
> starting/stopping the pb mode. Think about adding two more checkpoints?

As discussed on IRC this is worth noting for future endurance tests.
 
> >+EnduranceManager.prototype = {
> >+    
> >+    /**
> >+     * Run endurance test
> >+     *
> >+     * @param {function} callback
> >+     *        Callback function to call
> >+     */
> >+    run : function endurance_run(callback) {
> 
> Your indentation in the module has been mixed up. Please move back by 2 spaces.

Done.

> >+      this._testResults = {
> >+        "testMethod" : frame.events.currentTest.__name__,
> >+        "testFile" : frame.events.currentModule.__file__,
> >+        "iterations" : [ ]};
> 
> nit: for this block it would be helpful to have the closing bracket in the new
> line, like what we have for for loops. Oh and you can remove the quotes around
> the element names. Those are not necessary.

Done.

> >+        this._perfTracer.addCheckpoint("End iteration");
> >+        this._iterationResults = {"checkpoints" : this._perfTracer._log};
> >+        this._perfTracer.clearLog();
> >
> >+        this._testResults.iterations.push(this._iterationResults);
> 
> You can directly push the checkpoints. No need to store it in a separate
> variable first. Just call clearLog after the push command.

Done.

> >+    addCheckpoint : function endurance_addCheckpoint(label) {
> >+      this._perfTracer.addCheckpoint(label);
> >+      this._controller.sleep(200);
> >+    },
> 
> So you have said we need a timeout during each iteration. I can see that above
> in the for loop. Right now I don't understand this 200ms. If we really want to
> do an endurance tests to measure the timing we will break our results. Isn't
> this._delay enough?

Removed this delay for now as it's causing some confusion. It was basically an additional small delay to closer simulate a user. It can be readdressed in a future iteration if needed.

Comment 18

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Shared-module and initial test snippets v1.5.1

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 512304 [details] [diff] [review]
Shared-module and initial test snippets v1.5.1

Looks good. Lets get it landed!

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Landed on default only:
http://hg.mozilla.org/qa/mozmill-tests/rev/25f5f895535d

There are no requirements to get it landed on older branches. We could think of backports later if needed.

Dave, can you only create a dummy test for older branches? Similar as what we have for l10n tests on 1.9.1? That would be necessary to not break our test-run with automation scripts. I will leave this bug open for now.

Comment 21

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Dummy endurance test for mozilla1.9.2 branch v1.0

Comment 22

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Dummy endurance test for mozilla1.9.1 branch v1.0

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Dummy tests have been landed on older branches:
http://hg.mozilla.org/qa/mozmill-tests/rev/76ecfe94a265 (1.9.2)
http://hg.mozilla.org/qa/mozmill-tests/rev/1ea0d2b4822a (1.9.1)