Closed Bug 765357 Opened 8 years ago Closed 8 years ago

Add xpcshell tests for the Profiler

Categories

(Core :: Gecko Profiler, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: BenWa, Assigned: BenWa)

Details

Attachments

(1 file, 6 obsolete files)

Attached patch patch (obsolete) — Splinter Review
No description provided.
Attachment #633651 - Flags: review?(ehsan)
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
(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.
Attached patch patch (obsolete) — Splinter Review
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+
Attached patch patch (obsolete) — Splinter Review
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)
Attached patch right patch (obsolete) — Splinter Review
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+
Attached patch patch (obsolete) — Splinter Review
Removed the 'xul' check for shared library since it's not a proper cross platform test.
Attachment #636196 - Flags: review+
This is perma-orange on Mac OS X 32-bit.
(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?
Attached patch patch (obsolete) — Splinter Review
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+
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 :(.
Attachment #636479 - Attachment is obsolete: true
Attachment #636890 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/3ee5063de5d5
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.