Handle GTEST_SKIP() when calling `Environment::SetUp()` #2203

ngie-eign · 2019-03-30T04:49:25Z

gtest prior to this change would completely ignore GTEST_SKIP() if
called in Environment::SetUp(), instead of bailing out early, unlike
Test::SetUp(), which would cause the tests themselves to be skipped.
The only way (prior to this change) to skip the tests would be to
trigger a fatal error via GTEST_FAIL().

Desirable behavior, in this case, when dealing with
Environment::SetUp() is to check for prerequisites on a system
(example, kernel supports a particular featureset, e.g., capsicum), and
skip the tests. The alternatives prior to this change would be
undesirable:

Failing sends the wrong message to the test user, as the result of the
tests is indeterminate, not failed.
Having to add per-test class abstractions that override SetUp() to
test for the capsicum feature set, then skip all of the tests in their
respective SetUp fixtures, would be a lot of human and computational
work; checking for the feature would need to be done for all of the
tests, instead of once for all of the tests.

For those reasons, making Environment::SetUp() handle GTEST_SKIP(),
by not executing the testcases, is the most desirable solution.

In order to properly diagnose what happened when running the tests if
they are skipped, print out the diagnostics in an ad hoc manner.

Update the documentation to note this change and integrate a new test,
gtest_skip_in_environment_setup_test, into the test suite.

This change addresses #2189.

Signed-off-by: Enji Cooper yaneurabeya@gmail.com

gtest prior to this change would completely ignore `GTEST_SKIP()` if called in `Environment::SetUp()`, instead of bailing out early, unlike `Test::SetUp()`, which would cause the tests themselves to be skipped. The only way (prior to this change) to skip the tests would be to trigger a fatal error via `GTEST_FAIL()`. Desirable behavior, in this case, when dealing with `Environment::SetUp()` is to check for prerequisites on a system (example, kernel supports a particular featureset, e.g., capsicum), and skip the tests. The alternatives prior to this change would be undesirable: - Failing sends the wrong message to the test user, as the result of the tests is indeterminate, not failed. - Having to add per-test class abstractions that override `SetUp()` to test for the capsicum feature set, then skip all of the tests in their respective SetUp fixtures, would be a lot of human and computational work; checking for the feature would need to be done for all of the tests, instead of once for all of the tests. For those reasons, making `Environment::SetUp()` handle `GTEST_SKIP()`, by not executing the testcases, is the most desirable solution. In order to properly diagnose what happened when running the tests if they are skipped, print out the diagnostics in an ad hoc manner. Update the documentation to note this change and integrate a new test, gtest_skip_in_environment_setup_test, into the test suite. This change addresses google#2189. Signed-off-by: Enji Cooper <yaneurabeya@gmail.com>

asomers

Works for me!

ngie-eign · 2019-03-30T20:11:10Z

Works for me!

I'll proceed with http://reviews.freebsd.org/D19765, adjusting the code if need be later based on review from the maintainers.

Add backwards compatibility shim for versions of googletest without `GTEST_SKIP()` and google/googletest#2203, e.g., the version of googletest in ports and the embedded version in the capsicum-test project, mapping to `GTEST_FAIL()`. The goal in this case was to make `capsicum-test` do a hard stop and not execute if the kernel didn't have capsicum support or `kern.trap_enotcap` was enabled. The only way to achieve this prior to google/googletest#2203, was to trigger a fatal failure, e.g., call `GTEST_FAIL()`. Output the skip diagnostic message via std::cerr, instead of using the `GTEST_{FAIL,SKIP}()` macros. For some reason I don't yet understand, the diagnostic messages aren't being output when both macros are triggered, so there's a bug potentially with googletest version 1.8.1 (but not master) where it's not properly outputting the messages.

emaste · 2019-04-01T18:09:37Z

For reference, now committed to FreeBSD's in-tree googletest: https://svnweb.freebsd.org/changeset/base/345770

…SetUp` Per the upstream pull-request [1]: ``` gtest prior to this change would completely ignore `GTEST_SKIP()` if called in `Environment::SetUp()`, instead of bailing out early, unlike `Test::SetUp()`, which would cause the tests themselves to be skipped. The only way (prior to this change) to skip the tests would be to trigger a fatal error via `GTEST_FAIL()`. Desirable behavior, in this case, when dealing with `Environment::SetUp()` is to check for prerequisites on a system (example, kernel supports a particular featureset, e.g., capsicum), and skip the tests. The alternatives prior to this change would be undesirable: - Failing sends the wrong message to the test user, as the result of the tests is indeterminate, not failed. - Having to add per-test class abstractions that override `SetUp()` to test for the capsicum feature set, then skip all of the tests in their respective SetUp fixtures, would be a lot of human and computational work; checking for the feature would need to be done for all of the tests, instead of once for all of the tests. For those reasons, making `Environment::SetUp()` handle `GTEST_SKIP()`, by not executing the testcases, is the most desirable solution. In order to properly diagnose what happened when running the tests if they are skipped, print out the diagnostics in an ad hoc manner. Update the documentation to note this change and integrate a new test, gtest_skip_in_environment_setup_test, into the test suite. This change addresses #2189. Signed-off-by: Enji Cooper <yaneurabeya@gmail.com> ``` The goal with my merging in this change is to avoid requiring extensive refactoring/retesting of test suites when ensuring prerequisites are met, e.g., checking for a CAPABILITIES-enabled kernel before running capsicum-test (see D19758 for more details). The proof-of-concept is being imported before accepted by the upstream project due to the fact that the upstream project is undergoing a potential development freeze and the maintainers aren't responding to my PR. 1. google/googletest#2203 Reported by: asomers (google/googletest#2189) Reviewed by: asomers Approved by: emaste (mentor) MFC after: 2 months Differential Revision: https://reviews.freebsd.org/D19765 git-svn-id: svn+ssh://svn.freebsd.org/base/head@345770 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f

…SetUp` Per the upstream pull-request [1]: ``` gtest prior to this change would completely ignore `GTEST_SKIP()` if called in `Environment::SetUp()`, instead of bailing out early, unlike `Test::SetUp()`, which would cause the tests themselves to be skipped. The only way (prior to this change) to skip the tests would be to trigger a fatal error via `GTEST_FAIL()`. Desirable behavior, in this case, when dealing with `Environment::SetUp()` is to check for prerequisites on a system (example, kernel supports a particular featureset, e.g., capsicum), and skip the tests. The alternatives prior to this change would be undesirable: - Failing sends the wrong message to the test user, as the result of the tests is indeterminate, not failed. - Having to add per-test class abstractions that override `SetUp()` to test for the capsicum feature set, then skip all of the tests in their respective SetUp fixtures, would be a lot of human and computational work; checking for the feature would need to be done for all of the tests, instead of once for all of the tests. For those reasons, making `Environment::SetUp()` handle `GTEST_SKIP()`, by not executing the testcases, is the most desirable solution. In order to properly diagnose what happened when running the tests if they are skipped, print out the diagnostics in an ad hoc manner. Update the documentation to note this change and integrate a new test, gtest_skip_in_environment_setup_test, into the test suite. This change addresses #2189. Signed-off-by: Enji Cooper <yaneurabeya@gmail.com> ``` The goal with my merging in this change is to avoid requiring extensive refactoring/retesting of test suites when ensuring prerequisites are met, e.g., checking for a CAPABILITIES-enabled kernel before running capsicum-test (see D19758 for more details). The proof-of-concept is being imported before accepted by the upstream project due to the fact that the upstream project is undergoing a potential development freeze and the maintainers aren't responding to my PR. 1. google/googletest#2203 Reported by: asomers (google/googletest#2189) Reviewed by: asomers Approved by: emaste (mentor) MFC after: 2 months Differential Revision: https://reviews.freebsd.org/D19765

PiperOrigin-RevId: 241372438

gennadiycivil · 2019-04-03T17:19:05Z

Looking, may tweak the tests a little.

gennadiycivil · 2019-04-03T17:20:29Z

@ngie-eign somehow he bot is not able to verify the CLA. Could you please confirm that all commits are authored by you and you are ok with Google CLA?
Thanks

PiperOrigin-RevId: 241372438

ngie-eign · 2019-04-03T17:59:58Z

Hi @gennadiycivil! Yes, I have one commit attached to this PR, and it’s signed by me... hmm... is the bot working?

ngie-eign · 2019-04-03T18:05:06Z

Oh, I read the second paragraph... it says that “it was authored by someone other than the PR submitter or a collaborator”. I think it’s getting cranky about your commit to merge the change to the branch...?

PiperOrigin-RevId: 241372438

ngie-eign · 2019-04-03T18:21:49Z

Just to rule out one possibility, I removed my middle name from my signed CLA (just to see if it’s doing strict author matches).

gennadiycivil · 2019-04-03T18:53:00Z

Oh look at that, it found you now :-) Thank you

PiperOrigin-RevId: 241372438

gennadiycivil · 2019-04-03T19:22:22Z

Sorry for all back and forth, I had some issues on the back end... should be good now

PiperOrigin-RevId: 241803437

gennadiycivil · 2019-04-03T21:16:51Z

This code should be in now as of 1f3b098 with very minor tweak ( added another test )

…SetUp` Per the upstream pull-request [1]: ``` gtest prior to this change would completely ignore `GTEST_SKIP()` if called in `Environment::SetUp()`, instead of bailing out early, unlike `Test::SetUp()`, which would cause the tests themselves to be skipped. The only way (prior to this change) to skip the tests would be to trigger a fatal error via `GTEST_FAIL()`. Desirable behavior, in this case, when dealing with `Environment::SetUp()` is to check for prerequisites on a system (example, kernel supports a particular featureset, e.g., capsicum), and skip the tests. The alternatives prior to this change would be undesirable: - Failing sends the wrong message to the test user, as the result of the tests is indeterminate, not failed. - Having to add per-test class abstractions that override `SetUp()` to test for the capsicum feature set, then skip all of the tests in their respective SetUp fixtures, would be a lot of human and computational work; checking for the feature would need to be done for all of the tests, instead of once for all of the tests. For those reasons, making `Environment::SetUp()` handle `GTEST_SKIP()`, by not executing the testcases, is the most desirable solution. In order to properly diagnose what happened when running the tests if they are skipped, print out the diagnostics in an ad hoc manner. Update the documentation to note this change and integrate a new test, gtest_skip_in_environment_setup_test, into the test suite. This change addresses #2189. Signed-off-by: Enji Cooper <yaneurabeya@gmail.com> ``` The goal with my merging in this change is to avoid requiring extensive refactoring/retesting of test suites when ensuring prerequisites are met, e.g., checking for a CAPABILITIES-enabled kernel before running capsicum-test (see D19758 for more details). The proof-of-concept is being imported before accepted by the upstream project due to the fact that the upstream project is undergoing a potential development freeze and the maintainers aren't responding to my PR. 1. google/googletest#2203 Reported by: asomers (google/googletest#2189) Reviewed by: asomers Approved by: emaste (mentor) MFC after: 2 months Differential Revision: https://reviews.freebsd.org/D19765 git-svn-id: https://svn.freebsd.org/base/head@345770 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f

…SetUp` Per the upstream pull-request [1]: ``` gtest prior to this change would completely ignore `GTEST_SKIP()` if called in `Environment::SetUp()`, instead of bailing out early, unlike `Test::SetUp()`, which would cause the tests themselves to be skipped. The only way (prior to this change) to skip the tests would be to trigger a fatal error via `GTEST_FAIL()`. Desirable behavior, in this case, when dealing with `Environment::SetUp()` is to check for prerequisites on a system (example, kernel supports a particular featureset, e.g., capsicum), and skip the tests. The alternatives prior to this change would be undesirable: - Failing sends the wrong message to the test user, as the result of the tests is indeterminate, not failed. - Having to add per-test class abstractions that override `SetUp()` to test for the capsicum feature set, then skip all of the tests in their respective SetUp fixtures, would be a lot of human and computational work; checking for the feature would need to be done for all of the tests, instead of once for all of the tests. For those reasons, making `Environment::SetUp()` handle `GTEST_SKIP()`, by not executing the testcases, is the most desirable solution. In order to properly diagnose what happened when running the tests if they are skipped, print out the diagnostics in an ad hoc manner. Update the documentation to note this change and integrate a new test, gtest_skip_in_environment_setup_test, into the test suite. This change addresses #2189. Signed-off-by: Enji Cooper <yaneurabeya@gmail.com> ``` The goal with my merging in this change is to avoid requiring extensive refactoring/retesting of test suites when ensuring prerequisites are met, e.g., checking for a CAPABILITIES-enabled kernel before running capsicum-test (see D19758 for more details). The proof-of-concept is being imported before accepted by the upstream project due to the fact that the upstream project is undergoing a potential development freeze and the maintainers aren't responding to my PR. 1. google/googletest#2203 Reported by: asomers (google/googletest#2189) Reviewed by: asomers Approved by: emaste (mentor) MFC after: 2 months Differential Revision: https://reviews.freebsd.org/D19765 git-svn-id: svn+ssh://svn.freebsd.org/base/head@345770 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f

…SetUp` Per the upstream pull-request [1]: ``` gtest prior to this change would completely ignore `GTEST_SKIP()` if called in `Environment::SetUp()`, instead of bailing out early, unlike `Test::SetUp()`, which would cause the tests themselves to be skipped. The only way (prior to this change) to skip the tests would be to trigger a fatal error via `GTEST_FAIL()`. Desirable behavior, in this case, when dealing with `Environment::SetUp()` is to check for prerequisites on a system (example, kernel supports a particular featureset, e.g., capsicum), and skip the tests. The alternatives prior to this change would be undesirable: - Failing sends the wrong message to the test user, as the result of the tests is indeterminate, not failed. - Having to add per-test class abstractions that override `SetUp()` to test for the capsicum feature set, then skip all of the tests in their respective SetUp fixtures, would be a lot of human and computational work; checking for the feature would need to be done for all of the tests, instead of once for all of the tests. For those reasons, making `Environment::SetUp()` handle `GTEST_SKIP()`, by not executing the testcases, is the most desirable solution. In order to properly diagnose what happened when running the tests if they are skipped, print out the diagnostics in an ad hoc manner. Update the documentation to note this change and integrate a new test, gtest_skip_in_environment_setup_test, into the test suite. This change addresses #2189. Signed-off-by: Enji Cooper <yaneurabeya@gmail.com> ``` The goal with my merging in this change is to avoid requiring extensive refactoring/retesting of test suites when ensuring prerequisites are met, e.g., checking for a CAPABILITIES-enabled kernel before running capsicum-test (see D19758 for more details). The proof-of-concept is being imported before accepted by the upstream project due to the fact that the upstream project is undergoing a potential development freeze and the maintainers aren't responding to my PR. 1. google/googletest#2203 Reported by: asomers (google/googletest#2189) Reviewed by: asomers Approved by: emaste (mentor) MFC after: 2 months Differential Revision: https://reviews.freebsd.org/D19765

…346574,r346576: r345203: Initial googlemock/googletest integration into the build/FreeBSD test suite This initial integration takes googlemock/googletest release 1.8.1, integrates the library, tests, and sample unit tests into the build. googlemock/googletest's inclusion is optionally available via `MK_GOOGLETEST`. `MK_GOOGLETEST` is dependent on `MK_TESTS` and is enabled by default when built with a C++11 capable toolchain. Google tests can be specified via the `GTESTS` variable, which, in comparison with the other test drivers, is more simplified/streamlined, as Googletest only supports C++ tests; not raw C or shell tests (C tests can be written in C++ using the standard embedding methods). No dependent libraries are assumed for the tests. One must specify `gmock`, `gmock_main`, `gtest`, or `gtest_main`, via `LIBADD` for the program. More information about googlemock and googletest can be found on the Googletest [project page](https://github.com/google/googletest), and the [GoogleMock](https://github.com/google/googletest/blob/v1.8.x/googlemock/docs/Documentation.md) and [GoogleTest](https://github.com/google/googletest/tree/v1.8.x/googletest/docs) docs. These tests are originally integrated into the build as plain driver tests, but will be natively integrated into Kyua in a later version. Known issues/Errata: * [WhenDynamicCastToTest.AmbiguousCast fails on FreeBSD](google/googletest#2172) r345205: Integrate cddl/usr.sbin/zfds/tests into the FreeBSD test suite This change integrates the unit tests for zfsd into the test suite using the integration method described in r345203. This change removes the `LOCALBASE` includes added for the port version of googlemock/googletest, as well as unnecessary `LIBADD`/`DPADD` and `CXXFLAGS` defines, which are included in the `GTEST_CXXFLAGS` variable, as part of r345203. r345353 (by asomers): googletest: backport GTEST_SKIP to googletest 1.8.1 This commit backports revisions 00938b2b228f3b70d3d9e51f29a1505bdad43f1e and 59f90a338bce2376b540ee239cf4e269bf6d68ad from googletest's master branch to our included version of googletest, which is based on 1.8.1. It adds the GTEST_SKIP feature, which is very useful for a project like FreeBSD where some tests depend on particular system configurations. Obtained from: github.com/google/googletest r345645: Spam CXXFLAGS with `-I${DESTDIR}/usr/include/private`, instead of GTEST_CXXFLAGS This makes it easier for googletest users to leverage googletest, instead of forcing them to plug GTEST_CXXFLAGS into CXXFLAGS manually (resulting in unnecessary duplication). I will be following this up with a more proper fix in src.libnames.mk, as src.libnames.mk should be automatically adding this directory to CFLAGS/CXXFLAGS when private libraries are referenced. Not doing so can result in mismatches between base-provided private library's and ports-provided library's headers. While here, tweak the comment to clarify what the intent is behind spamming CXXFLAGS. r345708: Standardize `-std=c++* as `CXXSTD` CXXSTD was added as the C++ analogue to CSTD. CXXSTD defaults to `-std=c++11` with supporting compilers; `-std=gnu++98`, otherwise for older versions of g++. This change standardizes the CXXSTD variable, originally added to googletest.test.inc.mk as part of r345203. As part of this effort, convert all `CXXFLAGS+= -std=*` calls to use `CXXSTD`. Notes: This value is not sanity checked in bsd.sys.mk, however, given the two most used C++ compilers on FreeBSD (clang++ and g++) support both modes, it is likely to work with both toolchains. This method will be refined in the future to support more variants of C++, as not all versions of clang++ and g++ (for instance) support C++14, C++17, etc. Any manual appending of `-std=*` to `CXXFLAGS` should be replaced with CXXSTD. Example: Before this commit: ``` CXXFLAGS+= -std=c++14 ``` After this commit: ``` CXXSTD= c++14 ``` Relnotes: yes Tested with: make tinderbox r345709: Allow users to override CSTD/CXXSTD on a per-prog basis The current logic for CSTD/CXXSTD requires homogenity as far as the supported C/C++ standards, which is a sensible default. However, when dealing with differing versions of C++, some code may compile with C++11, but not C++17 (for instance). So in order to avoid having people convert over their code to the new standard, give the users the ability to specify the standard on a per-program basis. This will allow a user to override the supporting standard for a set of programs, mixing C++11 with C++14 (for instance). Approved by: emaste (mentor) r345735: Allow programs to set `NO_SHARED` on a per-PROG basis This is particularly useful when installing programs for tests that need to be linked statically, e.g., mini-me from capsicum-test, which is linked statically to avoid the dynamic library lookup in the upstream project. r345770: Import proof-of-concept for handling `GTEST_SKIP()` in `Environment::SetUp` Per the upstream pull-request [1]: ``` gtest prior to this change would completely ignore `GTEST_SKIP()` if called in `Environment::SetUp()`, instead of bailing out early, unlike `Test::SetUp()`, which would cause the tests themselves to be skipped. The only way (prior to this change) to skip the tests would be to trigger a fatal error via `GTEST_FAIL()`. Desirable behavior, in this case, when dealing with `Environment::SetUp()` is to check for prerequisites on a system (example, kernel supports a particular featureset, e.g., capsicum), and skip the tests. The alternatives prior to this change would be undesirable: - Failing sends the wrong message to the test user, as the result of the tests is indeterminate, not failed. - Having to add per-test class abstractions that override `SetUp()` to test for the capsicum feature set, then skip all of the tests in their respective SetUp fixtures, would be a lot of human and computational work; checking for the feature would need to be done for all of the tests, instead of once for all of the tests. For those reasons, making `Environment::SetUp()` handle `GTEST_SKIP()`, by not executing the testcases, is the most desirable solution. In order to properly diagnose what happened when running the tests if they are skipped, print out the diagnostics in an ad hoc manner. Update the documentation to note this change and integrate a new test, gtest_skip_in_environment_setup_test, into the test suite. This change addresses #2189. Signed-off-by: Enji Cooper <yaneurabeya@gmail.com> ``` The goal with my merging in this change is to avoid requiring extensive refactoring/retesting of test suites when ensuring prerequisites are met, e.g., checking for a CAPABILITIES-enabled kernel before running capsicum-test (see D19758 for more details). The proof-of-concept is being imported before accepted by the upstream project due to the fact that the upstream project is undergoing a potential development freeze and the maintainers aren't responding to my PR. 1. google/googletest#2203 r346574: Rework CXXSTD setting via r345708 This change allows the user to once again override the C++ standard, restoring high-level pre-r345708 behavior. This also unbreaks building lib/ofed/libibnetdisc/Makefile with a non-C++11 capable compiler, e.g., g++ 4.2.1, as the library supported being built with older C++ standards. r346576: Fix up CXXSTD support originally added in r345708 r345708 worked for the base system, but unfortunately, caused a lot of disruption for third-party packages that relied on C++, since bsd.sys.mk is used by applications outside the base system. The defaults picked didn't match the compiler's defaults and broke some builds that didn't specify a standard, as well as some that overrode the value by setting `-std=gnu++14` (for example) manually. This change takes a more relaxed approach to appending `-std=${CXXSTD}` to CXXFLAGS, by only doing so when the value is specified, as opposed to overriding the standard set by an end-user. This avoids the need for having to bake NOP default into bsd.sys.mk for supported compiler-toolchain versions. In order to make this change possible, add CXXSTD to Makefile snippets which relied on the default value (c++11) added in r345708.

…346081,r346270,r346574,r346576: r345203: Initial googlemock/googletest integration into the build/FreeBSD test suite This initial integration takes googlemock/googletest release 1.8.1, integrates the library, tests, and sample unit tests into the build. googlemock/googletest's inclusion is optionally available via `MK_GOOGLETEST`. `MK_GOOGLETEST` is dependent on `MK_TESTS` and is enabled by default when built with a C++11 capable toolchain. Google tests can be specified via the `GTESTS` variable, which, in comparison with the other test drivers, is more simplified/streamlined, as Googletest only supports C++ tests; not raw C or shell tests (C tests can be written in C++ using the standard embedding methods). No dependent libraries are assumed for the tests. One must specify `gmock`, `gmock_main`, `gtest`, or `gtest_main`, via `LIBADD` for the program. More information about googlemock and googletest can be found on the Googletest [project page](https://github.com/google/googletest), and the [GoogleMock](https://github.com/google/googletest/blob/v1.8.x/googlemock/docs/Documentation.md) and [GoogleTest](https://github.com/google/googletest/tree/v1.8.x/googletest/docs) docs. These tests are originally integrated into the build as plain driver tests, but will be natively integrated into Kyua in a later version. Known issues/Errata: * [WhenDynamicCastToTest.AmbiguousCast fails on FreeBSD](google/googletest#2172) r345205: Integrate cddl/usr.sbin/zfds/tests into the FreeBSD test suite This change integrates the unit tests for zfsd into the test suite using the integration method described in r345203. This change removes the `LOCALBASE` includes added for the port version of googlemock/googletest, as well as unnecessary `LIBADD`/`DPADD` and `CXXFLAGS` defines, which are included in the `GTEST_CXXFLAGS` variable, as part of r345203. r345353 (by asomers): googletest: backport GTEST_SKIP to googletest 1.8.1 This commit backports revisions 00938b2b228f3b70d3d9e51f29a1505bdad43f1e and 59f90a338bce2376b540ee239cf4e269bf6d68ad from googletest's master branch to our included version of googletest, which is based on 1.8.1. It adds the GTEST_SKIP feature, which is very useful for a project like FreeBSD where some tests depend on particular system configurations. Obtained from: github.com/google/googletest r345645: Spam CXXFLAGS with `-I${DESTDIR}/usr/include/private`, instead of GTEST_CXXFLAGS This makes it easier for googletest users to leverage googletest, instead of forcing them to plug GTEST_CXXFLAGS into CXXFLAGS manually (resulting in unnecessary duplication). I will be following this up with a more proper fix in src.libnames.mk, as src.libnames.mk should be automatically adding this directory to CFLAGS/CXXFLAGS when private libraries are referenced. Not doing so can result in mismatches between base-provided private library's and ports-provided library's headers. While here, tweak the comment to clarify what the intent is behind spamming CXXFLAGS. r345708: Standardize `-std=c++* as `CXXSTD` CXXSTD was added as the C++ analogue to CSTD. CXXSTD defaults to `-std=c++11` with supporting compilers; `-std=gnu++98`, otherwise for older versions of g++. This change standardizes the CXXSTD variable, originally added to googletest.test.inc.mk as part of r345203. As part of this effort, convert all `CXXFLAGS+= -std=*` calls to use `CXXSTD`. Notes: This value is not sanity checked in bsd.sys.mk, however, given the two most used C++ compilers on FreeBSD (clang++ and g++) support both modes, it is likely to work with both toolchains. This method will be refined in the future to support more variants of C++, as not all versions of clang++ and g++ (for instance) support C++14, C++17, etc. Any manual appending of `-std=*` to `CXXFLAGS` should be replaced with CXXSTD. Example: Before this commit: ``` CXXFLAGS+= -std=c++14 ``` After this commit: ``` CXXSTD= c++14 ``` Relnotes: yes Tested with: make tinderbox r345709: Allow users to override CSTD/CXXSTD on a per-prog basis The current logic for CSTD/CXXSTD requires homogenity as far as the supported C/C++ standards, which is a sensible default. However, when dealing with differing versions of C++, some code may compile with C++11, but not C++17 (for instance). So in order to avoid having people convert over their code to the new standard, give the users the ability to specify the standard on a per-program basis. This will allow a user to override the supporting standard for a set of programs, mixing C++11 with C++14 (for instance). Apprved by: emaste (mentor) r345735: Allow programs to set `NO_SHARED` on a per-PROG basis This is particularly useful when installing programs for tests that need to be linked statically, e.g., mini-me from capsicum-test, which is linked statically to avoid the dynamic library lookup in the upstream project. r345770: Import proof-of-concept for handling `GTEST_SKIP()` in `Environment::SetUp` Per the upstream pull-request [1]: ``` gtest prior to this change would completely ignore `GTEST_SKIP()` if called in `Environment::SetUp()`, instead of bailing out early, unlike `Test::SetUp()`, which would cause the tests themselves to be skipped. The only way (prior to this change) to skip the tests would be to trigger a fatal error via `GTEST_FAIL()`. Desirable behavior, in this case, when dealing with `Environment::SetUp()` is to check for prerequisites on a system (example, kernel supports a particular featureset, e.g., capsicum), and skip the tests. The alternatives prior to this change would be undesirable: - Failing sends the wrong message to the test user, as the result of the tests is indeterminate, not failed. - Having to add per-test class abstractions that override `SetUp()` to test for the capsicum feature set, then skip all of the tests in their respective SetUp fixtures, would be a lot of human and computational work; checking for the feature would need to be done for all of the tests, instead of once for all of the tests. For those reasons, making `Environment::SetUp()` handle `GTEST_SKIP()`, by not executing the testcases, is the most desirable solution. In order to properly diagnose what happened when running the tests if they are skipped, print out the diagnostics in an ad hoc manner. Update the documentation to note this change and integrate a new test, gtest_skip_in_environment_setup_test, into the test suite. This change addresses #2189. Signed-off-by: Enji Cooper <yaneurabeya@gmail.com> ``` The goal with my merging in this change is to avoid requiring extensive refactoring/retesting of test suites when ensuring prerequisites are met, e.g., checking for a CAPABILITIES-enabled kernel before running capsicum-test (see D19758 for more details). The proof-of-concept is being imported before accepted by the upstream project due to the fact that the upstream project is undergoing a potential development freeze and the maintainers aren't responding to my PR. 1. google/googletest#2203 r346081 (by trasz): Make zfsd(8) build obey CFLAGS. Obtained from: CheriBSD r346270 (by trasz): Drop -g from CFLAGS for zfsd(8). No idea why it was ever there. r346574: Rework CXXSTD setting via r345708 This change allows the user to once again override the C++ standard, restoring high-level pre-r345708 behavior. This also unbreaks building lib/ofed/libibnetdisc/Makefile with a non-C++11 capable compiler, e.g., g++ 4.2.1, as the library supported being built with older C++ standards. r346576: Fix up CXXSTD support originally added in r345708 r345708 worked for the base system, but unfortunately, caused a lot of disruption for third-party packages that relied on C++, since bsd.sys.mk is used by applications outside the base system. The defaults picked didn't match the compiler's defaults and broke some builds that didn't specify a standard, as well as some that overrode the value by setting `-std=gnu++14` (for example) manually. This change takes a more relaxed approach to appending `-std=${CXXSTD}` to CXXFLAGS, by only doing so when the value is specified, as opposed to overriding the standard set by an end-user. This avoids the need for having to bake NOP default into bsd.sys.mk for supported compiler-toolchain versions. In order to make this change possible, add CXXSTD to Makefile snippets which relied on the default value (c++11) added in r345708.

…346574,r346576: r345203: Initial googlemock/googletest integration into the build/FreeBSD test suite This initial integration takes googlemock/googletest release 1.8.1, integrates the library, tests, and sample unit tests into the build. googlemock/googletest's inclusion is optionally available via `MK_GOOGLETEST`. `MK_GOOGLETEST` is dependent on `MK_TESTS` and is enabled by default when built with a C++11 capable toolchain. Google tests can be specified via the `GTESTS` variable, which, in comparison with the other test drivers, is more simplified/streamlined, as Googletest only supports C++ tests; not raw C or shell tests (C tests can be written in C++ using the standard embedding methods). No dependent libraries are assumed for the tests. One must specify `gmock`, `gmock_main`, `gtest`, or `gtest_main`, via `LIBADD` for the program. More information about googlemock and googletest can be found on the Googletest [project page](https://github.com/google/googletest), and the [GoogleMock](https://github.com/google/googletest/blob/v1.8.x/googlemock/docs/Documentation.md) and [GoogleTest](https://github.com/google/googletest/tree/v1.8.x/googletest/docs) docs. These tests are originally integrated into the build as plain driver tests, but will be natively integrated into Kyua in a later version. Known issues/Errata: * [WhenDynamicCastToTest.AmbiguousCast fails on FreeBSD](google/googletest#2172) r345205: Integrate cddl/usr.sbin/zfds/tests into the FreeBSD test suite This change integrates the unit tests for zfsd into the test suite using the integration method described in r345203. This change removes the `LOCALBASE` includes added for the port version of googlemock/googletest, as well as unnecessary `LIBADD`/`DPADD` and `CXXFLAGS` defines, which are included in the `GTEST_CXXFLAGS` variable, as part of r345203. r345353 (by asomers): googletest: backport GTEST_SKIP to googletest 1.8.1 This commit backports revisions 00938b2b228f3b70d3d9e51f29a1505bdad43f1e and 59f90a338bce2376b540ee239cf4e269bf6d68ad from googletest's master branch to our included version of googletest, which is based on 1.8.1. It adds the GTEST_SKIP feature, which is very useful for a project like FreeBSD where some tests depend on particular system configurations. Obtained from: github.com/google/googletest r345645: Spam CXXFLAGS with `-I${DESTDIR}/usr/include/private`, instead of GTEST_CXXFLAGS This makes it easier for googletest users to leverage googletest, instead of forcing them to plug GTEST_CXXFLAGS into CXXFLAGS manually (resulting in unnecessary duplication). I will be following this up with a more proper fix in src.libnames.mk, as src.libnames.mk should be automatically adding this directory to CFLAGS/CXXFLAGS when private libraries are referenced. Not doing so can result in mismatches between base-provided private library's and ports-provided library's headers. While here, tweak the comment to clarify what the intent is behind spamming CXXFLAGS. r345708: Standardize `-std=c++* as `CXXSTD` CXXSTD was added as the C++ analogue to CSTD. CXXSTD defaults to `-std=c++11` with supporting compilers; `-std=gnu++98`, otherwise for older versions of g++. This change standardizes the CXXSTD variable, originally added to googletest.test.inc.mk as part of r345203. As part of this effort, convert all `CXXFLAGS+= -std=*` calls to use `CXXSTD`. Notes: This value is not sanity checked in bsd.sys.mk, however, given the two most used C++ compilers on FreeBSD (clang++ and g++) support both modes, it is likely to work with both toolchains. This method will be refined in the future to support more variants of C++, as not all versions of clang++ and g++ (for instance) support C++14, C++17, etc. Any manual appending of `-std=*` to `CXXFLAGS` should be replaced with CXXSTD. Example: Before this commit: ``` CXXFLAGS+= -std=c++14 ``` After this commit: ``` CXXSTD= c++14 ``` Relnotes: yes Tested with: make tinderbox r345709: Allow users to override CSTD/CXXSTD on a per-prog basis The current logic for CSTD/CXXSTD requires homogenity as far as the supported C/C++ standards, which is a sensible default. However, when dealing with differing versions of C++, some code may compile with C++11, but not C++17 (for instance). So in order to avoid having people convert over their code to the new standard, give the users the ability to specify the standard on a per-program basis. This will allow a user to override the supporting standard for a set of programs, mixing C++11 with C++14 (for instance). Approved by: emaste (mentor) r345735: Allow programs to set `NO_SHARED` on a per-PROG basis This is particularly useful when installing programs for tests that need to be linked statically, e.g., mini-me from capsicum-test, which is linked statically to avoid the dynamic library lookup in the upstream project. r345770: Import proof-of-concept for handling `GTEST_SKIP()` in `Environment::SetUp` Per the upstream pull-request [1]: ``` gtest prior to this change would completely ignore `GTEST_SKIP()` if called in `Environment::SetUp()`, instead of bailing out early, unlike `Test::SetUp()`, which would cause the tests themselves to be skipped. The only way (prior to this change) to skip the tests would be to trigger a fatal error via `GTEST_FAIL()`. Desirable behavior, in this case, when dealing with `Environment::SetUp()` is to check for prerequisites on a system (example, kernel supports a particular featureset, e.g., capsicum), and skip the tests. The alternatives prior to this change would be undesirable: - Failing sends the wrong message to the test user, as the result of the tests is indeterminate, not failed. - Having to add per-test class abstractions that override `SetUp()` to test for the capsicum feature set, then skip all of the tests in their respective SetUp fixtures, would be a lot of human and computational work; checking for the feature would need to be done for all of the tests, instead of once for all of the tests. For those reasons, making `Environment::SetUp()` handle `GTEST_SKIP()`, by not executing the testcases, is the most desirable solution. In order to properly diagnose what happened when running the tests if they are skipped, print out the diagnostics in an ad hoc manner. Update the documentation to note this change and integrate a new test, gtest_skip_in_environment_setup_test, into the test suite. This change addresses #2189. Signed-off-by: Enji Cooper <yaneurabeya@gmail.com> ``` The goal with my merging in this change is to avoid requiring extensive refactoring/retesting of test suites when ensuring prerequisites are met, e.g., checking for a CAPABILITIES-enabled kernel before running capsicum-test (see D19758 for more details). The proof-of-concept is being imported before accepted by the upstream project due to the fact that the upstream project is undergoing a potential development freeze and the maintainers aren't responding to my PR. 1. google/googletest#2203 r346574: Rework CXXSTD setting via r345708 This change allows the user to once again override the C++ standard, restoring high-level pre-r345708 behavior. This also unbreaks building lib/ofed/libibnetdisc/Makefile with a non-C++11 capable compiler, e.g., g++ 4.2.1, as the library supported being built with older C++ standards. r346576: Fix up CXXSTD support originally added in r345708 r345708 worked for the base system, but unfortunately, caused a lot of disruption for third-party packages that relied on C++, since bsd.sys.mk is used by applications outside the base system. The defaults picked didn't match the compiler's defaults and broke some builds that didn't specify a standard, as well as some that overrode the value by setting `-std=gnu++14` (for example) manually. This change takes a more relaxed approach to appending `-std=${CXXSTD}` to CXXFLAGS, by only doing so when the value is specified, as opposed to overriding the standard set by an end-user. This avoids the need for having to bake NOP default into bsd.sys.mk for supported compiler-toolchain versions. In order to make this change possible, add CXXSTD to Makefile snippets which relied on the default value (c++11) added in r345708. git-svn-id: https://svn.freebsd.org/base/stable/12@348136 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f

…346081,r346270,r346574,r346576: r345203: Initial googlemock/googletest integration into the build/FreeBSD test suite This initial integration takes googlemock/googletest release 1.8.1, integrates the library, tests, and sample unit tests into the build. googlemock/googletest's inclusion is optionally available via `MK_GOOGLETEST`. `MK_GOOGLETEST` is dependent on `MK_TESTS` and is enabled by default when built with a C++11 capable toolchain. Google tests can be specified via the `GTESTS` variable, which, in comparison with the other test drivers, is more simplified/streamlined, as Googletest only supports C++ tests; not raw C or shell tests (C tests can be written in C++ using the standard embedding methods). No dependent libraries are assumed for the tests. One must specify `gmock`, `gmock_main`, `gtest`, or `gtest_main`, via `LIBADD` for the program. More information about googlemock and googletest can be found on the Googletest [project page](https://github.com/google/googletest), and the [GoogleMock](https://github.com/google/googletest/blob/v1.8.x/googlemock/docs/Documentation.md) and [GoogleTest](https://github.com/google/googletest/tree/v1.8.x/googletest/docs) docs. These tests are originally integrated into the build as plain driver tests, but will be natively integrated into Kyua in a later version. Known issues/Errata: * [WhenDynamicCastToTest.AmbiguousCast fails on FreeBSD](google/googletest#2172) r345205: Integrate cddl/usr.sbin/zfds/tests into the FreeBSD test suite This change integrates the unit tests for zfsd into the test suite using the integration method described in r345203. This change removes the `LOCALBASE` includes added for the port version of googlemock/googletest, as well as unnecessary `LIBADD`/`DPADD` and `CXXFLAGS` defines, which are included in the `GTEST_CXXFLAGS` variable, as part of r345203. r345353 (by asomers): googletest: backport GTEST_SKIP to googletest 1.8.1 This commit backports revisions 00938b2b228f3b70d3d9e51f29a1505bdad43f1e and 59f90a338bce2376b540ee239cf4e269bf6d68ad from googletest's master branch to our included version of googletest, which is based on 1.8.1. It adds the GTEST_SKIP feature, which is very useful for a project like FreeBSD where some tests depend on particular system configurations. Obtained from: github.com/google/googletest r345645: Spam CXXFLAGS with `-I${DESTDIR}/usr/include/private`, instead of GTEST_CXXFLAGS This makes it easier for googletest users to leverage googletest, instead of forcing them to plug GTEST_CXXFLAGS into CXXFLAGS manually (resulting in unnecessary duplication). I will be following this up with a more proper fix in src.libnames.mk, as src.libnames.mk should be automatically adding this directory to CFLAGS/CXXFLAGS when private libraries are referenced. Not doing so can result in mismatches between base-provided private library's and ports-provided library's headers. While here, tweak the comment to clarify what the intent is behind spamming CXXFLAGS. r345708: Standardize `-std=c++* as `CXXSTD` CXXSTD was added as the C++ analogue to CSTD. CXXSTD defaults to `-std=c++11` with supporting compilers; `-std=gnu++98`, otherwise for older versions of g++. This change standardizes the CXXSTD variable, originally added to googletest.test.inc.mk as part of r345203. As part of this effort, convert all `CXXFLAGS+= -std=*` calls to use `CXXSTD`. Notes: This value is not sanity checked in bsd.sys.mk, however, given the two most used C++ compilers on FreeBSD (clang++ and g++) support both modes, it is likely to work with both toolchains. This method will be refined in the future to support more variants of C++, as not all versions of clang++ and g++ (for instance) support C++14, C++17, etc. Any manual appending of `-std=*` to `CXXFLAGS` should be replaced with CXXSTD. Example: Before this commit: ``` CXXFLAGS+= -std=c++14 ``` After this commit: ``` CXXSTD= c++14 ``` Relnotes: yes Tested with: make tinderbox r345709: Allow users to override CSTD/CXXSTD on a per-prog basis The current logic for CSTD/CXXSTD requires homogenity as far as the supported C/C++ standards, which is a sensible default. However, when dealing with differing versions of C++, some code may compile with C++11, but not C++17 (for instance). So in order to avoid having people convert over their code to the new standard, give the users the ability to specify the standard on a per-program basis. This will allow a user to override the supporting standard for a set of programs, mixing C++11 with C++14 (for instance). Apprved by: emaste (mentor) r345735: Allow programs to set `NO_SHARED` on a per-PROG basis This is particularly useful when installing programs for tests that need to be linked statically, e.g., mini-me from capsicum-test, which is linked statically to avoid the dynamic library lookup in the upstream project. r345770: Import proof-of-concept for handling `GTEST_SKIP()` in `Environment::SetUp` Per the upstream pull-request [1]: ``` gtest prior to this change would completely ignore `GTEST_SKIP()` if called in `Environment::SetUp()`, instead of bailing out early, unlike `Test::SetUp()`, which would cause the tests themselves to be skipped. The only way (prior to this change) to skip the tests would be to trigger a fatal error via `GTEST_FAIL()`. Desirable behavior, in this case, when dealing with `Environment::SetUp()` is to check for prerequisites on a system (example, kernel supports a particular featureset, e.g., capsicum), and skip the tests. The alternatives prior to this change would be undesirable: - Failing sends the wrong message to the test user, as the result of the tests is indeterminate, not failed. - Having to add per-test class abstractions that override `SetUp()` to test for the capsicum feature set, then skip all of the tests in their respective SetUp fixtures, would be a lot of human and computational work; checking for the feature would need to be done for all of the tests, instead of once for all of the tests. For those reasons, making `Environment::SetUp()` handle `GTEST_SKIP()`, by not executing the testcases, is the most desirable solution. In order to properly diagnose what happened when running the tests if they are skipped, print out the diagnostics in an ad hoc manner. Update the documentation to note this change and integrate a new test, gtest_skip_in_environment_setup_test, into the test suite. This change addresses #2189. Signed-off-by: Enji Cooper <yaneurabeya@gmail.com> ``` The goal with my merging in this change is to avoid requiring extensive refactoring/retesting of test suites when ensuring prerequisites are met, e.g., checking for a CAPABILITIES-enabled kernel before running capsicum-test (see D19758 for more details). The proof-of-concept is being imported before accepted by the upstream project due to the fact that the upstream project is undergoing a potential development freeze and the maintainers aren't responding to my PR. 1. google/googletest#2203 r346081 (by trasz): Make zfsd(8) build obey CFLAGS. Obtained from: CheriBSD r346270 (by trasz): Drop -g from CFLAGS for zfsd(8). No idea why it was ever there. r346574: Rework CXXSTD setting via r345708 This change allows the user to once again override the C++ standard, restoring high-level pre-r345708 behavior. This also unbreaks building lib/ofed/libibnetdisc/Makefile with a non-C++11 capable compiler, e.g., g++ 4.2.1, as the library supported being built with older C++ standards. r346576: Fix up CXXSTD support originally added in r345708 r345708 worked for the base system, but unfortunately, caused a lot of disruption for third-party packages that relied on C++, since bsd.sys.mk is used by applications outside the base system. The defaults picked didn't match the compiler's defaults and broke some builds that didn't specify a standard, as well as some that overrode the value by setting `-std=gnu++14` (for example) manually. This change takes a more relaxed approach to appending `-std=${CXXSTD}` to CXXFLAGS, by only doing so when the value is specified, as opposed to overriding the standard set by an end-user. This avoids the need for having to bake NOP default into bsd.sys.mk for supported compiler-toolchain versions. In order to make this change possible, add CXXSTD to Makefile snippets which relied on the default value (c++11) added in r345708. git-svn-id: https://svn.freebsd.org/base/stable/12@348138 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f

These google test version is based on release-1.7.0 with single backport patch google/googletest#2203 15dde751 (HEAD) Merge 2ce0685f76a4db403b7b2650433a584c150f2108 into 75e834700d19aa373b428c7c746f951737354c28 c9945853 (tag: release-1.7.0) Cuts release 1.7.0 Signed-off-by: Igor Ivanov <igori@nvidia.com>

googlebot added the cla: yes label Mar 30, 2019

ngie-eign force-pushed the issue-2189 branch from 9b4cb1b to 2fcb9e3 Compare March 30, 2019 05:27

ngie-eign mentioned this pull request Mar 30, 2019

GTEST_SKIP doesn't work in test environments #2189

Closed

ngie-eign force-pushed the issue-2189 branch 2 times, most recently from 4c7c316 to 67c75ff Compare March 30, 2019 18:37

asomers approved these changes Mar 30, 2019

View reviewed changes

ngie-eign force-pushed the issue-2189 branch 3 times, most recently from d57fdbe to 67c75ff Compare April 3, 2019 06:57

gennadiycivil mentioned this pull request Apr 3, 2019

Googletest export #2214

Closed

gennadiycivil added a commit that referenced this pull request Apr 3, 2019

Merge pull request #2203 from ngie-eign:issue-2189

511156e

PiperOrigin-RevId: 241372438

gennadiycivil added a commit that referenced this pull request Apr 3, 2019

Merge pull request #2203 from ngie-eign:issue-2189

a1ab1f0

PiperOrigin-RevId: 241372438

gennadiycivil added a commit that referenced this pull request Apr 3, 2019

Merge pull request #2203 from ngie-eign:issue-2189

bfd2285

PiperOrigin-RevId: 241372438

This was referenced Apr 3, 2019

Googletest export #2215

Closed

Googletest export #2216

Closed

gennadiycivil added a commit that referenced this pull request Apr 3, 2019

Merge pull request #2203 from ngie-eign:issue-2189

ef09313

PiperOrigin-RevId: 241372438

gennadiycivil mentioned this pull request Apr 3, 2019

Googletest export #2217

Closed

gennadiycivil added a commit that referenced this pull request Apr 3, 2019

Merge pull request #2203 from ngie-eign:issue-2189

a719bf9

PiperOrigin-RevId: 241372438

gennadiycivil added the imported-into-CL label Apr 3, 2019

gennadiycivil mentioned this pull request Apr 3, 2019

Googletest export #2218

Closed

gennadiycivil added a commit that referenced this pull request Apr 3, 2019

Merge pull request #2203 from ngie-eign:issue-2189

4a9fbcc

PiperOrigin-RevId: 241372438

gennadiycivil closed this Apr 3, 2019

gennadiycivil reopened this Apr 3, 2019

gennadiycivil merged commit 67c75ff into google:master Apr 3, 2019

gennadiycivil added a commit that referenced this pull request Apr 3, 2019

Merge pull request #2203 from ngie-eign:issue-2189

1f3b098

PiperOrigin-RevId: 241803437

ngie-eign deleted the issue-2189 branch April 4, 2019 01:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle GTEST_SKIP() when calling `Environment::SetUp()` #2203

Handle GTEST_SKIP() when calling `Environment::SetUp()` #2203

ngie-eign commented Mar 30, 2019 •

edited

asomers left a comment

ngie-eign commented Mar 30, 2019

emaste commented Apr 1, 2019

gennadiycivil commented Apr 3, 2019

gennadiycivil commented Apr 3, 2019

ngie-eign commented Apr 3, 2019

ngie-eign commented Apr 3, 2019

ngie-eign commented Apr 3, 2019

gennadiycivil commented Apr 3, 2019

gennadiycivil commented Apr 3, 2019

gennadiycivil commented Apr 3, 2019 •

edited

Handle GTEST_SKIP() when calling Environment::SetUp() #2203

Handle GTEST_SKIP() when calling Environment::SetUp() #2203

Conversation

ngie-eign commented Mar 30, 2019 • edited

asomers left a comment

Choose a reason for hiding this comment

ngie-eign commented Mar 30, 2019

emaste commented Apr 1, 2019

gennadiycivil commented Apr 3, 2019

gennadiycivil commented Apr 3, 2019

ngie-eign commented Apr 3, 2019

ngie-eign commented Apr 3, 2019

ngie-eign commented Apr 3, 2019

gennadiycivil commented Apr 3, 2019

gennadiycivil commented Apr 3, 2019

gennadiycivil commented Apr 3, 2019 • edited

Handle GTEST_SKIP() when calling `Environment::SetUp()` #2203

Handle GTEST_SKIP() when calling `Environment::SetUp()` #2203

ngie-eign commented Mar 30, 2019 •

edited

gennadiycivil commented Apr 3, 2019 •

edited