Skip to content

[CODE HEALTH] Move api/test and sdk/test classes into anonymous namespace - #4217

Merged
marcalff merged 2 commits into
open-telemetry:mainfrom
thc1006:codehealth/misc-internal-linkage-part2-4196
Jul 7, 2026
Merged

marcalff merged 2 commits into
open-telemetry:mainfrom
thc1006:codehealth/misc-internal-linkage-part2-4196

Conversation

@thc1006

@thc1006 thc1006 commented Jul 6, 2026

Copy link
Copy Markdown
Member

Changes

Part of #4196 (and #2053). Wraps file-local test helper classes in an anonymous namespace to enforce internal linkage, resolving the misc-use-internal-linkage warnings across 20 files under api/test and sdk/test. This follows the same approach as #4199 and #4200.

To keep the review manageable, this is the first batch. The remaining files (under exporters, ext, and functional, plus a few that need manual handling due to existing or nested namespaces) will follow in later batches. The files that overlap with my other open code-health PRs (#4215, #4216) are also deferred to avoid conflicts.

Four metrics tests that share the WritableMetricStorageTestFixture fixture (async_metric_storage_test and the sync_metric_storage counter/gauge/up_down_counter tests) are also deferred. They are linked together into //sdk/test/metrics:all_tests, and moving same-named fixtures into per-file anonymous namespaces makes gtest treat them as a redefinition of one test suite. They will be handled in a follow-up that renames the fixtures or shares one via a header.

Verification

  • Compiles clean (clang-22, abiv2 preset); a local clang-tidy run confirms the 31 warnings on these 20 files are resolved with no new warnings, and the code still builds.
  • clang-format clean on all changed files.
  • warning_limit lowered from 309/319 to 278/288 (the current baseline minus the 31 resolved here).

Note: this touches the same warning_limit line as #4215 and #4216. Whichever of these merges first, I will rebase the others accordingly (the final limit after all three is lower).

Copilot AI review requested due to automatic review settings July 6, 2026 16:37
@thc1006
thc1006 requested a review from a team as a code owner July 6, 2026 16:37

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@marcalff

marcalff commented Jul 6, 2026

Copy link
Copy Markdown
Member

CI failure, to investigate:

FAIL: //sdk/test/metrics:all_tests (Aborted) (see /home/runner/.cache/bazel/e6b8d6759295e14b76bc8cb98b604748/execroot/_main/bazel-out/k8-fastbuild/testlogs/sdk/test/metrics/all_tests/test.log)
INFO: From Testing //sdk/test/metrics:all_tests:
==================== Test output for //sdk/test/metrics:all_tests:

[ ERROR ] external/googletest+/googletest/src/gtest.cc:2838:: sdk/test/metrics/sync_metric_storage_counter_test.cc:43: Attempted redefinition of test suite WritableMetricStorageTestFixture.
All tests in the same test suite must use the same test fixture
class.  However, in test suite WritableMetricStorageTestFixture, you tried
to define a test using a fixture class different from the one
used earlier. This can happen if the two fixture classes are
from different namespaces and have the same name. You should
probably rename one of the classes to put the tests into different
test suites.```

…pace

Wrap file-local test helper classes in an anonymous namespace to enforce
internal linkage (clang-tidy misc-use-internal-linkage), across 20 files
under api/test and sdk/test, and lower the warning_limit to 278/288
accordingly. Part of open-telemetry#2053 / open-telemetry#4196.

Four metrics test files (async_metric_storage_test and
sync_metric_storage_{counter,gauge,up_down_counter}) share the fixture
class name WritableMetricStorageTestFixture. Bazel links all of
sdk/test/metrics into a single //sdk/test/metrics:all_tests binary, where
gtest keys each test suite by the fixture class name. Moving these
same-named fixtures into per-translation-unit anonymous namespaces turns
them into distinct types registered under one suite name and aborts with
"Attempted redefinition of test suite WritableMetricStorageTestFixture".
Those four files are left as-is here and deferred to a follow-up that
either renames the fixtures or hoists a shared one into a header. The
remaining files (exporters, ext, functional, and a few needing manual
handling) also follow in later batches.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
@thc1006
thc1006 force-pushed the codehealth/misc-internal-linkage-part2-4196 branch from 3a3ce6a to bdd6dff Compare July 6, 2026 17:17
@thc1006

thc1006 commented Jul 6, 2026

Copy link
Copy Markdown
Member Author

Thanks for flagging this, fixed and pushed.

Those four files (async_metric_storage_test and the sync_metric_storage counter/gauge/up_down_counter ones) all declare WritableMetricStorageTestFixture at namespace scope. That was fine while they shared a single external-linkage class, but once each moved into its own anonymous namespace they became four different classes sharing one suite name, and gtest aborts on that when //sdk/test/metrics:all_tests links them together.

Renaming as the gtest message suggests would work, but a test-suite rename feels out of place in a plain internal-linkage cleanup, so I've put those four back to their state on main and left them out of this batch. That takes the PR down to 20 files with the limit at 278/288. I'll pick the four up separately, either renaming the fixtures or hoisting a shared one into a header, whichever you'd rather see.

I didn't catch this locally because I was verifying with the CMake clang-tidy build, which links each test into its own binary and never exercises the combined target. I've added a duplicate-fixture-name check across the batch so it won't slip through again.

@codecov

codecov Bot commented Jul 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.83%. Comparing base (4d09399) to head (e7074ee).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4217   +/-   ##
=======================================
  Coverage   82.83%   82.83%           
=======================================
  Files         415      415           
  Lines       17423    17423           
=======================================
  Hits        14431    14431           
  Misses       2992     2992           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@marcalff marcalff left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the fix.

@marcalff
marcalff merged commit 4c05342 into open-telemetry:main Jul 7, 2026
72 checks passed
@thc1006
thc1006 deleted the codehealth/misc-internal-linkage-part2-4196 branch July 7, 2026 08:24
proost pushed a commit to proost/opentelemetry-cpp that referenced this pull request Jul 18, 2026
ayush-that pushed a commit to ayush-that/opentelemetry-cpp that referenced this pull request Jul 21, 2026
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>
thc1006 added a commit to thc1006/opentelemetry-cpp that referenced this pull request Jul 25, 2026
Six misc-use-internal-linkage sites, continuing from open-telemetry#4217 and open-telemetry#4286.

logger_test.cc had an anonymous namespace already, with TestLogger just
above it and TestProvider just below, so the existing namespace is
extended to cover both rather than adding two more. string_test.cc is
the same shape: its namespace closed immediately before
SplitStringTestFixture, so the closing brace moves to the end of the
file.

The two provider_test.cc files get a namespace around the test body.
In the logs one that puts the deprecated-declaration pragma push and
pop pair inside the namespace, which is fine since both ends stay on
the same side of the boundary.

Worth noting that api/test/logs holds two different classes both named
TestProvider, in logger_test.cc and provider_test.cc. They are in the
same Bazel package, so giving them internal linkage also removes a
latent one definition rule hazard.

Part of open-telemetry#4196

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
DCchoudhury15 added a commit to DCchoudhury15/opentelemetry-cpp that referenced this pull request Aug 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants