Page MenuHomeFreeBSD

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

Authored by ngie on Dec 20 2018, 7:38 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 21, 8:11 PM
Unknown Object (File)
Tue, Nov 19, 10:35 PM
Unknown Object (File)
Tue, Nov 19, 10:34 PM
Unknown Object (File)
Mon, Nov 18, 6:54 PM
Unknown Object (File)
Mon, Nov 18, 4:37 AM
Unknown Object (File)
Tue, Nov 12, 2:11 PM
Unknown Object (File)
Fri, Nov 1, 12:07 AM
Unknown Object (File)
Thu, Oct 31, 11:06 PM

Details

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 and integrate
googletest into Kyra.

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 - subversion
Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 21662
Build 20951: arc lint + arc unit

Event Timeline

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).

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).

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.

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)

@imp, @brooks you seem to be possible curators of contrib/; does google/googletest/ seem like the right location? Seems sensible to me.

@imp, @brooks you seem to be possible curators of contrib/; does google/googletest/ seem like the right location? Seems sensible to me.

Yeah, that matches existing practice.

This revision is now accepted and ready to land.Feb 13 2019, 2:02 AM