Closed Bug 767231 (gtest) Opened 12 years ago Closed 11 years ago

Add ability to write compiled-code unit tests using the GTest framework

Categories

(Testing :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla22

People

(Reporter: briansmith, Assigned: BenWa)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 9 obsolete files)

GTest would make writing compiled-code tests easier.

GTest is already being used in WebRTC

See https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.platform/Q8AJiU9yJDk

I propose that we use this bug for the following tasks:

1. Getting GTest to build as part of our build system.
2. Adding useful glue between GTest and our current C++ testing framework.
3. Adding Mozilla-specific assertion macros.

I have started working on these tasks.

So far, I have made it so one can write compiled-code unit tests using GTest for functions exported from libxul only; I didn't add the support for testing non-exported functions, though I agree that is a good idea. I would like to do that in a follow-up bug.
Some notes:

* I propose GTest itself should live in testing/gtest/gtest.
* I propose that we check out only a subset the GTest source tree that we actually use.
* I propose Mozilla-specific GTest stuff (e.g. custom assertion macros for XPCOM types) should live in testing/gtest/mozilla.
* I propose that GTest-based tests should work basically the same as normal C++ unit tests based on TestHarness.h, to minimize changes to the build system.
* I propose that we build a static library that contains a standard main() and some utilities that gets linked into every GTest-based test; this will minimize the amount of boilerplate that each test source file has to contain.

I know the directory structure is ugly [1] (we would having testing/gtest/gtest/include/gtest/" is ugly, but since it is a third-party library with a non-MPL license it should live in a separate subdirectory from MPL'd code.

[1] http://mozillamemes.tumblr.com/post/25245896832/because-everybody-loves-ns-prefixed-classes
Attachment #635577 - Flags: feedback?(khuey)
Attachment #635577 - Flags: feedback?(jwalden+bmo)
One goal I have (which I haven't achieved yet) is to have an assertion like this:

   ASSERT_NSRESULT_SUCCEEDED(SomeXPCOMFunction(....));

which, when the assertion fails, will print out the symbolic name of the error code that SomeXPCOMFunction returned (e.g. "NS_ERROR_INVALID_ARGUMENT") instead of the number, or, when the name isn't know, to print out something like "NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_XPCONNECT, 123)" when the module name is known.

Another goal I have is for us to transform the GTest output into a format that can be automatically understood by our test automation (with TEST-UNEXPECTED-FAIL prefixes and whatnot). 

I haven't got everything working correct, but feedback on the approach would be much appreciated.
Attachment #635578 - Flags: feedback?(khuey)
Attachment #635578 - Flags: feedback?(jwalden+bmo)
I've used gtest extensively for Breakpad tests (and found it quite enjoyable to use), so I'm happy to provide feedback here as well.
Comment on attachment 635577 [details] [diff] [review]
WIP: Add gtest to the build in testing/gtest

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

Seems plausible, maybe.  I have no experience with GTest, tho, so I'm probably not the best person to give knowledgeable feedback here.
Attachment #635577 - Flags: feedback?(jwalden+bmo) → feedback+
Comment on attachment 635578 [details] [diff] [review]
WIP: Integrate GTest with TestHarness.h

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

Also seems vaguely plausible, same caveats applying as before.

::: testing/gtest/mozilla/GTestHarness.cpp
@@ +196,5 @@
> +  }
> +
> +  virtual void OnTestIterationStart(const UnitTest&, int) MOZ_OVERRIDE {}
> +  virtual void OnEnvironmentsSetUpStart(const UnitTest&) MOZ_OVERRIDE {}
> +  virtual void OnEnvironmentsSetUpEnd(const UnitTest&) MOZ_OVERRIDE {}

#include "mozilla/Attributes.h" in this file, then.

::: testing/gtest/mozilla/GTestHarness.h
@@ +44,5 @@
> +private:
> +  ScopedLogging mScopedLogging;
> +  nsIServiceManager* mServMgr;
> +
> +  XPCOMTest(const XPCOMTest &) MOZ_DELETE;

#include "mozilla/Attributes.h" here as well.  (Okay, so I guess the previous file bootlegs a little.  I'd kind of prefer to see another include for extra clarity.)

::: xpcom/tests/TestHarness.h
@@ +1,5 @@
>  /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
>  /* 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/. */
>  

Splinter doesn't show it, but this file got moved to xpcom/tests/TestHarnessBase.h, right?  (Or copied.  Not sure I care which, just making sure this include-guard isn't changing to a misleading name for no reason.)
Attachment #635578 - Flags: feedback?(jwalden+bmo) → feedback+
I got a tentative approval to put GTest into NSS so that we can use it for testing the NSS repository. So, I may redo this so that parts of it become a test for NSS, since NSS is already a dependency.

Also, some WebRTC components are using gtest internally, and parts of our WebRTC implementation use gtest. It may be worth getting them to use this patch once I have time to update it.
OS: Windows 7 → All
Hardware: x86_64 → All
Comment on attachment 635577 [details] [diff] [review]
WIP: Add gtest to the build in testing/gtest

I'd like to take a look at these soon, putting in my queue to remind myself. I'd like to convert all our in-tree C++ tests to use Google Test at some point, but that's a longer-term project.
Attachment #635577 - Flags: feedback?(ted.mielczarek)
Attachment #635578 - Flags: feedback?(ted.mielczarek)
One thing I would like to do if we add testing/gtest is remove the duplicate copies of gtest (from places like webrtc) and make them use the one central copy.
Any update on this? This feature is the best solution to test some of the new code I'm introducing and would significant help development by providing test driven development.
(In reply to Benoit Girard (:BenWa) from comment #9)
> Any update on this? This feature is the best solution to test some of the
> new code I'm introducing and would significant help development by providing
> test driven development.

Unfortunately, I will not have time in the next few weeks to work on it. But, at the time I posted the patches, they were working, IIRC.

Please try out the patches (which have probably bitrotted) and see how well they work for you. If you have questions where you need additional documentation, then let me know and I will attach patches that add the documentation.

In particular, it would be great to get somebody to use the formatting code and the Mozilla-specific ASSERT_/ENSURE_ macros I created to make sure they work in a reasonable way.

Also, now that nsresult is an enum, the error formatting code can be significantly improved.

These patches may or may not depend on bug 804441 to be fixed to avoid linking problems on Linux.
Comment on attachment 635577 [details] [diff] [review]
WIP: Add gtest to the build in testing/gtest

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

::: mobile/android/build.mk
@@ +9,5 @@
>  include $(topsrcdir)/toolkit/toolkit-tiers.mk
>  else
>  ifdef ENABLE_TESTS
>  tier_testharness_dirs += \
> +  testing/gtest \

This is kind of weird, I'm not sure why testing/mochitest is here. Maybe we can just move these all to toolkit-tiers.mk?

::: testing/gtest/Makefile.in
@@ +16,5 @@
> +LIBRARY_NAME = gtest_mozilla
> +FORCE_STATIC_LIB = 1
> +
> +# Force to build a static library, instead of a fake library, without
> +# installing it in dist/lib.

What's the motivation for building an actual static library?

::: toolkit/toolkit-tiers.mk
@@ +258,5 @@
>  tier_platform_dirs += testing/marionette
>  endif
>  
>  ifdef ENABLE_TESTS
> +tier_platform_dirs += testing/gtest

You have this in both tier_testharness and tier_platform, why is that?
Attachment #635577 - Flags: feedback?(ted) → feedback+
Comment on attachment 635578 [details] [diff] [review]
WIP: Integrate GTest with TestHarness.h

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

Overall this looks pretty good, I'm excited to have a useful C++ test harness in-tree!

::: config/rules.mk
@@ +112,5 @@
> +# whether they use GTEST_TESTS.
> +GTEST_INCLUDES = -I$(DEPTH)/testing/gtest/include \
> +		 $(NULL)	      
> +GTEST_LIBS = $(DEPTH)/testing/gtest/$(LIB_PREFIX)gtest.$(LIB_SUFFIX) \
> +	     $(NULL)

These should go in configure or config.mk.

@@ +114,5 @@
> +		 $(NULL)	      
> +GTEST_LIBS = $(DEPTH)/testing/gtest/$(LIB_PREFIX)gtest.$(LIB_SUFFIX) \
> +	     $(NULL)
> +
> +ifdef GTEST_TESTS

Longer-term I'd like to make this the only type of C++ unit tests, and convert all our old tests to use gtest. That's a lot more work though.

@@ +125,5 @@
> +check::
> +	@$(EXIT_ON_ERROR) \
> +	  for f in $(subst .cpp,$(BIN_SUFFIX),$(GTEST_TESTS)); do \
> +	    XPCOM_DEBUG_BREAK=stack-and-abort $(RUN_TEST_PROGRAM) $(DIST)/bin/$$f; \
> +	  done

Note that I changed this in bug 787176. Can we just combine this rule with the CPP_UNIT_TESTS rule above?

::: testing/gtest/Makefile.in
@@ +33,1 @@
>  	    $(NULL)

nit: our Makefile style is:
INCLUDES += \
  -I... \
  -I... \
  $(NULL)

with two-space indents.

::: testing/gtest/mozilla/GTestHarness.cpp
@@ +94,5 @@
> +//  const char * moduleName = errorModuleRegistration->Search(module);
> +//  if (moduleName) {    
> +//    os << moduleName;
> +//  } else {
> +//    os << module;

If you're not going to use this code it'd be better to have it removed than commented out.

@@ +189,5 @@
> +private:
> +  virtual void OnTestProgramStart(const UnitTest& unitTest) MOZ_OVERRIDE
> +  {
> +    cout << "TEST-INFO"
> +         << " | GTest tests "

Can you get the executable filename here? It would help to be able to differentiate between different test files.

::: xpcom/tests/Makefile.in
@@ +150,4 @@
>  export::
>  	$(NSINSTALL) -D $(DIST)/include/testing
>  	$(INSTALL) $(srcdir)/TestHarness.h $(DIST)/include/testing
> +	$(INSTALL) $(srcdir)/TestHarnessBase.h $(DIST)/include/testing

Can you convert this to use INSTALL_TARGETS while you're here?
Attachment #635578 - Flags: feedback?(ted) → feedback+
Attached patch Add GTest (obsolete) — Splinter Review
This worked on the previous patch but makes several improvements:
- Added license to about:license
- Created an option to output info using the mozilla test logging format (TEST-UNEXPECTED-FAIL | test-name ....)
- GTEST_*SRCS to build c/cpp/objC sources only if ENABLE_TESTS is defined
Attachment #712022 - Flags: review?(ted)
Attached patch Add GTest (obsolete) — Splinter Review
Fixed headers export
Attachment #712022 - Attachment is obsolete: true
Attachment #712022 - Flags: review?(ted)
Attachment #712025 - Flags: review?(ted)
Attached patch GTest Gfx Test (obsolete) — Splinter Review
Attachment #712027 - Flags: review?(ted)
Blocks: 839740
Attached patch Add GTest (Disable GTEST RTTI) (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=5ad2143e4af9
Attachment #712025 - Attachment is obsolete: true
Attachment #712025 - Flags: review?(ted)
Attachment #712068 - Flags: review?(ted)
Changed configure.in to only build on desktop.
https://tbpl.mozilla.org/?tree=Try&rev=97da0a86c8db
More build config fixes based on feedback from Ted:
https://tbpl.mozilla.org/?tree=Try&rev=e92a722d94ce
Add missing nullptr include for linux. Fix OS detection.
https://tbpl.mozilla.org/?tree=Try&rev=98df608322c2
Attached patch Add GTest (obsolete) — Splinter Review
Alright this patch is good to go. For now it will require --enable-gtest. Follow up will be to generate libxul-unittest.so and replace enable-gtest by enable-tests, use googlemock, add this to run on TBPL and perhaps get this running on mobile.
Assignee: bsmith → bgirard
Attachment #712068 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #712068 - Flags: review?(ted)
Comment on attachment 712806 [details] [diff] [review]
Add GTest

This patch is ready to land as-is as confirm by try.
Attachment #712806 - Flags: review?(ted)
Blocks: gmock
Comment on attachment 712806 [details] [diff] [review]
Add GTest

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

And this needs a mach command.

::: config/rules.mk
@@ +100,5 @@
> +CPPSRCS += $(GTEST_CPPSRCS)
> +endif
> +
> +ifdef GTEST_CCPPSRCS
> +CCSRCS += $(GTEST_CCSRCS)

There's no such thing as CCSRCS

::: testing/gtest/Makefile.in
@@ +4,5 @@
> +
> +# Avoid recursive make to avoid having to add files to the gtest/ subdirectory
> +# (which is third-party code), and to make the build faster.
> +
> +DEPTH = ../..

@DEPTH@

@@ +7,5 @@
> +
> +DEPTH = ../..
> +topsrcdir = @top_srcdir@
> +srcdir = @srcdir@
> +VPATH = @srcdir@ $(srcdir)/gtest/src $(srcdir)/mozilla

This is frowned upon, AIUI.

@@ +18,5 @@
> +EXPORT_LIBRARY = 1
> +LIBXUL_LIBRARY = 1
> +IS_COMPONENT = 1
> +# Unused var warnings in GTest
> +# FAIL_ON_WARNINGS = 1

Just leave it out, then.

@@ +20,5 @@
> +IS_COMPONENT = 1
> +# Unused var warnings in GTest
> +# FAIL_ON_WARNINGS = 1
> +
> +CPPSRCS = gtest-all.cc \

VARIABLE = \
  foo \
...

@@ +57,5 @@
> +  gtest/include/gtest/internal/gtest-tuple.h \
> +  gtest/include/gtest/internal/gtest-type-util.h \
> +  $(NULL)
> +
> +LOCAL_INCLUDES += -I$(srcdir)/gtest \

As above

::: testing/gtest/mozilla/GTestHarness.h.orig
@@ +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/. */
> +
> +#ifndef mozilla_testing_XPCOMTest_h
> +#define mozilla_testing_XPCOMTest_h

Don't check in .orig files.

::: testing/gtest/mozilla/GTestRunner.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/. */

Looks wrong. You want

/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

And then the comment from <http://www.mozilla.org/MPL/headers/>. Check all other files too.

@@ +16,5 @@
> +using ::testing::UnitTest;
> +
> +namespace mozilla {
> +
> +class MozillaPrinter : public EmptyTestEventListener {

{ on the next line

@@ +17,5 @@
> +
> +namespace mozilla {
> +
> +class MozillaPrinter : public EmptyTestEventListener {
> +private:

I'm somewhat surprised that this is private.

@@ +18,5 @@
> +namespace mozilla {
> +
> +class MozillaPrinter : public EmptyTestEventListener {
> +private:
> +  virtual void OnTestProgramStart(const UnitTest& /* unit_test */) {

{ on the next line. Use MOZ_OVERRIDE. (Also elsewhere)

@@ +19,5 @@
> +
> +class MozillaPrinter : public EmptyTestEventListener {
> +private:
> +  virtual void OnTestProgramStart(const UnitTest& /* unit_test */) {
> +    printf("TEST-INFO | GTest unit test starting\n");

When is this called? Once per run/file/test? Should have a pointer to documentation, at least.

@@ +21,5 @@
> +private:
> +  virtual void OnTestProgramStart(const UnitTest& /* unit_test */) {
> +    printf("TEST-INFO | GTest unit test starting\n");
> +  }
> +  virtual void OnTestProgramEnd(const UnitTest& unit_test) {

aUnitTest. When is this called?

@@ +22,5 @@
> +  virtual void OnTestProgramStart(const UnitTest& /* unit_test */) {
> +    printf("TEST-INFO | GTest unit test starting\n");
> +  }
> +  virtual void OnTestProgramEnd(const UnitTest& unit_test) {
> +    printf("TEST-%s | GTest unit test: %s\n",

TEST-foo | path | message

@@ +23,5 @@
> +    printf("TEST-INFO | GTest unit test starting\n");
> +  }
> +  virtual void OnTestProgramEnd(const UnitTest& unit_test) {
> +    printf("TEST-%s | GTest unit test: %s\n",
> +           unit_test.Passed() ? "INFO" : "UNEXPECTED-FAIL",

INFO should be PASS

@@ +24,5 @@
> +  }
> +  virtual void OnTestProgramEnd(const UnitTest& unit_test) {
> +    printf("TEST-%s | GTest unit test: %s\n",
> +           unit_test.Passed() ? "INFO" : "UNEXPECTED-FAIL",
> +           unit_test.Passed() ? "PASSED" : "FAILED");

No need for the upper-case

@@ +28,5 @@
> +           unit_test.Passed() ? "PASSED" : "FAILED");
> +  }
> +  virtual void OnTestStart(const TestInfo& test_info) {
> +    mTestInfo = &test_info;
> +    printf("TEST-INFO | %s.%s | running test\n",

TEST-START; no need for "| running test"

@@ +32,5 @@
> +    printf("TEST-INFO | %s.%s | running test\n",
> +        mTestInfo->test_case_name(), mTestInfo->name());
> +  }
> +  virtual void OnTestPartResult(const TestPartResult& test_part_result) {
> +    if (test_part_result.failed()) {

And otherwise: TEST-PASS

@@ +41,5 @@
> +    }
> +  }
> +  virtual void OnTestEnd(const TestInfo& test_info) {
> +    printf("TEST-%s | %s.%s | test completed (time: %llims)\n",
> +           test_info.result()->Passed() ? "PASSED": "UNEXPECTED-FAIL",

PASS, not PASSED

@@ +65,5 @@
> +
> +int RunGTest()
> +{
> +  int c = 0;
> +  ::testing::InitGoogleTest(&c, (char**)0);

static_cast<char**>(nullptr)?

And you've got a using statement above; why fully qualify this here?

@@ +67,5 @@
> +{
> +  int c = 0;
> +  ::testing::InitGoogleTest(&c, (char**)0);
> +
> +  if (getenv("MOZ_TBPL_PARSER") != nullptr)

Drop the "!= nullptr"; add {}s

::: toolkit/content/license.html
@@ +1332,5 @@
>      <h1><a id="google-bsd"></a>Google BSD License</h1>
>  
>      <p>This license applies to files in the directories
>      <span class="path">toolkit/crashreporter/google-breakpad/</span>,
> +    <span class="path">testing/gtest/gtest</span>,

It's not clear to me that we need this if we don't ship the code; do we? Ask gerv.

::: toolkit/library/Makefile.in
@@ +301,5 @@
>  endif
>  
>  STATIC_LIBS += thebes gl ycbcr
>  
> +ifdef MOZ_ENABLE_GTEST

Any reason not to enable this if ENABLE_TESTS is defined?

::: toolkit/xre/nsAppRunner.cpp
@@ +3212,5 @@
>      *aExitFlag = true;
>      return 0;
>    }
>  
> +  ar = CheckArg("unittest", true);

Not sure about this...

@@ +3215,5 @@
>  
> +  ar = CheckArg("unittest", true);
> +  if (ar == ARG_FOUND) {
> +#if MOZ_ENABLE_GTEST
> +    int result = mozilla::RunGTest();

I'm pretty sure we want to be able to run individual tests or groups of tests.
(In reply to :Ms2ger from comment #25)

Any comment I didn't respond to will be fixed in the next patch.

> Comment on attachment 712806 [details] [diff] [review]
> Add GTest
> 
> Review of attachment 712806 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> And this needs a mach command.
> 

I think I'm going to defer this to a follow up. There's no common for running a build with arguments and this is platform specific so I'm going to defer here.

> @@ +7,5 @@
> > +
> > +DEPTH = ../..
> > +topsrcdir = @top_srcdir@
> > +srcdir = @srcdir@
> > +VPATH = @srcdir@ $(srcdir)/gtest/src $(srcdir)/mozilla
> 
> This is frowned upon, AIUI.

I'm trying to avoid having recursive Makefile. What should I do instead? Specify the full src path?

> @@ +17,5 @@
> > +
> > +namespace mozilla {
> > +
> > +class MozillaPrinter : public EmptyTestEventListener {
> > +private:
> 
> I'm somewhat surprised that this is private.

I was too. I'm just following the example but it really doesn't need to be private AFAIK.

> @@ +22,5 @@
> > +  virtual void OnTestProgramStart(const UnitTest& /* unit_test */) {
> > +    printf("TEST-INFO | GTest unit test starting\n");
> > +  }
> > +  virtual void OnTestProgramEnd(const UnitTest& unit_test) {
> > +    printf("TEST-%s | GTest unit test: %s\n",
> 
> TEST-foo | path | message

Which path do you want here? This is before/after a test case is running. UnitTest is a bit of a poorly name class. It really should be named TestSuite/TestList.

> 
> @@ +24,5 @@
> > +  }
> > +  virtual void OnTestProgramEnd(const UnitTest& unit_test) {
> > +    printf("TEST-%s | GTest unit test: %s\n",
> > +           unit_test.Passed() ? "INFO" : "UNEXPECTED-FAIL",
> > +           unit_test.Passed() ? "PASSED" : "FAILED");
> 
> No need for the upper-case

Which uppercase? TEST/INFO/... are all uppercase on TBPL. GTest->gtest?

> ::: toolkit/content/license.html
> @@ +1332,5 @@
> >      <h1><a id="google-bsd"></a>Google BSD License</h1>
> >  
> >      <p>This license applies to files in the directories
> >      <span class="path">toolkit/crashreporter/google-breakpad/</span>,
> > +    <span class="path">testing/gtest/gtest</span>,
> 
> It's not clear to me that we need this if we don't ship the code; do we? Ask
> gerv.

good point.

> 
> ::: toolkit/library/Makefile.in
> @@ +301,5 @@
> >  endif
> >  
> >  STATIC_LIBS += thebes gl ycbcr
> >  
> > +ifdef MOZ_ENABLE_GTEST
> 
> Any reason not to enable this if ENABLE_TESTS is defined?

Yes, we build our release builds with ENABLE_TESTS which would mean shipping libxul with unit tests. As a follow up I will have the build generate both libxul and libxul-unittest.so.

> 
> ::: toolkit/xre/nsAppRunner.cpp
> @@ +3212,5 @@
> >      *aExitFlag = true;
> >      return 0;
> >    }
> >  
> > +  ar = CheckArg("unittest", true);
> 
> Not sure about this...

Perhaps "gtest"? Are you objecting to this way of running it? I found doing it this way to be very simple to run and debug. Much easier than using EXTRA_TEST_ARGS.

> 
> @@ +3215,5 @@
> >  
> > +  ar = CheckArg("unittest", true);
> > +  if (ar == ARG_FOUND) {
> > +#if MOZ_ENABLE_GTEST
> > +    int result = mozilla::RunGTest();
> 
> I'm pretty sure we want to be able to run individual tests or groups of
> tests.

For now tests can be selected and even ran in parallel using GTest environment variables but I don't mind adding some addition command args.
http://code.google.com/p/googletest/wiki/V1_6_AdvancedGuide#Running_a_Subset_of_the_Tests
(In reply to Benoit Girard (:BenWa) from comment #26)
> (In reply to :Ms2ger from comment #25)
> 
> Any comment I didn't respond to will be fixed in the next patch.
> 
> > Comment on attachment 712806 [details] [diff] [review]
> > Add GTest
> > 
> > Review of attachment 712806 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > And this needs a mach command.
> > 
> 
> I think I'm going to defer this to a follow up. There's no common for
> running a build with arguments and this is platform specific so I'm going to
> defer here.
> 
> > @@ +7,5 @@
> > > +
> > > +DEPTH = ../..
> > > +topsrcdir = @top_srcdir@
> > > +srcdir = @srcdir@
> > > +VPATH = @srcdir@ $(srcdir)/gtest/src $(srcdir)/mozilla
> > 
> > This is frowned upon, AIUI.
> 
> I'm trying to avoid having recursive Makefile. What should I do instead?
> Specify the full src path?

I think so, yes. Ted?

> > @@ +17,5 @@
> > > +
> > > +namespace mozilla {
> > > +
> > > +class MozillaPrinter : public EmptyTestEventListener {
> > > +private:
> > 
> > I'm somewhat surprised that this is private.
> 
> I was too. I'm just following the example but it really doesn't need to be
> private AFAIK.
> 
> > @@ +22,5 @@
> > > +  virtual void OnTestProgramStart(const UnitTest& /* unit_test */) {
> > > +    printf("TEST-INFO | GTest unit test starting\n");
> > > +  }
> > > +  virtual void OnTestProgramEnd(const UnitTest& unit_test) {
> > > +    printf("TEST-%s | GTest unit test: %s\n",
> > 
> > TEST-foo | path | message
> 
> Which path do you want here? This is before/after a test case is running.
> UnitTest is a bit of a poorly name class. It really should be named
> TestSuite/TestList.

Ah, hmm. So wouldn't we already have unexpected-fails from the individual tests at this point?

> > 
> > @@ +24,5 @@
> > > +  }
> > > +  virtual void OnTestProgramEnd(const UnitTest& unit_test) {
> > > +    printf("TEST-%s | GTest unit test: %s\n",
> > > +           unit_test.Passed() ? "INFO" : "UNEXPECTED-FAIL",
> > > +           unit_test.Passed() ? "PASSED" : "FAILED");
> > 
> > No need for the upper-case
> 
> Which uppercase? TEST/INFO/... are all uppercase on TBPL. GTest->gtest?

"PASSED"/"FAILED".

> > ::: toolkit/content/license.html
> > @@ +1332,5 @@
> > >      <h1><a id="google-bsd"></a>Google BSD License</h1>
> > >  
> > >      <p>This license applies to files in the directories
> > >      <span class="path">toolkit/crashreporter/google-breakpad/</span>,
> > > +    <span class="path">testing/gtest/gtest</span>,
> > 
> > It's not clear to me that we need this if we don't ship the code; do we? Ask
> > gerv.
> 
> good point.
> 
> > 
> > ::: toolkit/library/Makefile.in
> > @@ +301,5 @@
> > >  endif
> > >  
> > >  STATIC_LIBS += thebes gl ycbcr
> > >  
> > > +ifdef MOZ_ENABLE_GTEST
> > 
> > Any reason not to enable this if ENABLE_TESTS is defined?
> 
> Yes, we build our release builds with ENABLE_TESTS which would mean shipping
> libxul with unit tests. As a follow up I will have the build generate both
> libxul and libxul-unittest.so.

That sounds nicer, I think... Except for linking libxul twice. But I don't understand the linking issues, so I'll leave that to ted :)

> > 
> > ::: toolkit/xre/nsAppRunner.cpp
> > @@ +3212,5 @@
> > >      *aExitFlag = true;
> > >      return 0;
> > >    }
> > >  
> > > +  ar = CheckArg("unittest", true);
> > 
> > Not sure about this...
> 
> Perhaps "gtest"? Are you objecting to this way of running it? I found doing
> it this way to be very simple to run and debug. Much easier than using
> EXTRA_TEST_ARGS.

Adding an argument to the executable... Anyway, I'll leave this to ted.

> > 
> > @@ +3215,5 @@
> > >  
> > > +  ar = CheckArg("unittest", true);
> > > +  if (ar == ARG_FOUND) {
> > > +#if MOZ_ENABLE_GTEST
> > > +    int result = mozilla::RunGTest();
> > 
> > I'm pretty sure we want to be able to run individual tests or groups of
> > tests.
> 
> For now tests can be selected and even ran in parallel using GTest
> environment variables but I don't mind adding some addition command args.
> http://code.google.com/p/googletest/wiki/
> V1_6_AdvancedGuide#Running_a_Subset_of_the_Tests

I see... I'm not a big fan of environment variables, but that works. :)
(In reply to :Ms2ger from comment #28)
> > > @@ +22,5 @@
> > > > +  virtual void OnTestProgramStart(const UnitTest& /* unit_test */) {
> > > > +    printf("TEST-INFO | GTest unit test starting\n");
> > > > +  }
> > > > +  virtual void OnTestProgramEnd(const UnitTest& unit_test) {
> > > > +    printf("TEST-%s | GTest unit test: %s\n",
> > > 
> > > TEST-foo | path | message
> > 
> > Which path do you want here? This is before/after a test case is running.
> > UnitTest is a bit of a poorly name class. It really should be named
> > TestSuite/TestList.
> 
> Ah, hmm. So wouldn't we already have unexpected-fails from the individual
> tests at this point?
> 

This is after *all* the tests have ran. I guess at this point gtest probably has a list of which tests have failed but I don't think it's worth printing a summary.


> ::: testing/gtest/mozilla/GTestRunner.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/. */
> 
> Looks wrong. You want
> 
> /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
> */
> 
> And then the comment from <http://www.mozilla.org/MPL/headers/>. Check all
> other files too.
> 

Are you asking that I put the vim line in a separate /* */ comment block?
I'm checking in gtest but we wont be shipping it part of the release builds. Should I add an entry to about:license or not?
Flags: needinfo?(gerv)
One thing I've noticed in gtest in WebRTC-land (which, admittedly, is a bit different than here, since that code is inside libxul and has required some fun hackery to make run with gtest) is that the threading setup is somewhat different than in Firefox.  

The way I've seen this so far is that we have to ifdef out all calls to NS_IsMainThread when tests are running, I'm not sure if there are other meaningful differences.  It would be nice if this could be avoided in the general case.
Right now unit test are set to run before we initialize any services. This means that NS_IsMainThread wont work similar to WebRTC-land. I think the idea is that gtest will be restricted to running test on individual classes that don't require the rest of mozilla to be running but wont be suitable to write integration test that require services to be initialized.

I imagine this will be a big pain because even trivial classes will want to use calls such as NS_IsMainThread that would otherwise be candidates for unit tests but I have no solution to propose at this time. I know for graphics there's still significant testing that can be done with this framework such as our 2D drawing library that are standalone, our utility classes etc...
Attached patch Add GTest (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=32d0bbeeb9ee
Attachment #712806 - Attachment is obsolete: true
Attachment #712806 - Flags: review?(ted)
Attachment #712949 - Flags: review?(ted)
(In reply to Benoit Girard (:BenWa) from comment #29)
> (In reply to :Ms2ger from comment #28)
> > > > @@ +22,5 @@
> > > > > +  virtual void OnTestProgramStart(const UnitTest& /* unit_test */) {
> > > > > +    printf("TEST-INFO | GTest unit test starting\n");
> > > > > +  }
> > > > > +  virtual void OnTestProgramEnd(const UnitTest& unit_test) {
> > > > > +    printf("TEST-%s | GTest unit test: %s\n",
> > > > 
> > > > TEST-foo | path | message
> > > 
> > > Which path do you want here? This is before/after a test case is running.
> > > UnitTest is a bit of a poorly name class. It really should be named
> > > TestSuite/TestList.
> > 
> > Ah, hmm. So wouldn't we already have unexpected-fails from the individual
> > tests at this point?
> > 
> 
> This is after *all* the tests have ran. I guess at this point gtest probably
> has a list of which tests have failed but I don't think it's worth printing
> a summary.

Do we want to print anything, then?

> > ::: testing/gtest/mozilla/GTestRunner.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/. */
> > 
> > Looks wrong. You want
> > 
> > /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
> > */
> > 
> > And then the comment from <http://www.mozilla.org/MPL/headers/>. Check all
> > other files too.
> > 
> 
> Are you asking that I put the vim line in a separate /* */ comment block?

Yes, and fix the double *s.
(In reply to Benoit Girard (:BenWa) from comment #23)
> Alright this patch is good to go. For now it will require --enable-gtest.
> Follow up will be to generate libxul-unittest.so and replace enable-gtest by
> enable-tests, use googlemock, add this to run on TBPL and perhaps get this
> running on mobile.

I am sure that the double-linking is useful and necessary for the specific tests that you want to write. However, for the PSM tests that I was originally working on this for, it was more than sufficient to build GTest in --enable-gtest and then use the GTestHarness.h framework I created to write "normal" C++ unit test executables--where the test programs can only use the external API, not the internal API. This way, we didn't have to do any double-linking or change how the TBPL integration works; we just had to export a few key functions from PSM that we were trying to test. I think it is very important that we keep this capability, unles the TBPL integration is going to get done right away, because that capability allows GTest-based tests to run on TBPL right away.
(In reply to Benoit Girard (:BenWa) from comment #32)
> Right now unit test are set to run before we initialize any services. This
> means that NS_IsMainThread wont work similar to WebRTC-land. 

Maybe providing a template, mix-in, or method that does the initialization for tests that need it?

Or having the tests run later?
(In reply to Dan Mosedale (:dmose) from comment #36)
> (In reply to Benoit Girard (:BenWa) from comment #32)
> > Right now unit test are set to run before we initialize any services. This
> > means that NS_IsMainThread wont work similar to WebRTC-land. 
> 
> Maybe providing a template, mix-in, or method that does the initialization
> for tests that need it?
> 
> Or having the tests run later?

Perhaps, but for now I really don't want to increase the scope and de-rail the landing of this. This patch doesn't do anything to make doing this any harder. I'll leave this task up for grabs for people writing such tests.
Sounds like an excellent idea.  I just thought it would be helpful to get a sense of whether there were known issues that would make that impossible.
If something is not shipped in Firefox, it doesn't need to be in about:license.

Gerv
Flags: needinfo?(gerv)
(In reply to Dan Mosedale (:dmose) from comment #38)
> Sounds like an excellent idea.  I just thought it would be helpful to get a
> sense of whether there were known issues that would make that impossible.

See the patch "WIP: Integrate GTest with TestHarness.h" attached to this bug. In my local tree, I have patches that use this framework to run tests that depend on XPCOM. Not sure what, if any, changes will be necessary.
I'm going to file a follow up bug for that once this lands.
Very exciting -- nice work, guys!  Looking forward to have nicer C++ unit testing ability.
Comment on attachment 712949 [details] [diff] [review]
Add GTest

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

Just a few fiddly things here. Obviously I didn't look at any of the imported code.

::: config/rules.mk
@@ +99,5 @@
> +ifdef GTEST_CPPSRCS
> +CPPSRCS += $(GTEST_CPPSRCS)
> +endif
> +
> +ifdef GTEST_CCPPSRCS

What is CCPPSRCS here?

::: configure.in
@@ +6342,5 @@
> +        AC_DEFINE_UNQUOTED(GTEST_HAS_RTTI, 0)
> +        AC_SUBST(MOZ_ENABLE_GTEST)
> +        AC_SUBST(GTEST_HAS_RTTI)
> +    fi
> +fi

This should AC_MSG_ERROR if you try to --enable-gtest on an unsupported platform.

::: testing/gtest/Makefile.in
@@ +10,5 @@
> +srcdir = @srcdir@
> +VPATH = \
> +  $(srcdir) \
> +	$(srcdir)/gtest/src \
> +	$(srcdir)/mozilla \

You have extraneous tabs here.

@@ +25,5 @@
> +
> +CPPSRCS = \
> +  gtest-all.cc \
> +  GTestRunner.cpp \
> +  $(NULL)

Does this build if you just include the subdir names instead of using VPATH? (I can't remember if I ever fixed that or if I just worked around it.)

::: testing/gtest/mozilla/GTestRunner.cpp
@@ +22,5 @@
> +class MozillaPrinter : public EmptyTestEventListener
> +{
> +public:
> +  virtual void OnTestProgramStart(const UnitTest& /* aUnitTest */) MOZ_OVERRIDE {
> +    printf("TEST-INFO | GTest unit test starting\n");

Does UnitTest have a name you could put in the output here?

@@ +25,5 @@
> +  virtual void OnTestProgramStart(const UnitTest& /* aUnitTest */) MOZ_OVERRIDE {
> +    printf("TEST-INFO | GTest unit test starting\n");
> +  }
> +  virtual void OnTestProgramEnd(const UnitTest& aUnitTest) MOZ_OVERRIDE {
> +    printf("TEST-%s | GTest unit test: %s\n",

also here

::: toolkit/content/license.html
@@ +1332,5 @@
>      <h1><a id="google-bsd"></a>Google BSD License</h1>
>  
>      <p>This license applies to files in the directories
>      <span class="path">toolkit/crashreporter/google-breakpad/</span>,
> +    <span class="path">testing/gtest/gtest</span>,

As noted above, not necessary.
Attachment #712949 - Flags: review?(ted) → review+
Comment on attachment 712027 [details] [diff] [review]
GTest Gfx Test

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

I don't know anything about the test code in question here, do you want a gfx/layers peer to review that?

::: gfx/layers/Makefile.in
@@ +84,5 @@
>          $(NULL)
>  
> +GTEST_CPPSRCS = \
> +        TestTiledLayerBuffer.cpp \
> +        $(NULL)

I really hate this non-2-space indentation, but I guess we're going to replace this all with moz.build files soon anyway.

::: gfx/layers/TestTiledLayerBuffer.cpp
@@ +1,1 @@
> +#include "TiledLayerBuffer.h"

You probably need a license header here. I think you can use public domain for test files if you want, per:
http://www.mozilla.org/MPL/license-policy.html
Attachment #712027 - Flags: review?(ted) → review+
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #43)
> Comment on attachment 712949 [details] [diff] [review]
> Add GTest
> 
> Review of attachment 712949 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just a few fiddly things here. Obviously I didn't look at any of the
> imported code.
> 
> ::: config/rules.mk
> @@ +99,5 @@
> > +ifdef GTEST_CPPSRCS
> > +CPPSRCS += $(GTEST_CPPSRCS)
> > +endif
> > +
> > +ifdef GTEST_CCPPSRCS
> 
> What is CCPPSRCS here?

The idea is when you specify GTEST_CPPSRCS if you're building with GTest they get treated as CPPSRCS and if you're building without GTest they go nowhere and get ignored.


> 
> Does this build if you just include the subdir names instead of using VPATH?
> (I can't remember if I ever fixed that or if I just worked around it.)

I tried this. It fails to compile because it compiles say subdir/a.cpp to subdir/a.o and complains subdir doesn't exist in the objdir or something like that.

> 
> ::: testing/gtest/mozilla/GTestRunner.cpp
> @@ +22,5 @@
> > +class MozillaPrinter : public EmptyTestEventListener
> > +{
> > +public:
> > +  virtual void OnTestProgramStart(const UnitTest& /* aUnitTest */) MOZ_OVERRIDE {
> > +    printf("TEST-INFO | GTest unit test starting\n");
> 
> Does UnitTest have a name you could put in the output here?

I'm not sure I follow. This is at the start of the global GTest run so at this point where about to starting running ALL of our tests, not a particular test. All declared GTest will run here (unless you request a subset of test using an environment variable).
(In reply to Benoit Girard (:BenWa) from comment #45)

> > What is CCPPSRCS here?
> 
> The idea is when you specify GTEST_CPPSRCS if you're building with GTest
> they get treated as CPPSRCS and if you're building without GTest they go
> nowhere and get ignored.

Sorry, maybe I wasn't clear: that looks like a typo.

> 
> 
> > 
> > Does this build if you just include the subdir names instead of using VPATH?
> > (I can't remember if I ever fixed that or if I just worked around it.)
> 
> I tried this. It fails to compile because it compiles say subdir/a.cpp to
> subdir/a.o and complains subdir doesn't exist in the objdir or something
> like that.

Ah, right. I have a workaround in the gyp->Makefile code, we could look into porting that into rules.mk:
http://mxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/tools/gyp/pylib/gyp/generator/mozmake.py#79

> > Does UnitTest have a name you could put in the output here?
> 
> I'm not sure I follow. This is at the start of the global GTest run so at
> this point where about to starting running ALL of our tests, not a
> particular test. All declared GTest will run here (unless you request a
> subset of test using an environment variable).

Ah, I thought maybe this was per-test-fixture.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #46)
> (In reply to Benoit Girard (:BenWa) from comment #45)
> 
> > > What is CCPPSRCS here?
> > 
> > The idea is when you specify GTEST_CPPSRCS if you're building with GTest
> > they get treated as CPPSRCS and if you're building without GTest they go
> > nowhere and get ignored.
> 
> Sorry, maybe I wasn't clear: that looks like a typo.

Right, fixed.

> 
> > 
> > 
> > > 
> > > Does this build if you just include the subdir names instead of using VPATH?
> > > (I can't remember if I ever fixed that or if I just worked around it.)
> > 
> > I tried this. It fails to compile because it compiles say subdir/a.cpp to
> > subdir/a.o and complains subdir doesn't exist in the objdir or something
> > like that.
> 
> Ah, right. I have a workaround in the gyp->Makefile code, we could look into
> porting that into rules.mk:
> http://mxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/tools/gyp/
> pylib/gyp/generator/mozmake.py#79
> 

I'd be happy to clean up this Makefile.in when that lands if you ping me. But in the mean time I'm going to land this today :)
Attachment #712949 - Attachment is obsolete: true
Attachment #717276 - Flags: review+
Attached patch GTest Gfx Test, r=ted (obsolete) — Splinter Review
Attachment #712027 - Attachment is obsolete: true
Attachment #717286 - Flags: review?(bjacob)
Changed the ordering as request by bjacob
Attachment #717286 - Attachment is obsolete: true
Attachment #717286 - Flags: review?(bjacob)
Attachment #717294 - Flags: review?(bjacob)
Inbound is close so I'm pushing to try for now:
https://tbpl.mozilla.org/?tree=Try&rev=bdb28bb8fc04
Blocks: 844279
Attachment #717294 - Flags: review?(bjacob) → review+
Blocks: 844288
Blocks: 844292
https://hg.mozilla.org/mozilla-central/rev/2e3a491f3631
https://hg.mozilla.org/mozilla-central/rev/351462147f91
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
I wrote the developer documentation for using GTest with mozilla:
https://developer.mozilla.org/en-US/docs/GTest

Please give me feedback. I'll blog and post to dev.platform next week.
Blocks: 844630
Blocks: 844852
Blocks: 844869
Attachment #635577 - Attachment is obsolete: true
Attachment #635577 - Flags: feedback?(khuey)
Attachment #635578 - Attachment is obsolete: true
Attachment #635578 - Flags: feedback?(khuey)
Blocks: 846356
Blocks: 853867
You need to log in before you can comment on or make changes to this bug.