feat: integration test suite #248
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR creates a dedicated testing crate to consolidate integration tests across the codebase and improves test reliability by switching from fixed ports to dynamic socket allocation. The changes enable the testing infrastructure to be reused elsewhere (relay) while fixing "Address already in use" errors.
- Refactors the RPC server API to use
Socketobjects instead of port numbers for better resource management - Centralizes integration test infrastructure in a new
testingcrate with reusable test cluster functionality - Removes redundant integration tests from individual crates now covered by the central test suite
Reviewed Changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/testing/ | New crate providing reusable test cluster infrastructure with load simulation capabilities |
| crates/rpc/src/server.rs | Refactors server config to use Socket objects instead of ports for better resource management |
| crates/node/src/lib.rs | Updates node configuration to use new socket-based RPC server API |
| crates/db/src/lib.rs | Updates database configuration to use new socket-based RPC server API |
| crates/*/tests/integration.rs | Removes individual integration tests now consolidated in testing crate |
| .github/workflows/ci.yaml | Updates CI to run integration tests from new testing crate |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| pub fn new_low_priority(port: u16) -> io::Result<Self> { | ||
| Self::new(port, transport::Priority::High) |
There was a problem hiding this comment.
The new_low_priority method incorrectly passes Priority::High instead of Priority::Low to the new method.
| pub fn new_low_priority(port: u16) -> io::Result<Self> { | |
| Self::new(port, transport::Priority::High) | |
| Self::new(port, transport::Priority::Low) |
Copilot uses AI. Check for mistakes.
|
|
||
| cluster.redeploy_all_offline_nodes().await; | ||
|
|
||
| tokio::time::sleep(Duration::from_secs(3600)).await; |
There was a problem hiding this comment.
The test includes a 1-hour sleep which makes tests extremely slow. Consider reducing this duration or making it configurable for testing environments.
Copilot uses AI. Check for mistakes.
| async fn set_exp(&self, key: u32, exp: RecordExpiration) { | ||
| self.client | ||
| .set_exp(self.namespace, expand_key(key), exp) | ||
| .pipe(|fut| self.stats.record(fut, "get_exp")) |
There was a problem hiding this comment.
The stats recording for set_exp operation is incorrectly labeled as 'get_exp' instead of 'set_exp'.
| .pipe(|fut| self.stats.record(fut, "get_exp")) | |
| .pipe(|fut| self.stats.record(fut, "set_exp")) |
Copilot uses AI. Check for mistakes.
| let id = cfg.id(); | ||
|
|
||
| tracing::info!(ports = ?[cfg.primary_rpc_server_port, cfg.secondary_rpc_server_port], %id, "starting database server"); | ||
| tracing::info!(ports = ?[cfg.primary_rpc_server_socket.port(), cfg.secondary_rpc_server_socket.port()], %id, "starting database server"); |
There was a problem hiding this comment.
The logging statement calls .port() method which returns io::Result<u16>, but the debug formatting expects direct values. This will log Result objects instead of port numbers.
| tracing::info!(ports = ?[cfg.primary_rpc_server_socket.port(), cfg.secondary_rpc_server_socket.port()], %id, "starting database server"); | |
| tracing::info!( | |
| ports = ?[ | |
| cfg.primary_rpc_server_socket.port().unwrap_or(0), | |
| cfg.secondary_rpc_server_socket.port().unwrap_or(0) | |
| ], | |
| %id, | |
| "starting database server" | |
| ); |
Copilot uses AI. Check for mistakes.
Description
testingcrate and move most of the integration tests theretestingto be a lib that can be used elsewhere (relay)How Has This Been Tested?
Integration
Due Diligence