Replace external relay tests with mock relay#14
Conversation
WalkthroughReplaces external test relay usage with an in-process MockRelay (via a dev dependency) and updates multinode tests to use MockRelay::run(), adds a graceful_shutdown helper, reduces test timing, and replaces prints with assertions so tests run CI-locally. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Test
participant MockRelay
participant NodeA as Node 1
participant NodeB as Node 2
Note over Test,MockRelay: Test orchestrates mock relay and nodes
Test->>MockRelay: start MockRelay::run() -> returns URL
Test->>NodeA: spawn NodeA with relay URL
Test->>NodeB: spawn NodeB with relay URL
NodeA->>MockRelay: CONNECT / ANNOUNCE
NodeB->>MockRelay: CONNECT / ANNOUNCE
MockRelay-->>NodeA: relay events (peer announcements)
MockRelay-->>NodeB: relay events (peer announcements)
Note right of NodeA: NodeA processes PeerDiscovered events
Note right of NodeB: NodeB processes PeerDiscovered events
Test->>NodeA: initiate signing flow (if applicable)
NodeA->>MockRelay: publish signing metadata / messages
MockRelay-->>NodeB: deliver signing messages
alt successful flow
NodeB-->>NodeA: responses via relay
Test->>Test: assertions on signing results & PeerDiscovered events
end
Test->>NodeA: graceful_shutdown(shutdown_tx, handle)
Test->>NodeB: graceful_shutdown(shutdown_tx, handle)
Test->>MockRelay: shutdown
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (5)
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. Comment |
![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
🧹 Nitpick comments (3)
keep-frost-net/tests/multinode_test.rs (3)
11-11: Consider more specific imports for clarity.While wildcard imports from prelude modules are acceptable in test code, being more explicit (e.g.,
use nostr_relay_builder::prelude::MockRelay;) can improve code clarity and make dependencies more obvious.
86-97: Consider adding context parameter back to graceful_shutdown.The simplified
graceful_shutdownhelper removes thenameparameter, which eliminates debugging context when timeouts occur. While the simplification is cleaner, consider keeping a context parameter for better test diagnostics.🔎 Proposed enhancement with context parameter
async fn graceful_shutdown( shutdown_tx: Option<mpsc::Sender<()>>, handle: tokio::task::JoinHandle<()>, + context: &str, ) { if let Some(tx) = shutdown_tx { let _ = tx.try_send(()); } - let _ = timeout(Duration::from_secs(2), handle).await; + if timeout(Duration::from_secs(2), handle).await.is_err() { + eprintln!("Warning: {} shutdown timed out", context); + } } -graceful_shutdown(shutdown1, node1_handle).await; -graceful_shutdown(shutdown2, node2_handle).await; +graceful_shutdown(shutdown1, node1_handle, "node1").await; +graceful_shutdown(shutdown2, node2_handle, "node2").await;
210-222: Extract duplicated graceful_shutdown helper to module level.The
graceful_shutdownhelper function is duplicated in bothtest_peer_discovery_with_running_nodes(lines 86-94) andtest_full_signing_flow(lines 210-218). Consider extracting it to the module level to eliminate duplication.🔎 Proposed refactor to eliminate duplication
Add at the module level (after imports, before tests):
async fn graceful_shutdown( shutdown_tx: Option<mpsc::Sender<()>>, handle: tokio::task::JoinHandle<()>, ) { if let Some(tx) = shutdown_tx { let _ = tx.try_send(()); } let _ = timeout(Duration::from_secs(2), handle).await; }Then remove the duplicate function definitions from within both test functions.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
keep-frost-net/Cargo.tomlkeep-frost-net/tests/multinode_test.rs
🧰 Additional context used
🧬 Code graph analysis (1)
keep-frost-net/tests/multinode_test.rs (1)
keep-frost-net/src/node.rs (1)
share_index(102-104)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (5)
keep-frost-net/tests/multinode_test.rs (4)
75-79: Excellent addition of explicit peer discovery assertion.The added assertion verifying that Node 1 discovers Node 2 (not itself) significantly improves test reliability and makes the expected behavior explicit.
99-99: Good addition of final assertion.The explicit assertion that peer discovery didn't time out improves test clarity and ensures the test properly validates the expected behavior.
202-202: Verify the 3-second sleep duration is sufficient.The sleep duration was adjusted for the mock relay runtime. Please verify that 3 seconds provides sufficient time for node initialization and peer discovery before initiating the signing request, especially in CI environments with variable performance.
15-16: MockRelay resource cleanup is handled automatically via Drop.MockRelay (from nostr-relay-builder) wraps LocalRelay, which implements automatic cleanup through a shutdown notification pattern triggered when the instance is dropped. When the test function ends and
mock_relaygoes out of scope, the shutdown notification properly terminates the relay's internal tasks and cleans up resources. No explicit shutdown call is necessary, and test isolation is maintained.keep-frost-net/Cargo.toml (1)
30-30: Dependency verified. Version 0.44.0 ofnostr-relay-builderis valid, not yanked, and has no known security advisories. The version alignment withnostr-sdkis appropriate.
kwsantiago
force-pushed
the
Add-mock-relay-tests
branch
from
894f7c2 to
77ce30a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
keep-frost-net/tests/multinode_test.rs (1)
210-218: Duplicate helper function definition.This
graceful_shutdownfunction is identical to the one defined intest_peer_discovery_with_running_nodes(lines 86-94). See the earlier comment for the refactoring suggestion to extract it to module level.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
keep-frost-net/Cargo.tomlkeep-frost-net/tests/multinode_test.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- keep-frost-net/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (7)
keep-frost-net/tests/multinode_test.rs (7)
15-16: LGTM! Clean migration to MockRelay.The MockRelay setup is straightforward and replaces the external relay dependency effectively.
42-43: LGTM! Consistent MockRelay setup.The MockRelay initialization follows the same clean pattern as other tests.
75-80: Good addition of assertion for peer discovery.The assertion
assert_eq!(share_index, 2, ...)ensures Node 1 discovers the correct peer (Node 2), improving test reliability.
99-99: Good assertion to verify discovery success.The final assertion ensures the peer discovery completed within the timeout, making the test more robust.
160-161: LGTM! Consistent MockRelay setup.The MockRelay initialization follows the established pattern used across all tests.
202-202: Appropriate timing adjustment for MockRelay.The 3-second startup delay is reasonable for the mock relay environment and should be more consistent than the previous 5-second delay with external relays.
220-222: LGTM! Proper cleanup of all nodes.The graceful shutdown calls ensure all three node tasks are properly cleaned up after the test completes.
kwsantiago
force-pushed
the
Add-mock-relay-tests
branch
from
77ce30a to
6682837
Compare
2 participants
Use nostr-relay-builder MockRelay for integration tests
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.