Don't build googletest internal tests by default

[arichardson]

Summary

These tests can take a *very* long time to compile and link (especially when
using an assertions-enabled compiler) and are unlikely to find real bugs
in CI testing (other than imported a broken version of googletest or errors
in the compiler/libc++).

This change only enables these internal googletest tests when a new
make flag WITH_GOOGLETEST_INTERNAL_TESTS is passed instead
of always building them when tests are enabled.
However, the GOOGLETEST option remains on-by-default since it
is useful for writing tests.

For the CheriBSD CI setup I disabled building these tests and this reduced
the time our CI runs take by over 10 minutes/25% (the total time is around
30 minutes for a basic compile+boot test depending on Jenkins load).

See also https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=241848

[lwhsu]

I think marking these test as EXTRA_TESTS is a good idea, and I don't strongly object setting them off. However maybe we can also check why these tests are slow, and maybe only disabled on !x86 architectures. If we disabled these tests by default, I think it is still worth to run them at least daily. (This is the planned frequency of !x86 test jobs on ci.freebsd.org)

Indeed in the most of the time the test of the test framework should be fine, but I like to include them in CI because of it can serve as a sanity check of the tests, i.e., we should not trust the results if any of these tests fails. Recently there do exist failures of googletest when we switched to the new regex lib. In fact, I'm also planning to enable installing the kyua tests by default, which is currently running in ci.freebsd.org via kyua package.

BTW, on amd64, the lib.googletest.* take under 10 seconds: https://ci.freebsd.org/job/FreeBSD-head-amd64-test/16159/testReport/ which is very minor to the total tests execution time (~1h). This difference is probably due to running in an emulator is much slower and there are less test cases enabled on !x86 architectures, so as regarding to the compile time.

In the long term, I want to have at least two test categories, one is fast/smoky/sanity tests and the other is full/functional/regression tests. For each commit or pre-commit, we can only execute the first one, and we run the later category after certain period, like one hour or longer, depends on the computing resource we have. These EXTRA_TESTS definitely belong to the second category.

[arichardson]

In D26067#578588, @lwhsu wrote:

I think marking these test as EXTRA_TESTS is a good idea, and I don't strongly object setting them off. However maybe we can also check why these tests are slow, and maybe only disabled on !x86 architectures. If we disabled these tests by default, I think it is still worth to run them at least daily. (This is the planned frequency of !x86 test jobs on ci.freebsd.org)

Indeed in the most of the time the test of the test framework should be fine, but I like to include them in CI because of it can serve as a sanity check of the tests, i.e., we should not trust the results if any of these tests fails. Recently there do exist failures of googletest when we switched to the new regex lib. In fact, I'm also planning to enable installing the kyua tests by default, which is currently running in ci.freebsd.org via kyua package.

BTW, on amd64, the lib.googletest.* take under 10 seconds: https://ci.freebsd.org/job/FreeBSD-head-amd64-test/16159/testReport/ which is very minor to the total tests execution time (~1h). This difference is probably due to running in an emulator is much slower and there are less test cases enabled on !x86 architectures, so as regarding to the compile time.

In the long term, I want to have at least two test categories, one is fast/smoky/sanity tests and the other is full/functional/regression tests. For each commit or pre-commit, we can only execute the first one, and we run the later category after certain period, like one hour or longer, depends on the computing resource we have. These EXTRA_TESTS definitely belong to the second category.

I probably wasn't clear enough in my message, it's not running the tests that's slow, it's building them (even for x86). The problem is that this seems to happen towards the end of the libraries build so it's mostly serial even when using something like -j72

[lwhsu]

In D26067#578589, @arichardson wrote:

I probably wasn't clear enough in my message, it's not running the tests that's slow, it's building them (even for x86). The problem is that this seems to happen towards the end of the libraries build so it's mostly serial even when using something like -j72

Sorry, I missed that the test part of the statistics is only for booting test and thought it was slow in both build and run. Now I agree it's fine to have these off by default. These tests are not that useful for end users with -RELEASE, I may still enable this in ci.freebsd.org, or have another full test job executed in less frequently.

BTW, do we need to create tools/build/options/WITH_EXTRA_TESTS and regen share/man/man5/src.conf.5 ? And if possible, please mention this option in test(7).

share/mk/src.opts.mk

466

I'm not sure if we need to add MK_EXTRA_TESTS:= no here. It seems not really necessary for now as WITHOUT_TESTS implies WITHOUT_GOOGLETEST and EXTRA_TESTS is under googletest. We may need it in the future when we have more tests moved to EXTRA_TESTS.

[ngie]

Please use WITH_GOOGLETEST_INTERNAL_TESTS or something similar and default to on, since it seems to be limited to that case.

The reasoning I have is that I still use these tests, base system changes (like upgrading llvm) can break googletest resulting in hard to triage issues--bisecting between a large number of issues, and WITH_EXTRA_TESTS is sufficiently arbitrary that it doesn't accurately describe what the aim of the option is.

PS An option description is missing from the change as well.

[arichardson]

This comment was removed by arichardson.

[arichardson]

In D26067#580258, @ngie wrote:

Please use WITH_GOOGLETEST_INTERNAL_TESTS or something similar and default to on, since it seems to be limited to that case.

The reasoning I have is that I still use these tests, base system changes (like upgrading llvm) can break googletest resulting in hard to triage issues--bisecting between a large number of issues, and WITH_EXTRA_TESTS is sufficiently arbitrary that it doesn't accurately describe what the aim of the option is.

PS An option description is missing from the change as well.

I wasn't sure about the name for the option so I haven't added a description yet, will do in the next version.
I also assumed this knob could be used for other tests that are only useful for "full CI builds" rather than checking for regressions.

I believe the default should be off until the excessive compile time is fixed. These tests are unlikely to catch any regressions but add about 20 minutes of compile time to *all* builds. If the default isn't off I might as well just keep our downstream change that disables them completely and not bother upstreaming it. Having the option to turn these tests on for people who care makes more sense to me.

Also why should we run Googletest tests (an internal test framework) by default but not the dtrace tests (a user-facing tool)?

[lwhsu]

In D26067#580958, @arichardson wrote:

I wasn't sure about the name for the option so I haven't added a description yet, will do in the next version.
I also assumed this knob could be used for other tests that are only useful for "full CI builds" rather than checking for regressions.

I believe the default should be off until the excessive compile time is fixed. These tests are unlikely to catch any regressions but add about 20 minutes of compile time to *all* builds. If the default isn't off I might as well just keep our downstream change that disables them completely and not bother upstreaming it. Having the option to turn these tests on for people who care makes more sense to me.

I think both names are fine, as currently this option only disable internal tests of googletest. Maybe in the future we need both when we install kyua's self tests, then we may have WITH_GOOGLETEST_INTERNAL_TESTS and WITH_KYUA_INTERNAL_TESTS, both are covered by WITH_EXTRA_TESTS. So normally we have WITH_EXTRA_TESTS off, and googletest maintainer can enable WITH_GOOGLETEST_INTERNAL_TESTS and kyua maintainers can enable WITH_KYUA_INTERNAL_TESTS when working on importing new the version. And if we have WITH_EXTRA_TESTS default off, I plan to have a separated job in CI which maybe runs once a day to ensure the key components of tests are sane.

Also why should we run Googletest tests (an internal test framework) by default but not the dtrace tests (a user-facing tool)?

I want and am slowly working on it. See D20096 , the main reason it still hasn't been merged is there are still some tests failing, e.g., https://bugs.freebsd.org/237641 I may go ahead to skip these tests and track them in PRs, but there are still some new flakey tests pop out so they need more monitoring and fixing.

[arichardson]

rename option, rebase on top of D26748, add docs

[ngie]

tools/build/options/WITH_GOOGLETEST_INTERNAL_TESTS

4

typo: goooogletest -> googletest .

I would just shorten this to state that this option enables/disables building the googletest internal tests, period. This description paints the pathological case (which as of yet hasn't really been demonstrated from what I've seen on amd64) as being an issue on all platforms.

Finally, the potential rationale for disabling this option is misleading: someone changing libc++ or the compiler could effectively break googletest, making finding errors with base system changes more difficult to bisect. ATF/kyua builds and distributes its internal tests--I don't see why googletest shouldn't follow suit as well by default (especially since it's a more complicated modern C++-based framework).

[ngie]

tools/build/options/WITH_GOOGLETEST_INTERNAL_TESTS

4

Admittedly, I'm reacting from a place of having seen tests break in FreeBSD and elsewhere where folks haven't reacted quickly enough to address issues.

Not having the data makes the job of those triaging issues much harder -- especially when we don't have a closed loop system for running CI on changes before merge in a staging environment, or as part of CRs.

[arichardson]

tools/build/options/WITH_GOOGLETEST_INTERNAL_TESTS

4

Admittedly, I'm reacting from a place of having seen tests break in FreeBSD and elsewhere where folks haven't reacted quickly enough to address issues.

Not having the data makes the job of those triaging issues much harder -- especially when we don't have a closed loop system for running CI on changes before merge in a staging environment, or as part of CRs.

I am also very unhappy about disabling tests (especially considering that the current coverage is mediocre).
However, I feel like spending almost 15-25% of our build time on one set of tests is not worth it.

The real problem is the build system which just can't do parallelism so we end up being stuck building these tests on a few cores while all others are idle :(

[arichardson]

I think this option should be set to on in the default ci.freebsd environment, but most users probably don't need to run these tests.

[arichardson]

update

[arichardson]

Build time is no longer too bad, will drop this for now.

In the future it might make sense to add a separate TEST_FOR_CI (or similar) that builds the tests that don't really check the systems runs as expected (e.g. atf/kyua/googletest) but rather test that contrib software was imported at a known-good state.