open-telemetry / open-telemetry/opentelemetry-cpp

[TEST] CTest reports a pass for gtest cases the binary does not contain

Open
#4,341 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

triage/accepted
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

  1. A convention, no build change. Do not compile a case out. Give the suite a fixture whose SetUp calls GTEST_SKIP, so the case is always in the binary and CTest reports Skipped. This is what #4331 and #4337 now do. It costs a TEST to TEST_F change per case, and it relies on everyone remembering.
  2. gtest_discover_tests() in place of gtest_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.
  3. --gtest_fail_if_no_test_selected. Turns the empty match into a failure. Not available yet: it is on googletest main but not in v1.17.0, which is what third_party_release and MODULE.bazel pin, 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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.