Last Comment Bug 765357 - Add xpcshell tests for the Profiler
: Add xpcshell tests for the Profiler
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Gecko Profiler (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla16
Assigned To: Benoit Girard (:BenWa)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-15 13:28 PDT by Benoit Girard (:BenWa)
Modified: 2012-06-27 03:34 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (10.08 KB, patch)
2012-06-15 13:28 PDT, Benoit Girard (:BenWa)
ehsan: review-
Details | Diff | Splinter Review
patch (9.76 KB, patch)
2012-06-18 17:09 PDT, Benoit Girard (:BenWa)
ehsan: review+
Details | Diff | Splinter Review
patch (9.98 KB, patch)
2012-06-22 12:53 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
right patch (10.91 KB, patch)
2012-06-22 13:09 PDT, Benoit Girard (:BenWa)
ehsan: review+
Details | Diff | Splinter Review
patch (10.85 KB, patch)
2012-06-24 15:54 PDT, Benoit Girard (:BenWa)
bgirard: review+
Details | Diff | Splinter Review
patch (10.72 KB, patch)
2012-06-25 13:50 PDT, Benoit Girard (:BenWa)
bgirard: review+
Details | Diff | Splinter Review
patch (disable test_run.js) (10.74 KB, patch)
2012-06-26 14:59 PDT, Benoit Girard (:BenWa)
bgirard: review+
Details | Diff | Splinter Review

Description Benoit Girard (:BenWa) 2012-06-15 13:28:18 PDT
Created attachment 633651 [details] [diff] [review]
patch
Comment 1 Benoit Girard (:BenWa) 2012-06-15 13:30:18 PDT
Not sure if changes to 'testing/xpcshell/xpcshell.ini' are correct if we don't build with MOZ_PROFILER_ENABLED.
Comment 2 :Ehsan Akhgari 2012-06-18 12:04:53 PDT
Comment on attachment 633651 [details] [diff] [review]
patch

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

This looks great overall, minusing for the comments.

::: testing/xpcshell/xpcshell.ini
@@ +124,5 @@
>  
>  [include:modules/libmar/tests/unit/xpcshell.ini]
>  skip-if = os == "android"
> +
> +[include:tools/profiler/tests/xpcshell.ini]

This probably won't work for non-profiling builds, but I'm not sure how to fix it.  The xpcshell manifest format sort of sucks.  :(

::: tools/profiler/Makefile.in
@@ +9,5 @@
>  srcdir      = @srcdir@
>  VPATH       = $(srcdir)
> +relativesrcdir = tools/profiler
> +
> +XPCSHELL_TESTS = tests

You should probably only enable this when MOZ_PROFILING is defined, right?

::: tools/profiler/TableTicker.cpp
@@ +247,3 @@
>        switch (entry.mTagName) {
>          case 's':
> +          printf("\n\n\n\nread pos %i %c\n", readPos, entry.mTagName);

Doing printf's here is not a good idea, because they can be expensive, and also don't interact well with the rest of the program output...

::: tools/profiler/nsIProfiler.idl
@@ +7,5 @@
>  
>  [scriptable, uuid(e388fded-1321-41af-a988-861a2bc5cfc3)]
>  interface nsIProfiler : nsISupports
>  {
> +  void StartProfiler(in PRUint32 aEntries, in PRUint32 aInterval,

Hmm, why are you making this change?  This suddenly changes the semantics of the function silently.  How about renaming the method, and revving the uuid?

::: tools/profiler/tests/test_run.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +function run_test() {
> +  profiler = Cc["@mozilla.org/tools/profiler;1"].getService(Ci.nsIProfiler);
> +  profiler.StopProfiler();

Nit: you don't need to stop the profiler because it has not started yet.

@@ +9,5 @@
> +  do_check_true(!profiler.IsActive());
> +
> +  profiler.StartProfiler(1000, 10, [], 0);
> +
> +  do_check_true(true);

Let's assume sanity in the framework.  ;-)

@@ +28,5 @@
> +    var profileObj = profiler.getProfileData();
> +    do_check_true(profileObj != null);
> +    do_check_true(profileObj.threads != null);
> +    do_check_true(profileObj.threads.length >= 1);
> +    do_check_true(profileObj.threads[0].samples != null);

Nit: please use do_check_neq for all except for the third check here.

::: tools/profiler/tests/test_shared_library.js
@@ +13,5 @@
> +  do_check_true(sharedStr.indexOf("xul") != -1);
> +  do_check_true(sharedStr.indexOf("name") != -1);
> +  do_check_true(sharedStr.indexOf("start") != -1);
> +  do_check_true(sharedStr.indexOf("end") != -1);
> +  do_check_true(sharedStr.indexOf("offset") != -1);

Nit: please use do_check_neq here.

::: tools/profiler/tests/test_start.js
@@ +9,5 @@
> +  do_check_true(!profiler.IsActive());
> +
> +  profiler.StartProfiler(10, 100, [], 0);
> +
> +  do_check_true(true);

Nit: please take this out.
Comment 3 Benoit Girard (:BenWa) 2012-06-18 17:09:04 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #2)
> ::: tools/profiler/Makefile.in
> @@ +9,5 @@
> >  srcdir      = @srcdir@
> >  VPATH       = $(srcdir)
> > +relativesrcdir = tools/profiler
> > +
> > +XPCSHELL_TESTS = tests
> 
> You should probably only enable this when MOZ_PROFILING is defined, right?
>
 
No, the profiler modules works in non profiling builds so we should test it there too.

> ::: tools/profiler/nsIProfiler.idl
> @@ +7,5 @@
> >  
> >  [scriptable, uuid(e388fded-1321-41af-a988-861a2bc5cfc3)]
> >  interface nsIProfiler : nsISupports
> >  {
> > +  void StartProfiler(in PRUint32 aEntries, in PRUint32 aInterval,
> 
> Hmm, why are you making this change?  This suddenly changes the semantics of
> the function silently.  How about renaming the method, and revving the uuid?

The param names were wrong as I noticed in testing. Note that the ordering did NOT chance, they were just labeled incorrectly.

> 
> ::: tools/profiler/tests/test_run.js
> @@ +3,5 @@
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +function run_test() {
> > +  profiler = Cc["@mozilla.org/tools/profiler;1"].getService(Ci.nsIProfiler);
> > +  profiler.StopProfiler();
> 
> Nit: you don't need to stop the profiler because it has not started yet.

I wanted to do that in case previous test had failed and/or to make sure we're back in a base state.
Comment 4 Benoit Girard (:BenWa) 2012-06-18 17:09:26 PDT
Created attachment 634245 [details] [diff] [review]
patch
Comment 5 :Ehsan Akhgari 2012-06-18 18:08:51 PDT
(In reply to Benoit Girard (:BenWa) from comment #3)
> (In reply to Ehsan Akhgari [:ehsan] from comment #2)
> > ::: tools/profiler/Makefile.in
> > @@ +9,5 @@
> > >  srcdir      = @srcdir@
> > >  VPATH       = $(srcdir)
> > > +relativesrcdir = tools/profiler
> > > +
> > > +XPCSHELL_TESTS = tests
> > 
> > You should probably only enable this when MOZ_PROFILING is defined, right?
> >
>  
> No, the profiler modules works in non profiling builds so we should test it
> there too.

OK, fair enough.

> > ::: tools/profiler/nsIProfiler.idl
> > @@ +7,5 @@
> > >  
> > >  [scriptable, uuid(e388fded-1321-41af-a988-861a2bc5cfc3)]
> > >  interface nsIProfiler : nsISupports
> > >  {
> > > +  void StartProfiler(in PRUint32 aEntries, in PRUint32 aInterval,
> > 
> > Hmm, why are you making this change?  This suddenly changes the semantics of
> > the function silently.  How about renaming the method, and revving the uuid?
> 
> The param names were wrong as I noticed in testing. Note that the ordering
> did NOT chance, they were just labeled incorrectly.

OK, cool.

> > ::: tools/profiler/tests/test_run.js
> > @@ +3,5 @@
> > > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > > +
> > > +function run_test() {
> > > +  profiler = Cc["@mozilla.org/tools/profiler;1"].getService(Ci.nsIProfiler);
> > > +  profiler.StopProfiler();
> > 
> > Nit: you don't need to stop the profiler because it has not started yet.
> 
> I wanted to do that in case previous test had failed and/or to make sure
> we're back in a base state.

Each xpcshell test is run in its own separate process, so individual tests do not interact with each other.
Comment 6 :Ehsan Akhgari 2012-06-18 18:10:20 PDT
Comment on attachment 634245 [details] [diff] [review]
patch

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

r=me

::: tools/profiler/tests/test_run.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +function run_test() {
> +  profiler = Cc["@mozilla.org/tools/profiler;1"].getService(Ci.nsIProfiler);
> +  profiler.StopProfiler();

Please take this out as I mentioned in the previous comment.
Comment 7 Benoit Girard (:BenWa) 2012-06-22 12:53:46 PDT
Created attachment 635873 [details] [diff] [review]
patch

Don't run the tests if the profiler module isn't built.

This patch is ready to land.
Comment 8 Benoit Girard (:BenWa) 2012-06-22 13:09:17 PDT
Created attachment 635878 [details] [diff] [review]
right patch
Comment 9 :Ehsan Akhgari 2012-06-22 13:16:47 PDT
Comment on attachment 635878 [details] [diff] [review]
right patch

Looks great!
Comment 10 Benoit Girard (:BenWa) 2012-06-24 15:54:35 PDT
Created attachment 636196 [details] [diff] [review]
patch

Removed the 'xul' check for shared library since it's not a proper cross platform test.
Comment 12 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-24 17:39:44 PDT
This is perma-orange on Mac OS X 32-bit.
Comment 13 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-24 17:44:43 PDT
And also failed on 1 out of 2 Linux64 debug runs:
https://tbpl.mozilla.org/php/getParsedLog.php?id=12958435&tree=Mozilla-Inbound
Comment 14 Benoit Girard (:BenWa) 2012-06-24 18:01:28 PDT
Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/61b4a68e24c9
Comment 15 Benoit Girard (:BenWa) 2012-06-24 18:05:01 PDT
(In reply to David Baron [:dbaron] from comment #13)
> And also failed on 1 out of 2 Linux64 debug runs:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=12958435&tree=Mozilla-
> Inbound

It's not clear to me what the failure is here. What's return code -11? Leaks?
Comment 16 Benoit Girard (:BenWa) 2012-06-25 13:50:44 PDT
Created attachment 636479 [details] [diff] [review]
patch

Fixed the mac test by making more general. Still not sure what is causing the linux64 intermittent crash.
Comment 17 Benoit Girard (:BenWa) 2012-06-25 15:53:31 PDT
I relanded this with the tests disabled since this has bug fixes for the profiler until I can figure out what the linux 64 crash is. I couldn't reproduce locally :(.
Comment 19 Benoit Girard (:BenWa) 2012-06-26 14:59:29 PDT
Created attachment 636890 [details] [diff] [review]
patch (disable test_run.js)
Comment 21 Ed Morley [:emorley] 2012-06-27 03:34:29 PDT
https://hg.mozilla.org/mozilla-central/rev/3ee5063de5d5

Note You need to log in before you can comment on or make changes to this bug.