open-telemetry / open-telemetry/opentelemetry-cpp
[TEST] CTest reports a pass for gtest cases the binary does not contain
Nobody has claimed this yet.
- Dominant language
- C++
- Stars
- 1.4k
- Forks
- 632
- Avg merge
- 1d 13h
- Merged PRs (30d)
- 75
Description
gtest_add_tests() registers cases by scanning the source, so a case behind a preprocessor conditional stays registered in configurations where it is compiled out. CTest then runs it as <binary> --gtest_filter=<name>, gtest matches nothing, and exits 0. The case reports a pass having run nothing.
Reproduction
A default Linux CMake build with -DBUILD_TESTING=ON, so OPENTELEMETRY_ABI_VERSION_NO is 1 and the abiv2 cases are compiled out:
$ ./sdk/test/metrics/meter_provider_sdk_test --gtest_list_tests | grep -c GetMeterAbiv2
0
$ ctest -R "MeterProvider.GetMeterAbiv2"
Start 443: metrics.MeterProvider.GetMeterAbiv2
1/1 Test #443: metrics.MeterProvider.GetMeterAbiv2 ... Passed 0.01 sec
100% tests passed, 0 tests failed out of 1
$ ./sdk/test/metrics/meter_provider_sdk_test --gtest_filter=MeterProvider.GetMeterAbiv2
[==========] Running 0 tests from 0 test suites.
[ PASSED ] 0 tests.
$ echo $?
0
How much of it there is
Comparing every CTest name against what each binary reports from --gtest_list_tests, in that one build: 45 registered gtest cases across 14 test binaries are not present in the binary CTest runs them against. All 45 report a pass.
Two categories are excluded from that number rather than counted: 10 further differences are examples.*, which are add_test entries rather than gtest cases, and parameterised suites, whose registered and instantiated names differ by construction.
The guards behind them are the ones you would expect, and none of them is wrong: OPENTELEMETRY_ABI_VERSION_NO, NO_GETENV, ENABLE_ASYNC_EXPORT, OPENTELEMETRY_HAVE_WORKING_REGEX, OPENTELEMETRY_STL_VERSION, __cplusplus >= 202002L, and one #if 0. The problem is only that the resulting CTest line is green rather than absent or skipped.
Repo wide, 40 test sources contain a TEST, TEST_F or TEST_P inside a preprocessor conditional, under the 34 CMakeLists.txt that call gtest_add_tests. Which subset goes phantom depends on the configuration, so each job has its own set.
Why it matters
A green line that ran nothing is indistinguishable from a green line that passed. I ran into it while adding cases to the Elasticsearch exporter test in #4337: eight cases were compiled out in the synchronous builds and reported eight passes there, which is the opposite of what a reader would conclude.
Options
- A convention, no build change. Do not compile a case out. Give the suite a fixture whose
SetUpcallsGTEST_SKIP, so the case is always in the binary and CTest reportsSkipped. This is what #4331 and #4337 now do. It costs aTESTtoTEST_Fchange per case, and it relies on everyone remembering. gtest_discover_tests()in place ofgtest_add_tests(). CMake's GoogleTest module enumerates from the binary rather than the source, so a compiled-out case is never registered. It runs the test binary at build or test time, which has cross compilation and Windows DLL path caveats worth checking against this job matrix first.--gtest_fail_if_no_test_selected. Turns the empty match into a failure. Not available yet: it is on googletestmainbut not inv1.17.0, which is whatthird_party_releaseandMODULE.bazelpin, so it needs a googletest bump first.
Happy to take whichever you prefer, or to leave this as a note if you would rather not churn the test CMake right now. If option 1 is the answer, I can also add a check that fails when a TEST macro appears inside a preprocessor conditional in a gtest_add_tests target, so the convention does not depend on memory.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Reproduce the issue with the default Linux CMake build and compare CTest names against each binary's --gtest_list_tests output. Start by reviewing the 34 CMakeLists.txt files that call gtest_add_tests, along with third_party_release and MODULE.bazel for the pinned googletest version. Done means compiled-out cases no longer appear as passing CTest tests while the documented build matrix remains supported.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cmake, cpp
- Domain
- build-system, testing-qa
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100