Create unit tests for the Gecko Profiler

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: vikstrous, Assigned: BenWa)

Tracking

(Depends on 1 bug)

unspecified
mozilla32
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 12 obsolete attachments)

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.
Assignee: nobody → vstanchev
Posted patch profiler_memory (obsolete) — Splinter Review
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)
Posted patch profiler_memory (obsolete) — Splinter Review
Oops forgot to delete osx version of PlatformData.
Attachment #8394403 - Attachment is obsolete: true
Posted patch profiler_memory (obsolete) — Splinter Review
Attachment #8394405 - Attachment is obsolete: true
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-
Flags: needinfo?(bgirard)
(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.
Posted patch profiler_tests (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=b04a74b35982
Attachment #8395034 - Attachment is obsolete: true
Attachment #8398086 - Flags: review?(bgirard)
Comment on attachment 8398086 [details] [diff] [review]
profiler_tests

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

Sweet!
Attachment #8398086 - Flags: review?(bgirard) → review+
Flags: needinfo?(bgirard)
Posted patch profiler_tests2 (obsolete) — Splinter Review
Attachment #8398086 - Attachment is obsolete: true
Keywords: checkin-needed
Flags: needinfo?(bgirard)
This doesn't apply to inbound. Please rebase.
Keywords: checkin-needed
Posted patch profiler_tests3 (obsolete) — Splinter Review
Rebased.
Keywords: checkin-needed
Attachment #8399639 - Attachment is obsolete: true
This still doesn't apply in toolkit/library/Makefile.in. Are you sure you're rebasing to tip?
Posted patch profiler_tests4 (obsolete) — Splinter Review
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)
Attachment #8400804 - Flags: review?(bgirard) → review+
https://hg.mozilla.org/mozilla-central/rev/1e38b4aa8889
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
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.
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 → ---
Depends on: 992251
Backed out in http://hg.mozilla.org/mozilla-central/rev/5fa70bd90a8b until it can be looked at closer.
Flags: in-testsuite+
Target Milestone: mozilla31 → ---
Posted patch profiler_tests5 (obsolete) — Splinter Review
use gtest_prod.h
Attachment #8400804 - Attachment is obsolete: true
Attachment #8402845 - Flags: review+
Flags: needinfo?(bgirard)
Attachment #8402846 - Flags: review?(gps)
Attachment #8402846 - Flags: review?(mh+mozilla)
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-
(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
(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.
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: vikstrous → bgirard
Attachment #8402846 - Attachment is obsolete: true
Status: REOPENED → ASSIGNED
Attachment #8415638 - Flags: review?(mh+mozilla)
Posted patch Part 2: profiler_test6 (obsolete) — Splinter Review
Attachment #8402845 - Attachment is obsolete: true
Attachment #8415639 - Flags: review+
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+
(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.
Attachment #8415638 - Attachment is obsolete: true
Attachment #8416036 - Flags: review+
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)
Sure, if it was dead code take it out.
Flags: needinfo?(jduell.mcbugs)
Posted patch Part 2: profiler_test7 (obsolete) — Splinter Review
Attachment #8415639 - Attachment is obsolete: true
Attachment #8416178 - Flags: review+
Correct patch
Attachment #8416178 - Attachment is obsolete: true
Attachment #8416253 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/ba1a2edc9f46
https://hg.mozilla.org/mozilla-central/rev/6e6296b804c2
Status: ASSIGNED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.