[CODE HEALTH] Move SDK common and logs test helpers into anonymous namespaces#4302
Draft
thc1006 wants to merge 3 commits into
Draft
[CODE HEALTH] Move SDK common and logs test helpers into anonymous namespaces#4302thc1006 wants to merge 3 commits into
thc1006 wants to merge 3 commits into
Conversation
…mespaces Three misc-use-internal-linkage sites, continuing from open-telemetry#4217 and open-telemetry#4286. global_log_handle_singleton_lifetime_test.cc had an anonymous namespace holding only ConstructChecker(), with GlobalLogHandlerChecker above it and CustomLogHandler below, so the existing namespace is extended over both rather than joined by a second one. That file is a destruction-order test whose checker aborts if the global log handler outlives it, so I built and ran it rather than reasoning about it. The order is unchanged: the handler is destroyed first and the checker's check passes. An anonymous namespace changes linkage, not storage duration, and the function-local static is still constructed on its first call from the test body. logger_provider_set_test.cc gets a namespace around the test body. The warning limit drops by three on both presets, 151 to 148 and 161 to 158. None of these three are ABI gated. Part of open-telemetry#4196 Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4302 +/- ##
=======================================
Coverage 81.31% 81.31%
=======================================
Files 445 445
Lines 18859 18859
=======================================
Hits 15333 15333
Misses 3526 3526 🚀 New features to boost your workflow:
|
Review of open-telemetry#4302 pointed out the checker's assertion could pass for the wrong reason. The predicate was `handle && custom_handler_destroyed`, which only aborts when the handler destructor has already run yet a handle is still reachable. If the checker were destroyed first, the case the test exists to catch, custom_handler_destroyed is still false and the whole predicate is false, so it would not abort. It is now `!custom_handler_destroyed || handle`: abort if the checker outruns the handler destructor, or if the handler is still reachable. This is a pre-existing weakness rather than something the namespace move introduced, but the PR described the test as failing loudly on a reversed order, so the test should actually do that. The correct order still passes: the handler is destroyed first and the checker's check passes. Part of open-telemetry#4196 Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
thc1006
added a commit
to thc1006/opentelemetry-cpp
that referenced
this pull request
Jul 25, 2026
…namespaces Nine misc-use-internal-linkage sites, the last of three batches after open-telemetry#4301 and open-telemetry#4302. tracer_test.cc had an anonymous namespace holding only initTracer(), with the three mock classes above it, so that namespace is extended over them rather than joined by a second one. measurements_benchmark.cc is the same shape, with MockMetricExporter pulled into the existing namespace. The other three get a namespace around the test body. batch_span_processor_test.cc already sits inside a named namespace via OPENTELEMETRY_BEGIN_NAMESPACE; the anonymous namespace nests inside it and covers every class and TEST in the file. Its BatchSpanProcessorTestPeer and the metrics file's AlignedHistogramBucketExemplarReservoirTestPeer are the only TEST_F suite names here, and each is unique to its file, so this does not repeat the all_tests redefinition from open-telemetry#4217. The warning limit drops by nine on both presets, 151 to 142 and 161 to 152. None of these are ABI gated. This branch is cut from main. If open-telemetry#4301 or open-telemetry#4302 merge first, the limit needs recomputing down by their counts as well. Part of open-telemetry#4196 Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #4196
Draft while CI runs. Ready for review from my side.
Three
misc-use-internal-linkagesites in the SDK tests, the second of three batches after #4301.sdk/test/common/global_log_handle_singleton_lifetime_test.ccGlobalLogHandlerChecker,CustomLogHandlersdk/test/logs/logger_provider_set_test.ccTestProviderThe one that needed care
global_log_handle_singleton_lifetime_test.ccis a destruction-order test:GlobalLogHandlerChecker's destructor must run after the global log handler and aborts otherwise.Review noted that the existing assertion could not actually catch a reversed order:
handle && custom_handler_destroyedonly aborts when the handler destructor has run yet a handle is still reachable, so a checker destroyed first (the case the test is for) slipped through. That is a pre-existing weakness, not something the namespace move caused, but since this PR leans on the test I tightened the predicate to!custom_handler_destroyed || handleso a reversed order now aborts. The correct order still passes.It already had an anonymous namespace holding only
ConstructChecker(), with one flagged class above it and the other below, so I extended that namespace over both instead of adding a second one.I built and ran the test rather than reasoning about it:
The handler is destroyed first and the checker's check passes, which is the invariant the test exists for. That is what I expected, since an anonymous namespace changes linkage rather than storage duration and the function-local static is still constructed on its first call from the test body, but the test is cheap to run and reasoning about destruction order is a bad place to be confident.
logger_provider_set_test.ccis the ordinary case: a namespace around the test body.Warning limit
151 to 148 and 161 to 158. Three on each preset, since none of these are ABI gated, unlike the
TestEventLoggerProvidersite in #4301.This branch is cut from main rather than stacked on #4301, so if that one merges first this needs a rebase and the limit recomputed to 142 and 153. Happy to do that whenever it is useful; I kept them independent so they can be reviewed in either order.
Verification
TestProviderend up inside an anonymous namespace, checked by walking namespace depth, and both files have balanced open and close.-DOPENTELEMETRY_ABI_VERSION_NO=1and=2.sdk/test/commonnorsdk/test/logsshares aTESTsuite name across files, so this should not repeat theall_testsredefinition from [CODE HEALTH] Move api/test and sdk/test classes into anonymous namespace #4217.Same honest gap as #4301: I could not re-run
misc-use-internal-linkagelocally, because it only reports classes from llvm-22 onward and clang-tidy 18, 20 and 21 report functions and variables only, so a local run returns zero regardless. The site list comes from the CI log artifact for main, and CI is the authority on the resulting count.