Closed
Bug 767231
(gtest)
Opened 12 years ago
Closed 12 years ago
Add ability to write compiled-code unit tests using the GTest framework
Categories
(Testing :: General, defect)
Testing
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla22
People
(Reporter: briansmith, Assigned: BenWa)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 9 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bjacob
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•12 years ago
|
||
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)
Reporter | ||
Comment 2•12 years ago
|
||
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)
Comment 3•12 years ago
|
||
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 4•12 years ago
|
||
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 5•12 years ago
|
||
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+
Reporter | ||
Comment 6•12 years ago
|
||
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 7•12 years ago
|
||
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)
Updated•12 years ago
|
Attachment #635578 -
Flags: feedback?(ted.mielczarek)
Comment 8•12 years ago
|
||
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.
Assignee | ||
Comment 9•12 years ago
|
||
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.
Reporter | ||
Comment 10•12 years ago
|
||
(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 11•12 years ago
|
||
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 12•12 years ago
|
||
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+
Assignee | ||
Comment 13•12 years ago
|
||
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)
Assignee | ||
Comment 14•12 years ago
|
||
Fixed headers export
Attachment #712022 -
Attachment is obsolete: true
Attachment #712022 -
Flags: review?(ted)
Attachment #712025 -
Flags: review?(ted)
Assignee | ||
Comment 15•12 years ago
|
||
Attachment #712027 -
Flags: review?(ted)
Assignee | ||
Comment 16•12 years ago
|
||
Assignee | ||
Comment 17•12 years ago
|
||
Assignee | ||
Comment 18•12 years ago
|
||
Attachment #712025 -
Attachment is obsolete: true
Attachment #712025 -
Flags: review?(ted)
Attachment #712068 -
Flags: review?(ted)
Assignee | ||
Comment 19•12 years ago
|
||
Assignee | ||
Comment 20•12 years ago
|
||
Changed configure.in to only build on desktop.
https://tbpl.mozilla.org/?tree=Try&rev=97da0a86c8db
Assignee | ||
Comment 21•12 years ago
|
||
More build config fixes based on feedback from Ted:
https://tbpl.mozilla.org/?tree=Try&rev=e92a722d94ce
Assignee | ||
Comment 22•12 years ago
|
||
Add missing nullptr include for linux. Fix OS detection.
https://tbpl.mozilla.org/?tree=Try&rev=98df608322c2
Assignee | ||
Comment 23•12 years ago
|
||
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)
Assignee | ||
Comment 24•12 years ago
|
||
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)
Comment 25•12 years ago
|
||
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.
Assignee | ||
Comment 26•12 years ago
|
||
(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
Comment 28•12 years ago
|
||
(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. :)
Assignee | ||
Comment 29•12 years ago
|
||
(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?
Assignee | ||
Comment 30•12 years ago
|
||
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)
Comment 31•12 years ago
|
||
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.
Assignee | ||
Comment 32•12 years ago
|
||
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...
Assignee | ||
Comment 33•12 years ago
|
||
Attachment #712806 -
Attachment is obsolete: true
Attachment #712806 -
Flags: review?(ted)
Attachment #712949 -
Flags: review?(ted)
Comment 34•12 years ago
|
||
(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.
Reporter | ||
Comment 35•12 years ago
|
||
(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.
Comment 36•12 years ago
|
||
(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?
Assignee | ||
Comment 37•12 years ago
|
||
(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.
Comment 38•12 years ago
|
||
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.
Comment 39•12 years ago
|
||
If something is not shipped in Firefox, it doesn't need to be in about:license.
Gerv
Flags: needinfo?(gerv)
Reporter | ||
Comment 40•12 years ago
|
||
(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.
Assignee | ||
Comment 41•12 years ago
|
||
I'm going to file a follow up bug for that once this lands.
Comment 42•12 years ago
|
||
Very exciting -- nice work, guys! Looking forward to have nicer C++ unit testing ability.
Comment 43•12 years ago
|
||
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 44•12 years ago
|
||
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+
Assignee | ||
Comment 45•12 years ago
|
||
(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).
Comment 46•12 years ago
|
||
(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.
Assignee | ||
Comment 47•12 years ago
|
||
(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 :)
Assignee | ||
Comment 48•12 years ago
|
||
Attachment #712949 -
Attachment is obsolete: true
Attachment #717276 -
Flags: review+
Assignee | ||
Comment 49•12 years ago
|
||
Attachment #712027 -
Attachment is obsolete: true
Attachment #717286 -
Flags: review?(bjacob)
Assignee | ||
Comment 50•12 years ago
|
||
Changed the ordering as request by bjacob
Attachment #717286 -
Attachment is obsolete: true
Attachment #717286 -
Flags: review?(bjacob)
Attachment #717294 -
Flags: review?(bjacob)
Assignee | ||
Comment 51•12 years ago
|
||
Inbound is close so I'm pushing to try for now:
https://tbpl.mozilla.org/?tree=Try&rev=bdb28bb8fc04
Updated•12 years ago
|
Attachment #717294 -
Flags: review?(bjacob) → review+
Assignee | ||
Comment 52•12 years ago
|
||
Comment 53•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2e3a491f3631
https://hg.mozilla.org/mozilla-central/rev/351462147f91
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Assignee | ||
Comment 54•12 years ago
|
||
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.
Attachment #635577 -
Attachment is obsolete: true
Attachment #635577 -
Flags: feedback?(khuey)
Attachment #635578 -
Attachment is obsolete: true
Attachment #635578 -
Flags: feedback?(khuey)
You need to log in
before you can comment on or make changes to this bug.
Description
•