Closed
Bug 986160
Opened 10 years ago
Closed 10 years ago
Create unit tests for the Gecko Profiler
Categories
(Core :: Gecko Profiler, defect)
Core
Gecko Profiler
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: vikstrous, Assigned: BenWa)
References
Details
Attachments
(2 files, 12 obsolete files)
9.12 KB,
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
5.31 KB,
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
Create unit tests for the Gecko Profiler using gtest.
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → vstanchev
Reporter | ||
Comment 1•10 years ago
|
||
This is my current approach. Let me know if this makes sense. I had to move some class definitions out of the os-specific cc files to get the tests to compile properly.
Flags: needinfo?(bgirard)
Reporter | ||
Comment 2•10 years ago
|
||
Oops forgot to delete osx version of PlatformData.
Attachment #8394403 -
Attachment is obsolete: true
Reporter | ||
Comment 3•10 years ago
|
||
Attachment #8394405 -
Attachment is obsolete: true
Assignee | ||
Comment 4•10 years ago
|
||
Comment on attachment 8395034 [details] [diff] [review] profiler_memory Review of attachment 8395034 [details] [diff] [review]: ----------------------------------------------------------------- ::: tools/profiler/ProfileEntry.h @@ +12,4 @@ > #include "platform.h" > #include "ProfilerBacktrace.h" > #include "mozilla/Mutex.h" > +#include "gtest/gtest_prod.h" This should be gtest.h. Is there a good reason for using gtest_proh instead? ::: tools/profiler/tests/gtest/BasicSamplerTest.cpp @@ +1,4 @@ > +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ IMO I'd drop Basic from the name. Really this is more 'ThreadProfileTest' @@ +19,5 @@ > +TEST(ThreadProfile, InsertOneTag) { > + PseudoStack stack; > + Thread::tid_t tid = 1000; > + PlatformData platform; > + ThreadProfile tp("testThread", 10, &stack, tid, &platform, true, nullptr); Can we get away with not moving PlatformData into the header and passing null here for the test instead? PlatformData is meant to be an implementation detail so keeping that hidden is desirable. @@ +35,5 @@ > + ThreadProfile tp("testThread", 1, &stack, tid, &platform, true, nullptr); > + tp.addTag(ProfileEntry('t', 123.1)); > + ASSERT_TRUE(tp.mEntries != nullptr); > + ASSERT_TRUE(tp.mEntries[tp.mReadPos].mTagName != 't'); > + ASSERT_TRUE(tp.mEntries[tp.mReadPos].mTagFloat != 123.1); I don't think this isn't a valid test. mReadPos doesn't point to a valid tag in this case so you have no idea what you're reading. If it un-initialized memory then you may get these values by change. Also I think this test is too specific to the implementation. That is: We should be able to switch to a circular buffer that doesn't keep an empty slot without having any test regress. If we really want to test this then we should check that mReadPos and mWritePos are set such that there are no tag stored in the profile. @@ +97,5 @@ > + } > + uintptr_t size = (char*)&tp.mEntries[buffer_size] - (char*)&tp.mEntries[0]; > + printf("Number of buffer entries %d\n", tags); > + // TODO: What printf type to use? > + printf("Size of buffer in bytes %ld\n", size); This test doesn't assert and shouldn't print if it's successful.
Attachment #8395034 -
Flags: review-
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(bgirard)
Reporter | ||
Comment 5•10 years ago
|
||
(In reply to Benoit Girard (:BenWa) from comment #4) > Comment on attachment 8395034 [details] [diff] [review] > profiler_memory > > Review of attachment 8395034 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: tools/profiler/ProfileEntry.h > @@ +12,4 @@ > > #include "platform.h" > > #include "ProfilerBacktrace.h" > > #include "mozilla/Mutex.h" > > +#include "gtest/gtest_prod.h" > > This should be gtest.h. Is there a good reason for using gtest_proh instead? Looks like that was more specific. gtest.h includes gtest_proh.h. The example from Google I was looking at included that. It dosn't really matter though, so I changed it to gtest.h. > > ::: tools/profiler/tests/gtest/BasicSamplerTest.cpp > @@ +1,4 @@ > > +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > > +/* This Source Code Form is subject to the terms of the Mozilla Public > > + * License, v. 2.0. If a copy of the MPL was not distributed with this > > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > IMO I'd drop Basic from the name. Really this is more 'ThreadProfileTest' Yep. > > @@ +19,5 @@ > > +TEST(ThreadProfile, InsertOneTag) { > > + PseudoStack stack; > > + Thread::tid_t tid = 1000; > > + PlatformData platform; > > + ThreadProfile tp("testThread", 10, &stack, tid, &platform, true, nullptr); > > Can we get away with not moving PlatformData into the header and passing > null here for the test instead? PlatformData is meant to be an > implementation detail so keeping that hidden is desirable. Oh, I didn't try that. Yeah, that works. > > @@ +35,5 @@ > > + ThreadProfile tp("testThread", 1, &stack, tid, &platform, true, nullptr); > > + tp.addTag(ProfileEntry('t', 123.1)); > > + ASSERT_TRUE(tp.mEntries != nullptr); > > + ASSERT_TRUE(tp.mEntries[tp.mReadPos].mTagName != 't'); > > + ASSERT_TRUE(tp.mEntries[tp.mReadPos].mTagFloat != 123.1); > > I don't think this isn't a valid test. mReadPos doesn't point to a valid tag > in this case so you have no idea what you're reading. If it un-initialized > memory then you may get these values by change. Also I think this test is > too specific to the implementation. That is: We should be able to switch to > a circular buffer that doesn't keep an empty slot without having any test > regress. Okay. They only reason I added it was because when I tried size 1 it wasn't raving the tag, so I wrote a test checking that it keeps failing for size one. I agree that this test is bad. I'll remove it. > > If we really want to test this then we should check that mReadPos and > mWritePos are set such that there are no tag stored in the profile. > > @@ +97,5 @@ > > + } > > + uintptr_t size = (char*)&tp.mEntries[buffer_size] - (char*)&tp.mEntries[0]; > > + printf("Number of buffer entries %d\n", tags); > > + // TODO: What printf type to use? > > + printf("Size of buffer in bytes %ld\n", size); > > This test doesn't assert and shouldn't print if it's successful. I was using this just to check the size of the buffer when I made changes to it. It's not really a test. I'll take it out.
Reporter | ||
Comment 6•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=b04a74b35982
Attachment #8395034 -
Attachment is obsolete: true
Attachment #8398086 -
Flags: review?(bgirard)
Assignee | ||
Comment 7•10 years ago
|
||
Comment on attachment 8398086 [details] [diff] [review] profiler_tests Review of attachment 8398086 [details] [diff] [review]: ----------------------------------------------------------------- Sweet!
Attachment #8398086 -
Flags: review?(bgirard) → review+
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(bgirard)
Reporter | ||
Comment 8•10 years ago
|
||
Attachment #8398086 -
Attachment is obsolete: true
Reporter | ||
Updated•10 years ago
|
Keywords: checkin-needed
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(bgirard)
Reporter | ||
Comment 10•10 years ago
|
||
Rebased.
Reporter | ||
Updated•10 years ago
|
Keywords: checkin-needed
Reporter | ||
Updated•10 years ago
|
Attachment #8399639 -
Attachment is obsolete: true
Comment 11•10 years ago
|
||
This still doesn't apply in toolkit/library/Makefile.in. Are you sure you're rebasing to tip?
Updated•10 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 12•10 years ago
|
||
My last rebase was not on a sufficiently new version. The testing build system changed a bit. This should work now. https://tbpl.mozilla.org/?tree=Try&rev=786fa5f6cd7b
Attachment #8400047 -
Attachment is obsolete: true
Attachment #8400804 -
Flags: review?(bgirard)
Assignee | ||
Updated•10 years ago
|
Attachment #8400804 -
Flags: review?(bgirard) → review+
Assignee | ||
Comment 13•10 years ago
|
||
:D https://hg.mozilla.org/integration/mozilla-inbound/rev/1e38b4aa8889
Comment 14•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1e38b4aa8889
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Comment 15•10 years ago
|
||
I get this error in my nightly build. mozilla/tools/profiler/ProfileEntry.h:15:25: fatal error: gtest/gtest.h: No such file or directory If --enable-profiling is required in .mozconfig, maybe there needs to be some #ifdef MOZ_PROFILING in this file.
Reporter | ||
Comment 16•10 years ago
|
||
I suspect that the issue is in tools/profiler/moz.build : if CONFIG['ENABLE_TESTS']: DIRS += ['tests/gtest'] I suspect that the gtest headers are not even included in the nightly build. I'm not sure what the right solution is and I'm not that familiar with the build system. It might make sense to revert this patch for now until we have a solution.
Status: RESOLVED → REOPENED
Flags: needinfo?(bgirard)
Resolution: FIXED → ---
Backed out in http://hg.mozilla.org/mozilla-central/rev/5fa70bd90a8b until it can be looked at closer.
Flags: in-testsuite+
Target Milestone: mozilla31 → ---
Assignee | ||
Comment 18•10 years ago
|
||
use gtest_prod.h
Attachment #8400804 -
Attachment is obsolete: true
Attachment #8402845 -
Flags: review+
Flags: needinfo?(bgirard)
Assignee | ||
Comment 19•10 years ago
|
||
Attachment #8402846 -
Flags: review?(gps)
Assignee | ||
Updated•10 years ago
|
Attachment #8402846 -
Flags: review?(mh+mozilla)
Comment 20•10 years ago
|
||
Comment on attachment 8402846 [details] [diff] [review] Part 1: Always install gtest_prod.h Review of attachment 8402846 [details] [diff] [review]: ----------------------------------------------------------------- I'd rather see a wrapper header that only includes gtest_prod when ENABLE_TESTS is defined, and defines FRIEND_TEST to expand to nothing otherwise. Which would be friendlier in a SDK environment.
Attachment #8402846 -
Flags: review?(mh+mozilla)
Attachment #8402846 -
Flags: review?(gps)
Attachment #8402846 -
Flags: review-
Assignee | ||
Comment 21•10 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #20) > Comment on attachment 8402846 [details] [diff] [review] > Part 1: Always install gtest_prod.h > > Review of attachment 8402846 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'd rather see a wrapper header that only includes gtest_prod when > ENABLE_TESTS is defined, and defines FRIEND_TEST to expand to nothing > otherwise. Which would be friendlier in a SDK environment. That's exactly what gtest_prod does BTW
Comment 22•10 years ago
|
||
(In reply to Benoit Girard (:BenWa) from comment #21) > > I'd rather see a wrapper header that only includes gtest_prod when > > ENABLE_TESTS is defined, and defines FRIEND_TEST to expand to nothing > > otherwise. Which would be friendlier in a SDK environment. > > That's exactly what gtest_prod does BTW No it's not.
Assignee | ||
Comment 23•10 years ago
|
||
I mean that effectively it's the same. gtest_prod only expends FRIEND_TEST to a friend class statement which doesn't require any forward declaration. I don't see the point of adding more ifdef cruf when gtest_prod will work regardless of the build configuration. We get simpler code and code that is more familiar to core gtest. I don't see the value of expending FRIEND_TEST to nothing in a SDK environment.
Assignee | ||
Comment 24•10 years ago
|
||
Assignee: vikstrous → bgirard
Attachment #8402846 -
Attachment is obsolete: true
Status: REOPENED → ASSIGNED
Attachment #8415638 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 25•10 years ago
|
||
Attachment #8402845 -
Attachment is obsolete: true
Attachment #8415639 -
Flags: review+
Comment 26•10 years ago
|
||
Comment on attachment 8415638 [details] [diff] [review] Part 1: Always install gtest_prod.h Review of attachment 8415638 [details] [diff] [review]: ----------------------------------------------------------------- ::: configure.in @@ +6428,5 @@ > ENABLE_TESTS=1 ) > > if test -n "$ENABLE_TESTS"; then > GTEST_HAS_RTTI=0 > + AC_DEFINE(ENABLE_TESTS) I think there are a few "manual" ENABLE_TESTS throughout the tree that could be removed, with this. File a followup? ::: testing/gtest/moz.build @@ +4,5 @@ > # License, v. 2.0. If a copy of the MPL was not distributed with this > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > EXPORTS.gtest += [ > + 'MozGtestFriend.h', <insert naming bikeshedding> ;) @@ +9,4 @@ > ] > > +if CONFIG['ENABLE_TESTS']: > + EXPORTS.gtest += [ If you installed the file from testing/, you wouldn't have to add so much indentation.
Attachment #8415638 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 27•10 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #26) > Comment on attachment 8415638 [details] [diff] [review] > Part 1: Always install gtest_prod.h > > Review of attachment 8415638 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: configure.in > @@ +6428,5 @@ > > ENABLE_TESTS=1 ) > > > > if test -n "$ENABLE_TESTS"; then > > GTEST_HAS_RTTI=0 > > + AC_DEFINE(ENABLE_TESTS) > > I think there are a few "manual" ENABLE_TESTS throughout the tree that could > be removed, with this. File a followup? I just rolled it into this patch. No point in splitting. This does raise a problem: http://mxr.mozilla.org/mozilla-central/source/netwerk/ipc/NeckoCommon.h#20 I decided to remove ' || defined(ENABLE_TESTS)' since it was dead code for most of the tree (except for a few dirs that had a local -DENABLE_TESTS) and not working as intended. This should minimize the side effects of this patch. > > ::: testing/gtest/moz.build > @@ +4,5 @@ > > # License, v. 2.0. If a copy of the MPL was not distributed with this > > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > > > EXPORTS.gtest += [ > > + 'MozGtestFriend.h', > > <insert naming bikeshedding> ;) > I'm open to suggestion. Otherwise I'll land it as it. It will be easy to search and replace if we don't like it. > @@ +9,4 @@ > > ] > > > > +if CONFIG['ENABLE_TESTS']: > > + EXPORTS.gtest += [ > > If you installed the file from testing/, you wouldn't have to add so much > indentation. There's no moz.build file there now. Isn't adding one for this a waste? The indentation sucks but more build steps is worse IMO.
Assignee | ||
Comment 28•10 years ago
|
||
Attachment #8415638 -
Attachment is obsolete: true
Attachment #8416036 -
Flags: review+
Assignee | ||
Comment 29•10 years ago
|
||
Just a heads up I'm removing this because it never worked properly and I'm trying to preserve the existing behavior: http://mxr.mozilla.org/mozilla-central/source/netwerk/ipc/NeckoCommon.h#20 If this is important please file a bug to clean it up.
Flags: needinfo?(jduell.mcbugs)
Assignee | ||
Comment 31•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=ebf77e6d7c38
Assignee | ||
Comment 32•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=47de4d32e0db
Assignee | ||
Comment 33•10 years ago
|
||
Attachment #8415639 -
Attachment is obsolete: true
Attachment #8416178 -
Flags: review+
Assignee | ||
Comment 34•10 years ago
|
||
Correct patch
Attachment #8416178 -
Attachment is obsolete: true
Attachment #8416253 -
Flags: review+
Assignee | ||
Comment 35•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ba1a2edc9f46 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/6e6296b804c2
Comment 36•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ba1a2edc9f46 https://hg.mozilla.org/mozilla-central/rev/6e6296b804c2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•