test/gtest: stop the inert-tracer test depending on NIXL_PLUGIN_DIR - #2092
Merged
ovidiusm merged 1 commit intoAug 18, 2026
Merged
Conversation
Tracing.RequestedBackendWithoutPluginIsInert asked makeTracer for the "nvtx" backend and expected a null tracer, on the premise that no libtrace_backend_*.so is registered in the unit binary. That is a property of the environment rather than of the code under test: when a real libtrace_backend_nvtx.so is discoverable -- e.g. NIXL_PLUGIN_DIR pointing at an install tree, as in the dev container, or any CI leg that installs NIXL before running the unit suite -- the plugin loads, makeTracer correctly returns a live tracer, and the test fails. Request a name no plugin can provide instead, as the sibling test MakeTracerUnknownBackendReturnsNull already does, and lift that name into one kUnloadableBackend constant so a real backend name cannot creep back in. The test's subject is unchanged: a null tracer must leave call sites on the safe default-constructed Span path. Real NVTX behaviour stays covered by the e2e TestTransferTracing tests, which load the plugin. Clearing NIXL_PLUGIN_DIR from inside the test was rejected: getPluginDir() is read once in the nixlPluginManager constructor and cached in plugin_dirs_ for the process lifetime, so the result would depend on whether an earlier test already touched the plugin manager. With the container's default NIXL_PLUGIN_DIR, all 17 Tracing tests now pass (previously 16, and green only with the variable pointed at an empty directory); the unit suite drops to 157 passed / 2 skipped / 3 failed, the remainder being object-storage tests that need an S3 endpoint. Signed-off-by: Efraim Eygin <eeygin@nvidia.com>
|
👋 Hi e-eygin! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe tracing tests add a shared guaranteed-unloadable backend constant. Plugin-loading tests reuse the constant, and the inert-tracer test now covers the unavailable-plugin runtime path instead of requesting ChangesTracing test stabilization
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change makes the tracing unit test independent of the runtime plugin environment without changing production behavior or coverage; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
/build |
Contributor
Author
|
/ok to test ee80261 |
ovidiusm
approved these changes
Aug 18, 2026
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
2 participants
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.
What?
Tracing.RequestedBackendWithoutPluginIsInertaskedmakeTracerfor thenvtxbackend and expected a null tracer. It now asks for a backend name no plugin can provide, matching what the sibling testTracing.MakeTracerUnknownBackendReturnsNullalready did; the name lives in onekUnloadableBackendconstant used by both.Why?
The old assertion rested on a property of the environment, not of the code: its own comment said "no
libtrace_backend_*.sois registered in this unit binary". Whenever a reallibtrace_backend_nvtx.sois discoverable, the plugin loads,makeTracercorrectly returns a live tracer, and the test fails. That is the case in the dev container, whereNIXL_PLUGIN_DIRpoints at an install tree containing the NVTX trace plugin, and it would be the case in any CI leg that installs NIXL before running theunitsuite.The test has been permanently red locally for weeks and was written off as "pre-existing, unrelated" in the validation notes of three telemetry PRs (#1952, #2054, #2086). Beyond the recurring explanation, a genuine regression in the null-tracer / inert-
Spanpath would have been indistinguishable from the known-red state.Coverage is unchanged: this test exists to prove that a null tracer leaves call sites on the safe default-constructed
Spanpath, which never required NVTX specifically. Real NVTX behaviour stays covered by the e2eTestTransferTracingtests, which load the actual plugin.Tracking: NIX-1710.
Rejected alternative, and verification
Clearing
NIXL_PLUGIN_DIRinside the test via the existinggtest::ScopedEnvhelper does not work reliably:getPluginDir()is read once in thenixlPluginManagerconstructor and cached inplugin_dirs_for the process lifetime, so the outcome would depend on whether an earlier test in the binary already touched the plugin manager.Verified both directions with the container's default
NIXL_PLUGIN_DIR, i.e. with no workaround applied:RequestedBackendWithoutPluginIsInert, reporting a live tracer pointer wherenullptrwas expected.Tracing.*tests pass, and the fullunitsuite is 157 passed / 2 skipped / 3 failed — the three failures beingobjCrtTestFixture.TransferBelowThresholdand the twoObjClientTests/objParamTestFixture.ReadTransferparams, which need an object-storage endpoint this container does not provide.Summary by CodeRabbit