Closed
Bug 765357
Opened 12 years ago
Closed 12 years ago
Add xpcshell tests for the Profiler
Categories
(Core :: Gecko Profiler, defect)
Tracking
()
RESOLVED
FIXED
mozilla16
People
(Reporter: BenWa, Assigned: BenWa)
Details
Attachments
(1 file, 6 obsolete files)
10.74 KB,
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Attachment #633651 -
Flags: review?(ehsan)
Assignee | ||
Comment 1•12 years ago
|
||
Not sure if changes to 'testing/xpcshell/xpcshell.ini' are correct if we don't build with MOZ_PROFILER_ENABLED.
Comment 2•12 years ago
|
||
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-
Updated•12 years ago
|
Assignee: nobody → bgirard
Assignee | ||
Comment 3•12 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•12 years ago
|
||
Attachment #633651 -
Attachment is obsolete: true
Comment 5•12 years ago
|
||
(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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
Attachment #635873 -
Attachment is obsolete: true
Attachment #635873 -
Flags: review?(ehsan)
Attachment #635878 -
Flags: review?(ehsan)
Comment 9•12 years ago
|
||
Comment on attachment 635878 [details] [diff] [review] right patch Looks great!
Attachment #635878 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 10•12 years ago
|
||
Removed the 'xul' check for shared library since it's not a proper cross platform test.
Attachment #636196 -
Flags: review+
Assignee | ||
Comment 11•12 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•12 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/61b4a68e24c9
Assignee | ||
Comment 15•12 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•12 years ago
|
||
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•12 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•12 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•12 years ago
|
||
Attachment #636479 -
Attachment is obsolete: true
Attachment #636890 -
Flags: review+
Assignee | ||
Comment 20•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ee5063de5d5
Comment 21•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3ee5063de5d5
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in
before you can comment on or make changes to this bug.
Description
•