Page MenuHomeFreeBSD

Import GoogleTest 1.8.1 into the vendor tree under `^/google/googletest/dist`
Needs ReviewPublic

Authored by ngie on Dec 20 2018, 7:38 PM.

Details

Reviewers
emaste
jtl
Summary

GoogleTest is a widely used opensource C++ test framework, licensed under a
BSD 3-clause license. It fits best in the realm of doing functional/whitebox
testing, similar to ATF's C++ library. However, it has additional functionality
such as per-testcase setup fixtures, class level setup and teardown fixtures,
and a lot more functional/syntactic goodness.

In addition to large corporations adopting GoogleTest as their defacto C++ test
library (Facebook, Google, etc), many opensource projects have adopted
GoogleTest, e.g., the Capsicum project, Chrome, etc.

The goal for importing this is to enable testing with zfsd, integrate GoogleTest
into Kyua (in the medium term), and look at replacing atf-c++-api(3) (over the
longterm).

This is the final version that will support a pre-C++-11 compiler. As such, this
test framework will not be available to gcc 4.2.1, similar to ATF's C++ library.

A subsequent set of commits will:

  1. Tag ^/google/googletest/dist as ^/google/googletest/1.8.1 using svn cp ^/google/googletest/dist ^/google/googletest/1.8.1.
  2. Import this code into ^/head and integrate it into the build for all applicable platforms and C++ compile toolchains.

The import was done via the following command pipeline on OSX:

curl -L https://github.com/google/googletest/archive/release-1.8.1.tar.gz | tar --strip-components=1 -xvzf - -C dist/
Test Plan

N/A

Diff Detail

Repository
rS FreeBSD src repository
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 21662
Build 20951: arc lint + arc unit

Event Timeline

ngie created this revision.Dec 20 2018, 7:38 PM
ngie edited the summary of this revision. (Show Details)Dec 20 2018, 7:40 PM
jtl added a comment.Fri, Jan 11, 7:42 PM

I'm OK with importing the code as an option for developers to use. I'd be wary of specifying that there is a "plan" to convert existing test cases unless there is general buy in to do that.

Just make sure you follow the procedure in the committer's guide (which is what you listed).

Would appreciate @emaste double-checking my judgment on this.

The goal for importing this is to enable test suites via Capsicum (in the short
term), integrate GoogleTest into Kyua (in the medium term), and look at
replacing

Note that the capsicum-test project (https://github.com/google/capsicum-test) includes an embedded copy of googletest - do you plan to point it at a shared googletest instead? I'd like to figure out a bit more about how we'll actually deploy this.

I'm OK with importing the code as an option for developers to use. I'd be wary of specifying that there is a "plan" to convert existing test cases unless there is general buy in to do that.

Yes I'd agree with that - we'd import this for new uses cases without expecting that folks are going to convert over any existing ones (at least prior to further discussion/work).

ngie added a comment.Fri, Jan 11, 11:34 PM

The goal for importing this is to enable test suites via Capsicum (in the short
term), integrate GoogleTest into Kyua (in the medium term), and look at
replacing

Note that the capsicum-test project (https://github.com/google/capsicum-test) includes an embedded copy of googletest - do you plan to point it at a shared googletest instead? I'd like to figure out a bit more about how we'll actually deploy this.

Wow... that's a really old version of Googletest :(...

Part of the benefit of using a non-embedded copy of Googletest is that it would allow us to incorporate new features and fixes from upstream and reduce redundant copies of Googletest (say a common bug pops up in Googletest -- it only needs to be fixed once, instead of in n affected copies).

Well.. here goes... I guess I'll fork and fix those tests since it seems like low-hanging fruit (will CC you on the PRs).

ngie edited the summary of this revision. (Show Details)Sat, Jan 12, 12:01 AM
ngie added a subscriber: asomers.EditedSat, Jan 12, 12:04 AM

I'm OK with importing the code as an option for developers to use. I'd be wary of specifying that there is a "plan" to convert existing test cases unless there is general buy in to do that.

Yes I'd agree with that - we'd import this for new uses cases without expecting that folks are going to convert over any existing ones (at least prior to further discussion/work).

@asomers, @jmmv, and I spoke about this to some degree in an offline message thread.

Part of the reason why I haven't been pushing incredibly hard for parity with the C++ APIs is that it's a dead end. @jmmv hasn't been pushing for it and after discussing things at some length, I agree with his assessment. @jmmv and I believe that googletest is a better path forward to follow, so I want to pursue that. Having used it at Facebook (and seen its usefulness), as well as having seen it applied to a number of opensource projects, I think it would be a good path forward to have and leverage for future work in the FreeBSD project.

FWIW our tree already has some tests that require C++-11. See lib/libc/tests/stdlib/Makefile. So importing an old version of googletest may not be worthwhile if it means forfeiting important features. Also, you may not have noticed, but there's already one googletest program in base, in cddl/usr.sbin/zfsd/tests. It's been there for years, but never hooked up to the build.

ngie added a comment.EditedSun, Jan 13, 2:52 AM

FWIW our tree already has some tests that require C++-11. See lib/libc/tests/stdlib/Makefile.

That should be easily expressible using googletest instead of atf-c++(3).

So importing an old version of googletest may not be worthwhile if it means forfeiting important features.

1.8.1 is the latest release though:

$ pkg search googletest
googletest-1.8.1_1             Framework for writing C++ tests on a variety of platforms

Also, you may not have noticed, but there's already one googletest program in base, in cddl/usr.sbin/zfsd/tests. It's been there for years, but never hooked up to the build.

Actually, googletest is technically not installed (it's looking for the library in ports). From cddl/usr.sbin/zfsd/tests/Makefile:

20 # Googletest options
21 INCFLAGS+=      -I${LOCALBASE}/include -D_THREAD_SAFE -pthread
22 LDFLAGS.zfsd_unittest+= -L${LOCALBASE}/lib -D_THREAD_SAFE -pthread
23 LDADD.zfsd_unittest+=           ${LOCALBASE}/lib/libgtest.a
24 
25 # GoogleMock options
26 LDADD.zfsd_unittest+= ${LOCALBASE}/lib/libgmock.a ${LOCALBASE}/lib/libgmock_main.a

... or were you confirming the fact that there's actually a supporting test in base that could leverage googletest when imported?

ngie edited the summary of this revision. (Show Details)Sun, Jan 13, 2:55 AM
ngie edited the summary of this revision. (Show Details)