test(prover): Use separate ports when running 2 provers in test #164
Conversation
WalkthroughThe changes involve a modification to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
crates/node_types/prover/src/prover/tests.rs (2)
205-208: Consider making the port configuration more robustThe current approach of incrementing the port by 1 works but could be improved:
- Consider using a port allocation utility to ensure the chosen port is available
- Add cleanup to ensure ports are released after test completion
- Consider making the port difference configurable or using a random available port
Here's a suggested improvement:
- let mut cfg2 = cfg; - cfg2.webserver.port += 1; + let mut cfg2 = cfg; + cfg2.webserver.port = get_available_port().expect("Failed to get available port"); + + // Ensure cleanup after test + let _cleanup = TestCleanup::new(cfg2.webserver.port); // Add these utility implementations somewhere in the test module: fn get_available_port() -> std::io::Result<u16> { let socket = std::net::TcpListener::bind("127.0.0.1:0")?; Ok(socket.local_addr()?.port()) } struct TestCleanup { port: u16, } impl TestCleanup { fn new(port: u16) -> Self { Self { port } } } impl Drop for TestCleanup { fn drop(&mut self) { // Add any necessary cleanup for the port } }
Line range hint
208-214: Improve test robustness with proper task handlingThe test spawns async tasks but doesn't handle their cleanup or potential timeouts:
- Consider using
JoinHandleto properly manage spawned tasks- Add timeout handling for the assertions
- Ensure proper cleanup of resources
Here's a suggested improvement:
- let runner = prover2.clone(); - spawn(async move { runner.run().await.unwrap() }); - let epoch = prover2.clone().db.get_epoch().unwrap(); - assert_eq!(epoch, 4); - assert_eq!( - prover.get_commitment().await.unwrap(), - prover2.get_commitment().await.unwrap() - ); + let runner = prover2.clone(); + let handle = spawn(async move { runner.run().await.unwrap() }); + + // Add timeout for the assertions + let result = tokio::time::timeout(Duration::from_secs(5), async { + let epoch = prover2.clone().db.get_epoch().unwrap(); + assert_eq!(epoch, 4); + assert_eq!( + prover.get_commitment().await.unwrap(), + prover2.get_commitment().await.unwrap() + ); + }).await; + + // Cleanup + handle.abort(); + assert!(result.is_ok(), "Test timed out");
jns-ps
force-pushed
the
fix-prover-test-error
branch
from
c4a87ff to
a83d730
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
crates/node_types/prover/src/webserver.rs (1)
39-39: Consider separating test and production configurationsWhile using port 0 is excellent for testing, consider maintaining separate configuration defaults for testing and production environments. This could be achieved through feature flags or environment-specific configuration files.
Example approaches:
- Use a feature flag:
#[cfg(test)] fn default() -> Self { WebServerConfig { enabled: true, host: "127.0.0.1".to_string(), port: 0, } } #[cfg(not(test))] fn default() -> Self { WebServerConfig { enabled: true, host: "127.0.0.1".to_string(), port: 50524, } }
- Or use environment variables/configuration files to specify the port in production.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
crates/node_types/prover/src/webserver.rs(1 hunks)
🔇 Additional comments (2)
crates/node_types/prover/src/webserver.rs (2)
Line range hint 115-119: LGTM: Logging implementation correctly handles dynamic ports
The server startup logging is well implemented, using server.local_addr() to log the actual bound port. This is especially important with dynamic port allocation as it allows tests to determine which port was assigned.
39-39: LGTM: Dynamic port allocation will prevent test conflicts
The change to use port 0 is a good practice for testing scenarios, as it allows the OS to dynamically assign available ports, preventing conflicts when running multiple prover instances simultaneously.
Let's verify that the tests are actually using this configuration:
✅ Verification successful
Port 0 configuration is correctly used throughout the codebase
The verification confirms that:
- Test prover instances are created using
Config::default()which includesWebServerConfig::default() - The default port 0 configuration is consistently used across both test and production code paths
- The change is properly integrated into the configuration hierarchy
This dynamic port allocation setup will work correctly for parallel test execution.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test configurations to ensure they're using the dynamic port allocation
rg -t rust "WebServerConfig|test.*prover.*port" --no-heading
Length of output: 1006
Script:
#!/bin/bash
# Let's check test files for prover configurations and port usage
rg -t rust "test.*prover.*port|test.*webserver.*port" -A 5 --no-heading
# Also check for any test setup code that might be configuring the prover
ast-grep --pattern 'fn test_$_() {
$$$
WebServerConfig {
$$$
}
$$$
}'
Length of output: 157
Script:
#!/bin/bash
# Let's check for integration tests that might be using the prover
rg -t rust "async fn test" -A 10 crates/node_types/prover/src/
# Also check for any test modules
rg -t rust "mod tests" -A 10 crates/node_types/prover/src/
Length of output: 6713
Script:
#!/bin/bash
# Let's check the test helper function that creates the test prover
rg -t rust "create_test_prover" -A 15 crates/node_types/prover/src/prover/tests.rs
# And check the Config default implementation
ast-grep --pattern 'impl Default for Config {
$$$
}'
Length of output: 5588
Summary by CodeRabbit
New Features
Bug Fixes