refactor: share common drt test functions #2583
Conversation
…lpers module - Move create_test_drt_async and create_test_drt_with_settings_async from metrics and system_status_server modules to centralized location - Update all test imports to use the consolidated helpers - Eliminate code duplication across test modules
…us_server - Remove unused imports (HashMap, body, DefaultMakeSpan, TraceParent) - Consolidate std::sync imports - Separate regular tests from integration tests into distinct modules - Remove redundant feature gates inside integration_tests module - Move test command comments to appropriate module locations
WalkthroughCentralizes DistributedRuntime test helpers in distributed.rs under cfg(test) with optional integration feature. Removes duplicate helper from metrics.rs and updates tests to import the new helpers. Reorganizes system_status_server.rs tests into unit vs. integration modules and cleans up imports. No production logic changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Test
participant TR as Tokio Runtime
participant C as Config/Settings
participant H as RuntimeHandle
participant R as Runtime
participant D as DistributedRuntime
rect rgb(240,248,255)
note over T: create_test_drt_async()
T->>TR: Use current runtime
TR->>R: Build Runtime from handle
R->>D: from_settings_without_discovery()
D-->>T: DistributedRuntime
end
rect rgb(245,255,245)
note over T: create_test_drt_with_settings_async()
T->>TR: spawn_blocking(load settings)
TR-->>T: Settings
T->>H: derive handle from settings
H->>R: build Runtime
R->>D: from_settings_without_discovery()
D-->>T: DistributedRuntime
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/runtime/src/distributed.rs (1)
31-31: Duplicate/ambiguous import oferrorBoth
use super::{error, ...};(Line 27) anduse figment::error;(this line) bring the nameerrorinto scope, likely triggering E0252 (name defined multiple times). Thefigment::errorimport is unused in this file; drop or alias it.-use figment::error; +// removed: unused/duplicate import of `figment::error`lib/runtime/src/metrics.rs (1)
1343-1366: Fix order-sensitive comparison: sort the actual metric names before asserting equality
actual_drt_nats_metrics_sortedis not actually sorted; comparing to a sorted expected list can cause flaky test failures.- let actual_drt_nats_metrics_sorted: Vec<&str> = drt_nats_metric_lines - .iter() - .map(|line| { - let without_labels = line.split('{').next().unwrap_or(line); - // Remove the value part (everything after the last space) - without_labels.split(' ').next().unwrap_or(without_labels) - }) - .collect(); + let mut actual_drt_nats_metrics_sorted: Vec<&str> = drt_nats_metric_lines + .iter() + .map(|line| { + let without_labels = line.split('{').next().unwrap_or(line); + // Remove the value part (everything after the last space) + without_labels.split(' ').next().unwrap_or(without_labels) + }) + .collect(); + actual_drt_nats_metrics_sorted.sort_unstable();
🧹 Nitpick comments (5)
lib/runtime/src/distributed.rs (2)
359-373: Docstring contradicts cfg(feature="integration") gatingThe helper is described as “for basic unit tests” but is compiled only with the integration feature. Update the doc to avoid confusion.
- /// Helper function to create a DRT instance for basic unit tests - /// Uses from_current to leverage existing tokio runtime without environment configuration + /// Helper function to create a DRT instance for integration tests + /// Uses `from_current` to leverage the test tokio runtime; relies on env for configuration
379-400: Keep the two helpers DRYWith the above fix, both helpers become equivalent. Either alias the “with_settings” variant to the base helper or remove one to avoid duplication.
- pub async fn create_test_drt_with_settings_async() -> crate::DistributedRuntime { - let rt = crate::Runtime::from_current().unwrap(); - crate::DistributedRuntime::from_settings_without_discovery(rt) - .await - .unwrap() - } + pub async fn create_test_drt_with_settings_async() -> crate::DistributedRuntime { + // Delegate to the simpler helper + create_test_drt_async().await + }lib/runtime/src/system_status_server.rs (3)
451-454: Redundant cfg attribute inside an already integration-gated moduleThe module is gated with
#[cfg(all(test, feature = "integration"))]; function-level#[cfg(feature = "integration")]is redundant.- #[tokio::test] - #[cfg(feature = "integration")] + #[tokio::test]
490-491: Harden reqwest clients with timeouts to reduce test flakinessBuilding clients with a reasonable default timeout avoids hangs if the server fails to bind/respond.
- let client = reqwest::Client::new(); + let client = reqwest::Client::builder() + .timeout(std::time::Duration::from_secs(5)) + .build() + .unwrap();Apply the same change to other tests constructing
reqwest::Client(Lines 568 and 726).
606-707: High-iteration health polling: consider reducing iterations or adding jitter200 requests with strict success threshold can be slow on constrained CI runners. Add small jitter or reduce the loop count to improve stability; keep the essence of the test.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
lib/runtime/src/distributed.rs(1 hunks)lib/runtime/src/metrics.rs(5 hunks)lib/runtime/src/system_status_server.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-13T22:07:24.843Z
Learnt from: kthui
PR: ai-dynamo/dynamo#1424
File: lib/runtime/src/pipeline/network/egress/push_router.rs:204-209
Timestamp: 2025-06-13T22:07:24.843Z
Learning: The codebase uses async-nats version 0.40, not the older nats crate. Error handling should use async_nats::error::Error variants, not nats::Error variants.
Applied to files:
lib/runtime/src/metrics.rs
🧬 Code Graph Analysis (3)
lib/runtime/src/distributed.rs (2)
lib/runtime/src/config.rs (2)
from_settings(33-40)from_settings(207-211)lib/runtime/src/runtime.rs (1)
from_handle(67-71)
lib/runtime/src/system_status_server.rs (2)
lib/runtime/src/distributed.rs (2)
create_test_drt_async(367-372)create_test_drt_with_settings_async(379-400)lib/runtime/src/logging.rs (1)
load_log(867-896)
lib/runtime/src/metrics.rs (1)
lib/runtime/src/distributed.rs (1)
create_test_drt_async(367-372)
🔇 Additional comments (6)
lib/runtime/src/metrics.rs (4)
916-922: Good switch to centralized test helperImporting and using
crate::distributed::test_helpers::create_test_drt_asynckeeps tests consistent and removes duplication. Looks good.
1052-1058: LGTM on helper usageConsistent adoption of the shared helper improves maintainability.
1312-1316: LGTM on helper usage in NATS testsConsistent usage across integration tests is good.
682-910: ✔ test_metrics_registry_entry_callbacks is fully hermeticI’ve verified that the
test_metrics_registry_entry_callbacksinlib/runtime/src/metrics.rs(lines 817–910) is a plain, synchronous Rust unit test:
- It only uses
crate::MetricsRegistryEntry,std::sync::Arc, andAtomicUsize/Ordering.- It does not invoke any async runtimes (
#[tokio::test]), networking crates (nats,reqwest,hyper), or external services (etcd,std::net, etc.).- All callbacks are in-process closures returning
Ok(())/Err(...), and there are no global or integration feature-gated dependencies.No integration dependencies were detected—this test remains fully self-contained.
lib/runtime/src/system_status_server.rs (2)
300-328: Great: basic HTTP lifecycle test is lightweight and hermeticNice quick check for graceful shutdown via CancellationToken and timeout.
137-226: Server spawn pathway looks correctState initialization, route wiring, graceful shutdown with a child token, and early address bind checks look solid.
|
- Simplified create_test_drt_with_settings_async to use Runtime::from_current() - Merged create_test_drt_with_settings_async into create_test_drt_async - Both functions were identical after simplification - Updated all call sites to use the single unified function - All 188 tests pass with the simplified implementation
Removed 9 unused imports:
- crate::logging::tests::load_log
- anyhow::anyhow (kept Result)
- chrono::{DateTime, Utc}
- jsonschema::{Draft, JSONSchema}
- serde_json::Value
- std::fs::File
- std::io::{BufRead, BufReader}
- stdio_override::*
- tokio::time::sleep (kept Duration)
keivenchang
changed the title
Co-authored-by: Keiven Chang <[email protected]> Signed-off-by: Hannah Zhang <[email protected]>
Co-authored-by: Keiven Chang <[email protected]>
Co-authored-by: Keiven Chang <[email protected]> Signed-off-by: Krishnan Prashanth <[email protected]>
Co-authored-by: Keiven Chang <[email protected]> Signed-off-by: nnshah1 <[email protected]>
Overview:
Refactors test helper functions to eliminate code duplication by consolidating shared test utilities into a centralized module and cleaning up unused imports.
Details:
create_test_drt_asyncandcreate_test_drt_with_settings_asyncfunctions from multiple modules intodistributed::test_helpersWhere should the reviewer start?
Summary by CodeRabbit
Tests
Chores
No user-facing changes; functionality remains unchanged.