Closed Bug 677626 Opened 8 years ago Closed 8 years ago

Add a profiling test suite for mochitest

Categories

(Testing :: Mochitest, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla9

People

(Reporter: mdas, Assigned: mdas)

References

(Depends on 1 open bug)

Details

(Whiteboard: [buildfaster:p1][inbound])

Attachments

(3 files, 5 obsolete files)

We don't have a static set of tests that profile our harness or tools that we use in mochitest. We should add these to make sure any changes to mochitest moving forward will not affect performance
Assignee: nobody → mdas
Whiteboard: [buildfaster:p1]
Attached patch added and modified tests (obsolete) — Splinter Review
I've added new sanity tests for our mochitest tools, and merged test_synthesizeDrop.xul and test_synthesizeDragStart.xul into a new test, test_sanityEventUtilsChrome.xul, which tests the chrome facing parts of EventUtils. I was planning on moving those chrome only functions from EventUtils.js to ChromeUtils.js, but it would take more than just cutting the functions out and copying them into ChromeUtils; I'd have to add ChromeUtils to the scope of the browser tests and it wasn't falling in the scope of this bug, so I'll be reopening Bug 666654 and will work from there.

I also added log lines to some of the existing tests. They will be parsed to get the load and run time data. This data will be submitted to autolog.

This patch has passed on try.
Attachment #557233 - Flags: review?(jmaher)
moved synthesizeDrop and synthesizeDragStart to ChromeUtils.js, and updated chrome/browser-chrome tests to reflect the change. 

The browser-chrome harness adds EventUtils.js to the scope of the functions as a variable. As a result, browser tests that want to use a function in EventUtils will call a function like "EventUtils.synthesizeMouse()" instead of "synthesizeMouse()" as is done in the other harnesses. The two functions I'm adding to ChromeUtils expects "synthesizeMouse()" and other EventUtils functions to be in scope, so I allowed synthesize[Drop|DragStart] to accept an optional parameter, eventUtils, to define these methods if passed in. This way, the browser tests can call "synthesizeDrop( ...,.., EventUtils)" and the needed functions will be available.
Attachment #557417 - Flags: review?(jmaher)
Attached file added and modified tests (obsolete) —
This patch should be applied after the chrome functions patch.
Attachment #557233 - Attachment is obsolete: true
Attachment #557233 - Flags: review?(jmaher)
Attachment #557419 - Flags: review?(jmaher)
Attached patch added and modified tests (obsolete) — Splinter Review
This patch should be applied after the chrome functions patch.

reposting since I didn't hit the 'patch' button.
Attachment #557419 - Attachment is obsolete: true
Attachment #557419 - Flags: review?(jmaher)
Attachment #557420 - Flags: review?
Comment on attachment 557417 [details] [diff] [review]
move two chrome functions out of EventUtils.js and into ChromeUtils.js

Review of attachment 557417 [details] [diff] [review]:
-----------------------------------------------------------------

hmm, I don't like the browser_*.js files loading a subscript for chromeutils.js.  Can we do that inside of browser-test.js:Tester() where we setup the scope for the test?

Also lets look at removing the EventUtils.js if it isn't needed.

::: testing/mochitest/tests/SimpleTest/ChromeUtils.js
@@ +223,5 @@
> +    synthesizeMouse = eventUtils.synthesizeMouse;
> +  }
> +
> +  // For events to trigger the UA's default actions they need to be "trusted".
> +  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");

we don't need this in chrome land.

::: toolkit/mozapps/downloads/tests/chrome/test_bug_462172.xul
@@ +63,5 @@
>            src="utils.js"/>
>    <script type="application/javascript"
>            src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"></script>
> +  <script type="application/javascript"
> +          src="chrome://mochikit/content/tests/SimpleTest/ChromeUtils.js"></script>

do we need eventutils.js anymore?
Attachment #557417 - Flags: review?(jmaher) → review-
another piece of feedback here is that we are adding these awesome unit/sanity tests for the harness and utilities, but are these intended to be run during regular tests?  Will there be a separate test run to run these benchmarked?  Will we do regular, chrome, and browser-chrome runs?
(In reply to Joel Maher (:jmaher) from comment #5)
> Comment on attachment 557417 [details] [diff] [review]
> move two chrome functions out of EventUtils.js and into ChromeUtils.js
> 
> Review of attachment 557417 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> hmm, I don't like the browser_*.js files loading a subscript for
> chromeutils.js.  Can we do that inside of browser-test.js:Tester() where we
> setup the scope for the test?

We can do this, but I opted not to since only a few tests required ChromeUtils.js and I didn't want to add it to the scope of all the tests if it wasn't needed, but since the ChromeUtils.js doesn't require much load time (0-1ms), we can do this without seeing a performance problem.

> 
> Also lets look at removing the EventUtils.js if it isn't needed.
> 
> ::: testing/mochitest/tests/SimpleTest/ChromeUtils.js
> @@ +223,5 @@
> > +    synthesizeMouse = eventUtils.synthesizeMouse;
> > +  }
> > +
> > +  // For events to trigger the UA's default actions they need to be "trusted".
> > +  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> 
> we don't need this in chrome land.
> 
> ::: toolkit/mozapps/downloads/tests/chrome/test_bug_462172.xul
> @@ +63,5 @@
> >            src="utils.js"/>
> >    <script type="application/javascript"
> >            src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"></script>
> > +  <script type="application/javascript"
> > +          src="chrome://mochikit/content/tests/SimpleTest/ChromeUtils.js"></script>
> 
> do we need eventutils.js anymore?

All the ChromeUtils.js functions currently require EventUtils.js, since they call the helper function _getDOMWindowUtils, and the two functions that are being movied require other methods from EventUtils (synthesizeMouse and sythesizeMouseAtCenter).
(In reply to Joel Maher (:jmaher) from comment #6)
> another piece of feedback here is that we are adding these awesome
> unit/sanity tests for the harness and utilities, but are these intended to
> be run during regular tests?  Will there be a separate test run to run these
> benchmarked?  Will we do regular, chrome, and browser-chrome runs?

Yes, they will be run as regular tests, but we will have two VMs (win32 and linux) that are listening to pulse and will benchmark these tests by running them in loops, parsing out the logs for profiler specific output (load and run times are logged) and submit the average metrics to autolog.
I added some helpful comments: 
- explaining why we are loading ChromeUtils in the two browser-chrome tests
- explaining that ChromeUtils depends on EventUtils in ChromeUtils.js

Also, synthesizeDragStart isn't actually used by browser-chrome, it is only used by chrome. As a result, I removed the optional eventUtils parameter from its function declaration.
Attachment #557417 - Attachment is obsolete: true
Attachment #557647 - Flags: review?(jmaher)
Comment on attachment 557647 [details] [diff] [review]
move two chrome functions out of EventUtils.js and into ChromeUtils.js

Review of attachment 557647 [details] [diff] [review]:
-----------------------------------------------------------------

Can we make the comment about loadsubscript a bit more descriptive than "//load ChromeUtils.js into the scope of the test, so we may use synthesizeDrop".  I would like to see something like:
// Instead of loading ChromeUtils.js into the test scope in browser-test.js for all tests,
// we only need ChromeUtils.js for a few files which is why we are using loadSubScript.

r- because I am unclear on the use of synthesizeDrop and the eventutils parameter and we need to cleanup the src=EventUtils.js when it isn't needed.

::: testing/mochitest/tests/SimpleTest/ChromeUtils.js
@@ +218,5 @@
> +
> +  if (typeof(eventUtils) != 'undefined') {
> +    synthesizeMouseAtCenter = eventUtils.synthesizeMouseAtCenter;
> +    synthesizeMouse = eventUtils.synthesizeMouse;
> +  }

If no eventUtils, will we even have synthesizeMouse[AtCenter]?  Maybe some room for cleanup here.

::: toolkit/content/tests/chrome/findbar_window.xul
@@ +53,1 @@
>  

we include ChromeUtils.js for synthesizeDrop, but we don't modify the function call to synthesizeDrop to include EventUtils?  Am I missing something?

::: toolkit/mozapps/downloads/tests/chrome/test_bug_462172.xul
@@ +68,1 @@
>    <script type="application/javascript">

in this file we only use synthesizeDragStart, I don't think we need EventUtils.js anymore.

::: toolkit/mozapps/extensions/test/browser/browser_dragdrop.js
@@ +11,5 @@
>  var gManagerWindow;
> +var chromeUtils = {};
> +this._scriptLoader = Cc["@mozilla.org/moz/jssubscript-loader;1"].
> +                     getService(Ci.mozIJSSubScriptLoader);
> +this._scriptLoader.loadSubScript("chrome://mochikit/content/tests/SimpleTest/ChromeUtils.js", chromeUtils);

comment here about the loadSubScript.
Attachment #557647 - Flags: review?(jmaher) → review-
Comment on attachment 557420 [details] [diff] [review]
added and modified tests

Review of attachment 557420 [details] [diff] [review]:
-----------------------------------------------------------------

I am not verifying that we are getting full coverage on all our utility and harness files, but these new tests and profiling metrics look good.

::: testing/mochitest/chrome/test_sanityChromeUtils.xul
@@ +1,1 @@
> +<?xml version="1.0"?>

can we get a license header in this file?

::: testing/mochitest/tests/SimpleTest/EventUtils.js
@@ -543,5 @@
>  
> -/*
> - * synthesizeComposition, synthesizeText and synthesizeQuerySelectedText
> - * are only used by layout/base/tests/test_reftests_with_caret.html.
> - */

why is this comment removed?

::: testing/mochitest/tests/test_sanitySimpletest.html
@@ +69,5 @@
> +      //note: this also adds a short wait period
> +      SimpleTest.executeSoon(
> +        function () {
> +          //finish() calls a slew of SimpleTest functions
> +          SimpleTest.finish();

seems odd that we would call finish() before the rest of the code is completed?  Is there a reason this isn't at the end of the function?
Attachment #557420 - Flags: review? → review+
We talked about this offline, but for the record:

(In reply to Joel Maher (:jmaher) from comment #10)
> ::: testing/mochitest/tests/SimpleTest/ChromeUtils.js
> @@ +218,5 @@
> > +
> > +  if (typeof(eventUtils) != 'undefined') {
> > +    synthesizeMouseAtCenter = eventUtils.synthesizeMouseAtCenter;
> > +    synthesizeMouse = eventUtils.synthesizeMouse;
> > +  }
> 
> If no eventUtils, will we even have synthesizeMouse[AtCenter]?  Maybe some
> room for cleanup here.
> 

If eventUtils is not passed, then the function will assume that EventUtils.js was loaded into the scope. The next patch will include a comment about this.

> ::: toolkit/content/tests/chrome/findbar_window.xul
> @@ +53,1 @@
> >  
> 
> we include ChromeUtils.js for synthesizeDrop, but we don't modify the
> function call to synthesizeDrop to include EventUtils?  Am I missing
> something?
> 
> ::: toolkit/mozapps/downloads/tests/chrome/test_bug_462172.xul
> @@ +68,1 @@
> >    <script type="application/javascript">
> 
> in this file we only use synthesizeDragStart, I don't think we need
> EventUtils.js anymore.

All the functions in ChromeUtils.js, including synthesizeDragStart require EventUtils.js to be in the scope. The new patch has a comment about this in the first few lines of ChromeUtils.js.
(In reply to Joel Maher (:jmaher) from comment #11)
> ::: testing/mochitest/tests/SimpleTest/EventUtils.js
> @@ -543,5 @@
> >  
> > -/*
> > - * synthesizeComposition, synthesizeText and synthesizeQuerySelectedText
> > - * are only used by layout/base/tests/test_reftests_with_caret.html.
> > - */
> 
> why is this comment removed?

Comment was removed since those functions are now used in many other tests.

> 
> ::: testing/mochitest/tests/test_sanitySimpletest.html
> @@ +69,5 @@
> > +      //note: this also adds a short wait period
> > +      SimpleTest.executeSoon(
> > +        function () {
> > +          //finish() calls a slew of SimpleTest functions
> > +          SimpleTest.finish();
> 
> seems odd that we would call finish() before the rest of the code is
> completed?  Is there a reason this isn't at the end of the function?

The calls that follow the finish() call wouldn't allow the test to finish safely. 
SimpleTest.expectUncaughtException() causes the test to end prematurely if called before finish, and SimpleTest.expectChildProcessCrash() ended up generating very varied runtimes when called before finish.

There actually is a bug in this code though. Since I called expectedUncaughtException and then threw an exception right after it, it ended the test run, so SimpleTest.expectChildProcessCrash never got called, nor did the very important logging statement after it. This means I would have to throw the exception at the end in order for all lines to be processed. After doing so, I got failures since expectChildProcessCrash would never find a crashed child proc. I will remove this process crash test from this suite since it is already being tested in dom/plugins/test/mochitest/test_crashing.html, and actually testing a child process failure causes wildly variable runtimes, so it would affect our profiling suite.
added comments for clarification
Attachment #557647 - Attachment is obsolete: true
Attachment #558880 - Flags: review?(jmaher)
Fixed the test_sanitySimpletest.html bug I mentioned above by removing the expectChildProcessCrash test and by moving the thrown exception to the end of the test so now it successfully logs. I added comments about the reasoning.
Attachment #557420 - Attachment is obsolete: true
Attachment #559201 - Flags: review?(jmaher)
Attachment #558880 - Flags: review?(jmaher) → review+
Attachment #559201 - Flags: review?(jmaher) → review+
https://hg.mozilla.org/mozilla-central/rev/8707dfaa4a45
https://hg.mozilla.org/mozilla-central/rev/d29027e1fae4
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [buildfaster:p1][inbound] → [buildfaster:p1]
Target Milestone: --- → mozilla9
So, did the patches in this bug actually address the original intent of this bug, namely, profiling our test runs?
The bug reporter wrote the patch, so it can be assumed he knows what he wanted, right?
What do you mean with "profiling our test runs"?
(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #19)
> The bug reporter wrote the patch, so it can be assumed he knows what he
> wanted, right?
> What do you mean with "profiling our test runs"?

Please see comment 0.

I did not accuse anybody of anything, I was mostly curious since this is a buildfaster:p1 bug, and to the best of my knowledge reading the patches, they don't achieve the goal here...
Sorry, I didn't mean to imply you were accusing anybody of anything. I merely implied the reporter knew what he meant to do with this bug when he wrote this patch.
I guess you should file a new bug on whatever you meant this bug was about and isn't.
In reviewing these patches, I found they were good for adding unittests and profiling the harness proper vs the tests.  So now we will know if the core harness will change in speed.
Ehsan: the goal of this bug was to add a static set of Mochitests that we can use for profiling/measuring the speed of the harness itself, vs. the ever-changing set of existing mochitests. It sounds like that goal has been achieved, and now we just need to measure the results.
(In reply to Ted Mielczarek [:ted, :luser] from comment #23)
> Ehsan: the goal of this bug was to add a static set of Mochitests that we
> can use for profiling/measuring the speed of the harness itself, vs. the
> ever-changing set of existing mochitests. It sounds like that goal has been
> achieved, and now we just need to measure the results.

Right, what Ted said. The measurement of results is being done outside of the m-c tree, and has not yet been completed but will be added shortly to http://brasstacks.mozilla.com/testperf_dashboard/
(In reply to Malini Das from comment #24)
> (In reply to Ted Mielczarek [:ted, :luser] from comment #23)
> > Ehsan: the goal of this bug was to add a static set of Mochitests that we
> > can use for profiling/measuring the speed of the harness itself, vs. the
> > ever-changing set of existing mochitests. It sounds like that goal has been
> > achieved, and now we just need to measure the results.

Ah, OK, this makes a lot of sense!  Thanks for explaining.  :-)

> Right, what Ted said. The measurement of results is being done outside of
> the m-c tree, and has not yet been completed but will be added shortly to
> http://brasstacks.mozilla.com/testperf_dashboard/

This looks great!
Depends on: 688052
The patch mentioned in comment #15 wasn't actually attached and so the patch that made it to m-c is missing the mentioned changes. I will be uploading the correct patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #563534 - Flags: review?(jmaher) → review+
landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2d92f28915d
Whiteboard: [buildfaster:p1] → [buildfaster:p1][inbound]
https://hg.mozilla.org/mozilla-central/rev/e2d92f28915d
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.