test(api-core): extract reusable API test support builder#2149
Open
poroh wants to merge 1 commit into
Open
Conversation
Move shared API test fixture helpers into `test_support`, including network fixtures, IB fabric setup, and a `TestApiBuilder` for constructing test `Api` instances with configurable dependencies. Refactor existing API fixtures to use the new builder while preserving current test environment behavior. Signed-off-by: Dmitry Porokh <dporokh@nvidia.com>
Contributor
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/api-core/src/test_support/builder.rs (1)
151-243: ⚡ Quick winAdd module-level documentation for
TestApiBuilder.The builder is a critical test infrastructure component but lacks documentation explaining its purpose, usage, and relationship to test fixtures.
📝 Proposed documentation
Add before the struct definition:
/// Builder for constructing `Api` instances in tests. /// /// This builder provides a fluent interface for creating test `Api` instances /// with configurable dependencies. Required fields (database pool, common pools, /// and work lock manager) must be provided to `new()`, while optional components /// can be overridden via `with_*` methods. /// /// # Examples /// /// ```no_run /// let api = TestApiBuilder::new(db_pool, common_pools, work_lock_manager) /// .with_runtime_config(config) /// .with_credential_manager(creds) /// .build(); /// ``` pub struct TestApiBuilder {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-core/src/test_support/builder.rs` around lines 151 - 243, Add module-level documentation for the TestApiBuilder struct: create a doc comment block immediately before the TestApiBuilder struct definition describing its purpose as a builder for constructing Api instances in tests, noting required inputs (database pool, common pools, work lock manager), optional with_* override methods, and include a brief usage example showing new(...).with_runtime_config(...).with_credential_manager(...).build(); reference the TestApiBuilder and Api types so readers know what it constructs.crates/api-core/src/test_support/ib_fabric.rs (1)
26-60: ⚡ Quick winReplace
unwrap()withexpect()to provide panic context.Line 58 uses
unwrap()when creating the IB fabric manager. In test support code, panics should provide clear diagnostic context to aid debugging.♻️ Proposed fix with diagnostic context
) - .unwrap() + .expect("Failed to create test IB fabric manager") .into() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-core/src/test_support/ib_fabric.rs` around lines 26 - 60, In ib_fabric_test_manager, replace the bare unwrap() on the result of ib::create_ib_fabric_manager(...) with expect(...) to provide a clear panic message; update the call site (the unwrap() after ib::create_ib_fabric_manager in ib_fabric_test_manager) to use expect("failed to create IB Fabric manager for test: <brief context>") so test failures include diagnostic context when create_ib_fabric_manager returns an Err.crates/api-core/src/test_support/network.rs (1)
36-104: ⚡ Quick winRefactor repetitive
IpNetwork::new().unwrap()calls into a helper or iterator pattern.The lazy_static block contains 11 nearly identical repetitive blocks, each calling
IpNetwork::new(...).unwrap(). This pattern violates DRY principles and increases maintenance burden.♻️ Proposed refactor using iterator pattern
lazy_static! { pub static ref TEST_SITE_PREFIXES: Vec<IpNetwork> = vec![ - IpNetwork::new( - FIXTURE_ADMIN_NETWORK_SEGMENT_GATEWAY.network(), - FIXTURE_ADMIN_NETWORK_SEGMENT_GATEWAY.prefix(), - ) - .unwrap(), - IpNetwork::new( - FIXTURE_UNDERLAY_NETWORK_SEGMENT_GATEWAY.network(), - FIXTURE_UNDERLAY_NETWORK_SEGMENT_GATEWAY.prefix(), - ) - .unwrap(), - IpNetwork::new( - FIXTURE_TENANT_NETWORK_SEGMENT_GATEWAYS[0].network(), - FIXTURE_TENANT_NETWORK_SEGMENT_GATEWAYS[0].prefix(), - ) - .unwrap(), - IpNetwork::new( - FIXTURE_TENANT_NETWORK_SEGMENT_GATEWAYS[1].network(), - FIXTURE_TENANT_NETWORK_SEGMENT_GATEWAYS[1].prefix(), - ) - .unwrap(), - IpNetwork::new( - FIXTURE_TENANT_NETWORK_SEGMENT_GATEWAYS[2].network(), - FIXTURE_TENANT_NETWORK_SEGMENT_GATEWAYS[2].prefix(), - ) - .unwrap(), - IpNetwork::new( - FIXTURE_TENANT_NETWORK_SEGMENT_GATEWAYS[3].network(), - FIXTURE_TENANT_NETWORK_SEGMENT_GATEWAYS[3].prefix(), - ) - .unwrap(), - IpNetwork::new( - FIXTURE_TENANT_NETWORK_SEGMENT_GATEWAYS[4].network(), - FIXTURE_TENANT_NETWORK_SEGMENT_GATEWAYS[4].prefix(), - ) - .unwrap(), - IpNetwork::new( - FIXTURE_TENANT_NETWORK_SEGMENT_GATEWAYS[5].network(), - FIXTURE_TENANT_NETWORK_SEGMENT_GATEWAYS[5].prefix(), - ) - .unwrap(), - IpNetwork::new( - FIXTURE_TENANT_NETWORK_SEGMENT_GATEWAYS[6].network(), - FIXTURE_TENANT_NETWORK_SEGMENT_GATEWAYS[6].prefix(), - ) - .unwrap(), - IpNetwork::new( - FIXTURE_TENANT_NETWORK_SEGMENT_GATEWAYS[7].network(), - FIXTURE_TENANT_NETWORK_SEGMENT_GATEWAYS[7].prefix(), - ) - .unwrap(), - IpNetwork::new( - FIXTURE_TENANT_NETWORK_SEGMENT_GATEWAYS[8].network(), - FIXTURE_TENANT_NETWORK_SEGMENT_GATEWAYS[8].prefix(), - ) - .unwrap(), - IpNetwork::new( - FIXTURE_TENANT_NETWORK_SEGMENT_GATEWAYS[9].network(), - FIXTURE_TENANT_NETWORK_SEGMENT_GATEWAYS[9].prefix(), - ) - .unwrap(), - IpNetwork::new( - FIXTURE_TENANT_NETWORK_SEGMENT_GATEWAYS[10].network(), - FIXTURE_TENANT_NETWORK_SEGMENT_GATEWAYS[10].prefix(), - ) - .unwrap(), + FIXTURE_ADMIN_NETWORK_SEGMENT_GATEWAY, + FIXTURE_UNDERLAY_NETWORK_SEGMENT_GATEWAY, + ] + .into_iter() + .chain(FIXTURE_TENANT_NETWORK_SEGMENT_GATEWAYS.iter().copied()) + .map(|gw| IpNetwork::new(gw.network(), gw.prefix()).unwrap()) + .collect(); - ]; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-core/src/test_support/network.rs` around lines 36 - 104, TEST_SITE_PREFIXES currently repeats IpNetwork::new(...).unwrap() for each fixture; refactor by building the Vec via an iterator or small helper to avoid duplication: create either a helper function (e.g., fn make_ipnetwork(gw: &GatewayType) -> IpNetwork { IpNetwork::new(gw.network(), gw.prefix()).unwrap() }) and call it for each fixture, or collect from an iterator over a slice of fixtures (e.g., [FIXTURE_ADMIN_NETWORK_SEGMENT_GATEWAY, FIXTURE_UNDERLAY_NETWORK_SEGMENT_GATEWAY].iter().chain(FIXTURE_TENANT_NETWORK_SEGMENT_GATEWAYS.iter()).map(|g| IpNetwork::new(g.network(), g.prefix()).unwrap()).collect()) inside the lazy_static block to produce TEST_SITE_PREFIXES; update any symbol names used (TEST_SITE_PREFIXES, FIXTURE_TENANT_NETWORK_SEGMENT_GATEWAYS, IpNetwork::new) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/api-core/src/test_support/builder.rs`:
- Around line 151-243: Add module-level documentation for the TestApiBuilder
struct: create a doc comment block immediately before the TestApiBuilder struct
definition describing its purpose as a builder for constructing Api instances in
tests, noting required inputs (database pool, common pools, work lock manager),
optional with_* override methods, and include a brief usage example showing
new(...).with_runtime_config(...).with_credential_manager(...).build();
reference the TestApiBuilder and Api types so readers know what it constructs.
In `@crates/api-core/src/test_support/ib_fabric.rs`:
- Around line 26-60: In ib_fabric_test_manager, replace the bare unwrap() on the
result of ib::create_ib_fabric_manager(...) with expect(...) to provide a clear
panic message; update the call site (the unwrap() after
ib::create_ib_fabric_manager in ib_fabric_test_manager) to use expect("failed to
create IB Fabric manager for test: <brief context>") so test failures include
diagnostic context when create_ib_fabric_manager returns an Err.
In `@crates/api-core/src/test_support/network.rs`:
- Around line 36-104: TEST_SITE_PREFIXES currently repeats
IpNetwork::new(...).unwrap() for each fixture; refactor by building the Vec via
an iterator or small helper to avoid duplication: create either a helper
function (e.g., fn make_ipnetwork(gw: &GatewayType) -> IpNetwork {
IpNetwork::new(gw.network(), gw.prefix()).unwrap() }) and call it for each
fixture, or collect from an iterator over a slice of fixtures (e.g.,
[FIXTURE_ADMIN_NETWORK_SEGMENT_GATEWAY,
FIXTURE_UNDERLAY_NETWORK_SEGMENT_GATEWAY].iter().chain(FIXTURE_TENANT_NETWORK_SEGMENT_GATEWAYS.iter()).map(|g|
IpNetwork::new(g.network(), g.prefix()).unwrap()).collect()) inside the
lazy_static block to produce TEST_SITE_PREFIXES; update any symbol names used
(TEST_SITE_PREFIXES, FIXTURE_TENANT_NETWORK_SEGMENT_GATEWAYS, IpNetwork::new)
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5be81b61-52e4-4c5f-851c-a3e36c9a0519
📒 Files selected for processing (6)
crates/api-core/src/test_support/builder.rscrates/api-core/src/test_support/ib_fabric.rscrates/api-core/src/test_support/mod.rscrates/api-core/src/test_support/network.rscrates/api-core/src/test_support/network_segment.rscrates/api-core/src/tests/common/api_fixtures/mod.rs
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.
Description
Move shared API test fixture helpers into
test_support, including network fixtures, IB fabric setup, and aTestApiBuilderfor constructing testApiinstances with configurable dependencies. Refactor existing API fixtures to use the new builder while preserving current test environment behavior.Type of Change
Related Issues (Optional)
#2001
Breaking Changes
Testing
Additional Notes