Add xpcshell tests for the Profiler

RESOLVED FIXED in mozilla16

Status

()

Core
Gecko Profiler
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: BenWa, Assigned: BenWa)

Tracking

Trunk
mozilla16
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 633651 [details] [diff] [review]
patch
Attachment #633651 - Flags: review?(ehsan)
(Assignee)

Comment 1

5 years ago
Not sure if changes to 'testing/xpcshell/xpcshell.ini' are correct if we don't build with MOZ_PROFILER_ENABLED.
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.
Attachment #633651 - Flags: review?(ehsan) → review-
Assignee: nobody → bgirard
(Assignee)

Comment 3

5 years ago
(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.
(Assignee)

Comment 4

5 years ago
Created attachment 634245 [details] [diff] [review]
patch
Attachment #633651 - Attachment is obsolete: true
(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 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.
Attachment #634245 - Flags: review+
(Assignee)

Comment 7

5 years ago
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.
Attachment #634245 - Attachment is obsolete: true
Attachment #635873 - Flags: review?(ehsan)
(Assignee)

Comment 8

5 years ago
Created attachment 635878 [details] [diff] [review]
right patch
Attachment #635873 - Attachment is obsolete: true
Attachment #635873 - Flags: review?(ehsan)
Attachment #635878 - Flags: review?(ehsan)
Comment on attachment 635878 [details] [diff] [review]
right patch

Looks great!
Attachment #635878 - Flags: review?(ehsan) → review+
(Assignee)

Comment 10

5 years ago
Created attachment 636196 [details] [diff] [review]
patch

Removed the 'xul' check for shared library since it's not a proper cross platform test.
Attachment #636196 - Flags: review+
(Assignee)

Comment 11

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/746513dca667
This is perma-orange on Mac OS X 32-bit.
And also failed on 1 out of 2 Linux64 debug runs:
https://tbpl.mozilla.org/php/getParsedLog.php?id=12958435&tree=Mozilla-Inbound
(Assignee)

Comment 14

5 years ago
Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/61b4a68e24c9
(Assignee)

Comment 15

5 years ago
(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?
(Assignee)

Comment 16

5 years ago
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.
Attachment #635878 - Attachment is obsolete: true
Attachment #636196 - Attachment is obsolete: true
Attachment #636479 - Flags: review+
(Assignee)

Comment 17

5 years ago
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 :(.
(Assignee)

Comment 18

5 years ago
Push:
https://hg.mozilla.org/integration/mozilla-inbound/rev/493781e75ced
Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e13388210fc2
(Assignee)

Comment 19

5 years ago
Created attachment 636890 [details] [diff] [review]
patch (disable test_run.js)
Attachment #636479 - Attachment is obsolete: true
Attachment #636890 - Flags: review+
(Assignee)

Comment 20

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ee5063de5d5
https://hg.mozilla.org/mozilla-central/rev/3ee5063de5d5
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.