Test suite#1
Conversation
|
Warning Rate limit exceeded@datagutt has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 5 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (12)
WalkthroughVisibility of many internals was increased (mostly gated for tests), a crate/library and binary were declared, sender selection and connection-management APIs were expanded, a new utils::now_ms was introduced, extensive test helpers and unit/integration/end-to-end tests were added, and CI/Cargo test/profile settings were introduced. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Caller
participant Tog as DynamicToggles
participant Sel as select_connection_idx
participant Conns as Vec<SrtlaConnection>
App->>Tog: read flags (classic, quality, explore)
App->>Sel: select_connection_idx(conns, last_idx, last_switch, enable_quality, enable_explore, classic)
Sel->>Conns: compute base score per connection
rect rgba(200,230,255,0.25)
note right of Sel: New scoring logic (quality multiplier, penalties, explore)
Sel-->>Sel: apply quality multiplier & burst/NAK penalties
Sel-->>Sel: enforce minimum final score
Sel-->>Sel: optionally choose 2nd-best when exploring
end
Sel-->>App: Option<usize> selected index
sequenceDiagram
autonumber
participant App as Caller
participant AC as apply_connection_changes
participant Conns as Vec<SrtlaConnection>
participant Map as seq_to_conn
participant LS as last_selected_idx
App->>AC: apply_connection_changes(&mut Conns, new_ips, host, port, &mut LS, &mut Map)
AC->>Conns: remove stale entries, add new connections
rect rgba(220,255,220,0.25)
note right of AC: Side-effect updates for selection & mapping
AC->>Map: reset/rebuild seq→conn mapping for removed connections
AC->>LS: update/clear last_selected_idx to preserve or reset stickiness
end
AC-->>App: ()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (28)
src/registration.rs (2)
163-169: Consider code duplication with connection module.There are now two identical
now_ms()functions - one insrc/registration.rsand one insrc/connection.rs. Consider consolidating into a shared utility module.-pub fn now_ms() -> u64 { - use std::time::{SystemTime, UNIX_EPOCH}; - SystemTime::now() - .duration_since(UNIX_EPOCH) - .unwrap() - .as_millis() as u64 -} +// Remove this function and use crate::connection::now_ms insteadOr create a shared utility module:
// src/utils.rs pub fn now_ms() -> u64 { use std::time::{SystemTime, UNIX_EPOCH}; SystemTime::now() .duration_since(UNIX_EPOCH) .unwrap() .as_millis() as u64 }
9-16: Restrict registration state field visibility
These six fields are only accessed by your tests; make them private and expose viapub(crate)or public accessor methods to avoid leaking internal implementation details.tests/integration_tests.rs (3)
181-185: Avoid partial type corruption when testing wrong typesOnly overwriting the high byte may accidentally collide with valid 0x80xx control types. Write both bytes from an explicit non-KEEPALIVE control type.
- wrong_type[0] = 0x80; + wrong_type[0..2].copy_from_slice(&SRT_TYPE_ACK.to_be_bytes());
208-209: Replace magic REG3 bytes with constantDerive the packet from SRTLA_TYPE_REG3 to avoid brittleness if constants change.
- let reg3_packet = vec![0x92, 0x02]; + let reg3_packet = vec![ + (SRTLA_TYPE_REG3 >> 8) as u8, + (SRTLA_TYPE_REG3 & 0xFF) as u8, + ];
247-258: Hard-coding SRTLA_ID_LEN to 256 can make future changes noisyIf the spec evolves, this test will gate refactors. Consider asserting a sane range and using a single spec-check elsewhere.
src/tests/registration_tests.rs (5)
35-37: Timing flakiness: allow larger skewCI variance can exceed 10ms. Relax the bound.
- assert!(reg.reg1_next_send_at_ms <= current_time + 10); // Should be very soon + assert!(reg.reg1_next_send_at_ms <= current_time + 100); // Allow CI scheduler skew
69-71: Use REG3 constant instead of magic bytesPrevents silent breakage if constants change.
- let buf = vec![0x92, 0x02]; + let buf = vec![ + (SRTLA_TYPE_REG3 >> 8) as u8, + (SRTLA_TYPE_REG3 & 0xFF) as u8, + ];And similarly for the other occurrences.
Also applies to: 175-176, 224-225
106-111: Use SRT_TYPE_ACK constant for “unrecognized packet”Clearer intent and future-proof.
- let buf = vec![0x80, 0x02, 0x00, 0x00]; // SRT ACK + let mut buf = vec![0u8; 4]; + buf[0..2].copy_from_slice(&SRT_TYPE_ACK.to_be_bytes());
187-196: Make this a behavioral test, not a field readCurrently it only asserts the field was set. Convert to an async test and verify reg_driver_send_if_needed does not send when next-send is in the future.
- #[test] - fn test_reg_driver_timing() { + #[tokio::test] + async fn test_reg_driver_timing() { let mut reg = SrtlaRegistrationManager::new(); - // Set future send time to prevent immediate sending reg.reg1_next_send_at_ms = crate::registration::now_ms() + 5000; - // Even with connections available, should not send yet - assert!(reg.reg1_next_send_at_ms > crate::registration::now_ms()); + let mut connections = vec![create_test_connection().await]; + reg.reg_driver_send_if_needed(&mut connections).await; + assert_eq!(reg.pending_reg2_idx, None, "Should not send before next-send time"); }
231-238: Avoid randomness-dependent flakesTwo draws can collide. Sample multiple managers and assert not all equal.
- let reg1 = SrtlaRegistrationManager::new(); - let reg2 = SrtlaRegistrationManager::new(); - assert_ne!(reg1.srtla_id, reg2.srtla_id); + let ids: Vec<_> = (0..8).map(|_| SrtlaRegistrationManager::new().srtla_id).collect(); + let all_same = ids.windows(2).all(|w| w[0] == w[1]); + assert!(!all_same, "All generated IDs were identical unexpectedly");src/tests/protocol_tests.rs (3)
85-91: Change both bytes for “invalid type” checksSet both bytes to a known non-KEEPALIVE control to avoid accidental matches.
- pkt[0] = 0x80; + pkt[0..2].copy_from_slice(&SRT_TYPE_ACK.to_be_bytes());
207-211: Prefer SRTLA_TYPE_REG3 when crafting the packetRemoves magic numbers.
- let reg3_pkt = vec![0x92, 0x02]; + let reg3_pkt = vec![ + (SRTLA_TYPE_REG3 >> 8) as u8, + (SRTLA_TYPE_REG3 & 0xFF) as u8, + ];
1-233: Deduplicate overlapping protocol testsThis file and tests/integration_tests.rs validate many of the same paths. Consider extracting shared builders/asserts into a helper module to reduce duplication.
tests/end_to_end_tests.rs (4)
85-93: Increase UDP recv timeout to deflake under CI load100ms is tight on shared runners.
- let result = timeout(Duration::from_millis(100), socket2.recv_from(&mut buffer)).await; + let result = timeout(Duration::from_millis(500), socket2.recv_from(&mut buffer)).await;
156-165: Relax timing assertions for keepaliveScheduling jitter can make exact millisecond windows flaky.
- assert!( - timestamp_range >= 100, - "Timestamp range should be at least 100ms" - ); - assert!( - timestamp_range <= elapsed_ms + 50, - "Timestamps should not exceed actual elapsed time by much" - ); + assert!(timestamp_range >= 80, "Expected >= ~2*50ms with margin"); + assert!(timestamp_range <= elapsed_ms + 150, "Allow scheduler jitter");
295-317: Memory usage probe is a stub; gate or drop to avoid confusionThe used_memory() impl returns None, so the check is effectively disabled. Either remove the memory accounting or cfg-gate it by platform.
- let start_memory = std::alloc::System.used_memory().unwrap_or(0); - let naks = parse_srt_nak(&large_nak); - let end_memory = std::alloc::System.used_memory().unwrap_or(0); + let naks = parse_srt_nak(&large_nak); // Should be limited to 1000 items max assert!(naks.len() <= 1000); - if end_memory > start_memory { - let memory_used = end_memory - start_memory; - assert!(memory_used < 1_000_000, "Memory usage should be bounded"); - }Or wrap the memory section with a cfg and document supported targets.
Also applies to: 320-329
205-216: MTU-bound ACKs: add an equality assertionStrengthen by asserting exact size when divisible.
- assert!(packet.len() <= MTU, "ACK packet should not exceed MTU"); + assert!(packet.len() <= MTU, "ACK packet should not exceed MTU"); + assert_eq!(packet.len(), 2 + 4 * max_acks, "Expected full MTU utilization");src/tests/connection_tests.rs (3)
7-26: Prefer #[tokio::test] to avoid per-test Runtime boilerplateThese tests only need async once to create the connection. Switching to #[tokio::test(flavor = "current_thread")] reduces overhead and noise.
- #[test] - fn test_connection_score() { - let rt = tokio::runtime::Runtime::new().unwrap(); - let mut conn = rt.block_on(create_test_connection()); + #[tokio::test(flavor = "current_thread")] + async fn test_connection_score() { + let mut conn = create_test_connection().await;Apply similarly to other tests that spin up a Runtime.
299-307: Potentially surprising in_flight_packets growthRegistering PKT_LOG_SIZE+10 packets increases in_flight_packets linearly but overwrites the circular log. If invariants expect in_flight <= PKT_LOG_SIZE, consider capping or documenting.
68-75: Window growth/recovery assertions could be strongerYou can assert strict monotonicity or expected increments to catch regressions.
Also applies to: 170-186, 275-292
src/sender.rs (4)
24-28: Consider documentation for the public struct.Since
PendingConnectionChangesis now part of the public API, consider adding documentation comments to describe its purpose and when it should be used.Add documentation:
+/// Represents pending connection configuration changes to be applied safely between processing cycles. +/// This ensures connection state changes don't occur during active packet processing. pub struct PendingConnectionChanges { + /// New IP addresses to use for connections. If None, no IP changes are applied. pub new_ips: Option<Vec<IpAddr>>, + /// The receiver hostname for the connections. pub receiver_host: String, + /// The receiver port for the connections. pub receiver_port: u16, }
388-482: Consider extracting quality scoring logic for better testability.The
select_connection_idxfunction has grown complex with multiple scoring paths. Consider extracting the quality scoring calculation into a separate pure function for easier testing and maintenance.Extract quality scoring:
fn calculate_quality_multiplier(conn: &SrtlaConnection) -> f64 { if let Some(tsn) = conn.time_since_last_nak_ms() { let mut quality_mult = if tsn < 2000 { 0.1 } else if tsn < 5000 { 0.5 } else if tsn < 10_000 { 0.8 } else if conn.total_nak_count() == 0 { 1.2 } else { 1.0 }; // Extra penalty for burst NAKs if conn.nak_burst_count() > 1 && tsn < 5000 { quality_mult *= 0.5; } quality_mult } else if conn.total_nak_count() == 0 { 1.2 } else { 1.0 } }
446-449: Logic duplication in quality scoring.There's a subtle logic issue here: when
tsn >= 10_000andtotal_nak_count() == 0, the multiplier is set to 1.2 (line 439), but this condition at lines 446-449 would also match, potentially causing confusion. The current code works correctly due to the if-else structure, but consider clarifying the logic.The condition at lines 446-449 should explicitly check that we're not in the
Some(tsn)branch to make the logic clearer:- } else if c.total_nak_count() == 0 { + } else if c.time_since_last_nak_ms().is_none() && c.total_nak_count() == 0 { // Bonus for connections that have never had NAKs quality_mult = 1.2;
539-547: Consider using filter_map for cleaner code.The current implementation works correctly but could be more idiomatic.
- let new_ips_needed: Vec<IpAddr> = new_ips - .iter() - .filter(|ip| { - let label = format!("{}:{} via {}", receiver_host, receiver_port, ip); - !current_labels.contains(&label) - }) - .copied() - .collect(); + let new_ips_needed: Vec<IpAddr> = new_ips + .iter() + .copied() + .filter(|ip| { + let label = format!("{}:{} via {}", receiver_host, receiver_port, ip); + !current_labels.contains(&label) + }) + .collect();src/connection.rs (4)
220-226: Document the now-public register_packet method.Since
register_packetis now part of the public API, it should have documentation explaining its purpose and when to call it.Add documentation:
+ /// Registers a packet in the connection's tracking log for congestion control. + /// This should be called after successfully sending a packet with the given sequence number. + /// + /// # Arguments + /// * `seq` - The SRT sequence number of the sent packet pub fn register_packet(&mut self, seq: i32) {
365-397: Document the handle_srtla_ack_specific return value.The function returns a boolean indicating whether the sequence was found, but this isn't documented.
Add documentation:
+ /// Handles a SRTLA ACK for a specific sequence number. + /// Returns true if the sequence was found in the packet log, false otherwise. pub fn handle_srtla_ack_specific(&mut self, seq: i32) -> bool {
369-369: Remove unnecessary blank lines.These blank lines don't add clarity and could be removed for consistency with the rest of the codebase.
let mut found = false; - // Search backwards through the packet log (most recent first)self.packet_log[idx] = -1; - // Window increase logic from original implementationAlso applies to: 379-379
635-641: Extractnow_msinto a shared utility moduleMove the duplicate implementations in
src/connection.rs:635–641andsrc/registration.rs:163–169into a new (or existing) common module (e.g.src/time_utils.rs) and import it from both places.
📜 Review details
Configuration used: CodeRabbit UI
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 ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
.cargo/config.toml(1 hunks)Cargo.toml(1 hunks)src/connection.rs(5 hunks)src/lib.rs(1 hunks)src/main.rs(1 hunks)src/registration.rs(2 hunks)src/sender.rs(11 hunks)src/test_helpers.rs(1 hunks)src/tests/connection_tests.rs(1 hunks)src/tests/mod.rs(1 hunks)src/tests/protocol_tests.rs(1 hunks)src/tests/registration_tests.rs(1 hunks)src/tests/sender_tests.rs(1 hunks)src/tests/toggles_tests.rs(1 hunks)src/toggles.rs(1 hunks)tests/end_to_end_tests.rs(1 hunks)tests/integration_tests.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
tests/integration_tests.rs (1)
src/protocol.rs (15)
create_reg1_packet(61-66)is_srtla_reg1(167-169)get_packet_type(42-47)create_reg2_packet(68-73)is_srtla_reg2(170-172)create_keepalive_packet(75-83)is_srtla_keepalive(176-178)extract_keepalive_timestamp(85-97)parse_srt_ack(109-117)is_srt_ack(179-181)parse_srt_nak(119-148)create_ack_packet(99-107)parse_srtla_ack(150-165)get_srt_sequence_number(49-59)is_srtla_reg3(173-175)
src/tests/protocol_tests.rs (1)
src/protocol.rs (14)
get_packet_type(42-47)get_srt_sequence_number(49-59)create_reg1_packet(61-66)is_srtla_reg1(167-169)create_reg2_packet(68-73)is_srtla_reg2(170-172)create_keepalive_packet(75-83)is_srtla_keepalive(176-178)extract_keepalive_timestamp(85-97)create_ack_packet(99-107)parse_srt_ack(109-117)parse_srt_nak(119-148)is_srtla_reg3(173-175)is_srt_ack(179-181)
src/tests/sender_tests.rs (2)
src/test_helpers.rs (1)
create_test_connections(48-89)src/sender.rs (6)
std(66-66)select_connection_idx(388-482)None(67-67)read_ip_list(484-498)apply_connection_changes(500-558)create_connections_from_ips(560-579)
src/tests/registration_tests.rs (2)
src/test_helpers.rs (1)
create_test_connection(11-45)src/protocol.rs (1)
create_reg2_packet(68-73)
src/tests/toggles_tests.rs (1)
src/toggles.rs (3)
from_cli(30-42)apply_cmd(70-131)spawn_toggle_listener(45-68)
src/tests/connection_tests.rs (3)
src/test_helpers.rs (1)
create_test_connection(11-45)src/connection.rs (8)
get_score(93-98)nak_burst_count(537-539)needs_keepalive(446-451)needs_rtt_measurement(417-422)should_attempt_reconnect(541-559)is_timed_out(517-519)total_nak_count(533-535)time_since_last_nak_ms(525-531)src/protocol.rs (3)
create_keepalive_packet(75-83)get_packet_type(42-47)extract_keepalive_timestamp(85-97)
tests/end_to_end_tests.rs (1)
src/protocol.rs (12)
create_reg1_packet(61-66)create_reg2_packet(68-73)is_srtla_reg3(173-175)create_keepalive_packet(75-83)extract_keepalive_timestamp(85-97)create_ack_packet(99-107)parse_srtla_ack(150-165)parse_srt_nak(119-148)get_packet_type(42-47)get_srt_sequence_number(49-59)is_srtla_keepalive(176-178)is_srt_ack(179-181)
src/registration.rs (1)
src/connection.rs (1)
now_ms(635-641)
src/connection.rs (1)
src/registration.rs (1)
now_ms(163-169)
src/sender.rs (1)
src/connection.rs (3)
total_nak_count(533-535)time_since_last_nak_ms(525-531)nak_burst_count(537-539)
🔇 Additional comments (45)
src/main.rs (1)
75-82: LGTM! Improved code formatting.The multi-line format enhances readability without changing the functionality.
src/toggles.rs (1)
70-70: LGTM! Appropriate visibility change for testing.Making
apply_cmdpublic enables comprehensive testing of the command parsing logic while maintaining the same functionality for internal usage.src/tests/mod.rs (1)
1-15: LGTM! Well-organized test module structure.The test modules are properly guarded with
#[cfg(test)]and provide comprehensive coverage for the different components of the system.src/lib.rs (4)
1-6: LGTM! Clear library documentation.The module-level documentation provides a clear description of the library's purpose and functionality.
7-11: Good module organization.The module declarations expose the core functionality as public modules, making them available for external use while maintaining clear separation of concerns.
13-17: Appropriate test module gating.Test-only modules are correctly gated behind
#[cfg(test)], ensuring they don't affect production builds.
19-23: Clean public API surface.The re-exports provide a convenient public API by exposing commonly used types at the crate root. Using
pub use protocol::*is appropriate here since protocol types are likely to be used together.src/tests/sender_tests.rs (15)
1-14: LGTM! Well-organized imports.The imports cover all necessary dependencies for comprehensive sender testing, including test helpers, external dependencies like tempfile, and appropriate async/time utilities.
15-27: Good classic mode selection test.The test correctly validates that classic mode selects the connection with the highest score (lowest
in_flight_packets). The use oftokio::runtime::Runtimefor async test helpers is appropriate.
29-41: LGTM! Stickiness behavior validation.This test properly verifies that stickiness prevents switching connections within the minimum interval, which is crucial for connection stability.
43-73: Excellent quality scoring test.The test thoroughly validates the quality scoring logic by:
- Setting up connections with different NAK patterns
- Using realistic time differences for NAK penalties
- Ensuring old switch time allows new selection
- Verifying the no-NAK bonus works correctly
75-101: Good NAK burst penalty validation.This test correctly verifies that NAK bursts receive additional penalties compared to individual NAKs, which should improve connection quality decisions.
103-120: Comprehensive IP list parsing test.The test validates all important scenarios: valid IPs, empty lines, and invalid entries. The handling of invalid IPs (logging warnings but continuing) is appropriate for robust operation.
122-136: Good edge case testing.Testing both empty files and non-existent files ensures robust error handling in the IP list reading functionality.
138-172: Thorough connection management test.This test validates critical connection management behavior:
- Removing stale connections
- Resetting selection state to prevent index issues
- Cleaning up sequence tracking maps
The verification of state changes is comprehensive.
174-185: Simple struct validation.Basic test to ensure the
PendingConnectionChangesstruct can be constructed and accessed correctly.
187-196: Good constant validation.This test ensures that timing and capacity constants are within reasonable ranges, which helps catch configuration errors early.
198-210: Appropriate failure tolerance test.The test correctly validates that connection creation failures don't cause panics, which is important for robustness when network conditions are poor.
212-232: Good sequence tracking limits test.This test validates the sequence tracking mechanism properly limits memory usage by removing old entries when capacity is exceeded.
234-248: Good edge case: all connections disconnected.Testing behavior when all connections are disconnected ensures the system handles network outages gracefully.
250-266: Appropriate exploration mode test.Since exploration mode is time-dependent, the test correctly focuses on ensuring no panics occur rather than testing specific outcomes.
268-283: Good integration test for toggles.This test validates that the DynamicToggles integration works correctly with the expected default values.
src/test_helpers.rs (3)
1-9: LGTM! Appropriate test-only module.The module is correctly gated behind
#[cfg(test)]and includes all necessary imports for creating test connections.
10-45: Well-designed single connection test helper.The
create_test_connectionfunction provides a comprehensive, consistently initialized test connection with:
- Proper async socket setup
- Realistic default values
- All fields properly initialized
This will be very useful for unit tests.
47-89: Excellent multi-connection test helper.The
create_test_connectionsfunction builds on the single connection pattern with appropriate variations:
- Unique remote ports per connection
- Distinct local IPs
- Unique labels for identification
The pattern of using a loop with variations is clean and maintainable.
tests/integration_tests.rs (1)
1-28: Solid protocol roundtrip coverageGood end-to-end assertions for REG1/REG2/KEEPALIVE and timestamp extraction.
src/tests/connection_tests.rs (1)
221-224: Confirm Instant type matches productionEnsure SrtlaConnection::last_received uses tokio::time::Instant; if it is std::time::Instant, this assignment will not compile.
If it is std::time::Instant, change to:
- use std::time::Duration; - conn.last_received = tokio::time::Instant::now() - Duration::from_secs(CONN_TIMEOUT + 1); + use std::time::{Duration, Instant}; + conn.last_received = Instant::now() - Duration::from_secs(CONN_TIMEOUT + 1);src/sender.rs (5)
20-23: LGTM! Good API exposure for testing.Making these constants public is appropriate for testing purposes and allows external consumers to understand the sender's behavior parameters.
484-498: Input validation improvements for read_ip_list.Good addition of line trimming and empty line handling. The function properly validates IPs and logs warnings for invalid entries.
500-536: Good sequence tracking cleanup in apply_connection_changes.The function properly handles the cleanup of sequence tracking when connections are removed, preventing stale indices from causing issues. The rebuilding of the HashMap ensures consistency.
560-579: Improved error message formatting.The multi-line error message format provides better visibility for connection failures, making debugging easier.
133-133: Drop synchronization suggestion forpending_changes: it’s a local variable within the singlerun_sender_with_togglesfuture and never shared across tasks—Rust’s ownership model and Tokio’s executor guarantee exclusive, thread-safe access.Likely an incorrect or invalid review comment.
.cargo/config.toml (1)
1-14: LGTM! Excellent testing configuration.This Cargo configuration provides a comprehensive testing environment with strict warnings-as-errors for CI, full backtraces for debugging, verbose logging, and debugging-optimized test builds. The configuration will significantly improve the development and debugging experience for the extensive test suite being added.
Cargo.toml (2)
32-38: LGTM! Proper dual-target configuration.The library and binary target configuration is correctly structured, allowing the crate to be both imported as a library and executed as a binary. This aligns well with the PR's goal of expanding the public API surface while maintaining the existing CLI functionality.
40-43: LGTM! Appropriate test dependencies for comprehensive testing.The development dependencies are well-chosen for the test suite:
tokio-testfor async testing utilitiestempfilefor temporary file/socket creation in testsassert_matchesfor pattern matching assertionsThese align with the testing needs demonstrated in the test files.
src/tests/toggles_tests.rs (10)
7-15: LGTM! Comprehensive default value testing.The test correctly verifies the default initialization state of all toggles, matching the expected behavior from the
DynamicToggles::new()implementation.
17-32: LGTM! Thorough CLI configuration testing.The test properly validates both the default case and various CLI parameter combinations, correctly handling the negation logic for
no_stickinessandno_qualityparameters.
34-50: LGTM! Complete command format coverage.The test validates both command formats (
classic on/offandclassic=true/false) ensuring the toggle responds correctly to all supported input variations.
52-68: LGTM! Consistent testing pattern across all toggles.All toggle tests follow the same comprehensive pattern, testing both command formats and state transitions for stickiness, quality, and exploration toggles. The coverage is thorough and consistent.
Also applies to: 70-86, 88-104
106-131: LGTM! Proper status command validation.The test ensures the status command is read-only and doesn't modify any toggle states, which is the expected behavior for a status query.
133-150: LGTM! Robust error handling verification.The test confirms that empty commands and unknown commands are handled gracefully without panics or state changes, demonstrating good defensive programming in the implementation.
152-162: LGTM! Whitespace handling verification.The test ensures commands work correctly even with extra whitespace, demonstrating the robustness of the command parsing logic.
164-187: LGTM! Excellent concurrency testing.The test validates thread safety by having one thread modify toggle values while another reads them concurrently. This is crucial for a system that may receive commands from multiple sources.
189-198: LGTM! Listener spawning tests with proper platform gating.The tests verify that toggle listeners can be spawned both with and without Unix socket paths. The Unix-specific test is properly gated with
#[cfg(unix)]and usestempfileappropriately for temporary socket path creation.Also applies to: 200-217
219-287: LGTM! Comprehensive format validation with excellent test design.This test provides thorough coverage of all supported command formats by:
- Testing all command variations systematically
- Resetting to a known state before each test
- Capturing initial state and verifying changes occur
- Testing both on and off commands for completeness
The tuple-based state comparison is an elegant way to detect changes across all toggles.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
README.md (2)
39-39: Grammar: “arm” → “feature/behavior”.“Windows runs without that arm” is unclear. Suggest “Windows runs without that feature” or “without SIGHUP-based reload.”
Apply this diff:
- - Note: SIGHUP-based IP reload is Unix-only; Windows runs without that arm + - Note: SIGHUP-based IP reload is Unix-only; Windows runs without that feature
206-209: Grammar: plural verb agreement.“quality-based scoring and connection stickiness is enabled” → “are enabled.”
-These affect selection behavior in real time. By default, quality-based scoring and connection stickiness is enabled. +These affect selection behavior in real time. By default, quality-based scoring and connection stickiness are enabled.
🧹 Nitpick comments (12)
README.md (6)
3-4: Badges added: consider adding matching documentation for Debian packages.You advertise Debian packaging via a badge but there’s no section explaining how to build/use those packages. Add a short “Debian packages” section linking to the workflow artifacts or instructions.
37-38: Document MSRV.State the minimum supported Rust version (MSRV) here and enforce it in CI to prevent accidental bumps.
49-64: Testing section looks good.Clear commands and examples. Consider adding an example for running a single integration test or a test by path if useful.
66-74: Punctuate CI/CD bullet list for readability.Add periods to each bullet for consistency with surrounding prose.
Proposed edit:
- - Multi-platform testing (Linux, Windows, macOS) - - Code formatting and linting checks - - Security vulnerability scanning - - Build verification across multiple Rust versions + - Multi-platform testing (Linux, Windows, macOS). + - Code formatting and linting checks. + - Security vulnerability scanning. + - Build verification across multiple Rust versions.
165-172: Note external dependency for examples.Since examples use socat, add a one-liner install hint (e.g., apt/brew) so newcomers don’t get stuck.
Suggested snippet:
# Send commands remotely echo 'classic on' | socat - UNIX-CONNECT:/tmp/srtla.sock echo 'status' | socat - UNIX-CONNECT:/tmp/srtla.sock +# +## Note: Requires `socat` (e.g., `sudo apt-get install socat` or `brew install socat`).
243-245: Clarify the NAK burst penalty multiplier.“additional 0.5x penalty multiplier” is ambiguous (halve score or +50%?). Specify exact formula (e.g., score *= 0.5 or score *= 1.5) to avoid misinterpretation.
Example edit:
-... receive an additional 0.5x penalty multiplier to their quality score ... +... have their quality score multiplied by 0.5 (i.e., halved) when a recent burst is detected ....github/workflows/ci.yml (6)
42-53: Deduplicate test invocations.You’re running overlapping test suites three times. One all-features run usually suffices.
- - name: Run unit tests - run: cargo test --lib --verbose - - - name: Run integration tests - run: cargo test --test '*' --verbose - - - name: Run all tests - run: cargo test --verbose - - - name: Run tests with all features - run: cargo test --all-features --verbose + - name: Run all tests (all features) + run: cargo test --all-features --verbose
54-59: Harden cargo-audit installation.Use --locked to respect Cargo.lock and pin the tool’s version if reproducibility matters.
- - name: Install cargo-audit - run: cargo install cargo-audit + - name: Install cargo-audit + run: cargo install --locked cargo-audit
14-21: Security hardening: pin Actions to commit SHAs.actions/checkout@v4 and actions-rust-lang/setup-rust-toolchain@v1 (and actions/cache@v4) should be pinned to specific commit SHAs per GitHub’s security guidance.
22-32: Improve cache key robustness.Include the toolchain channel in the cache key to avoid cross-channel cache issues.
Example:
- key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }} + key: ${{ runner.os }}-cargo-${{ steps.rust-toolchain.outputs.cachekey || 'stable' }}-${{ hashFiles('**/Cargo.lock') }} + # Alternatively: add a step to capture `rustc -V` and append it to the key.Note: implement an output from the toolchain step or a preceding step to populate the channel.
60-114: Optional: reduce duplication with a matrix.test, test-stable, and test-beta can be a single matrix job over channels; likewise for OSes, unless you prefer explicit jobs for readability.
114-114: Add trailing newline.Fix “no newline at end of file” warning.
- run: cargo test --verbose + run: cargo test --verbose +
📜 Review details
Configuration used: CodeRabbit UI
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 (2)
.github/workflows/ci.yml(1 hunks)README.md(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 114-114: no new line character at the end of file
(new-line-at-end-of-file)
🪛 LanguageTool
README.md
[grammar] ~70-~70: There might be a mistake here.
Context: ...platform testing (Linux, Windows, macOS) - Code formatting and linting checks - Sec...
(QB_NEW_EN)
[grammar] ~71-~71: There might be a mistake here.
Context: ...OS) - Code formatting and linting checks - Security vulnerability scanning - Build ...
(QB_NEW_EN)
[grammar] ~72-~72: There might be a mistake here.
Context: ...checks - Security vulnerability scanning - Build verification across multiple Rust ...
(QB_NEW_EN)
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
33-41: Pre-checks are solid.fmt, clippy (deny warnings), and cargo check before tests are well-ordered.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
.github/workflows/ci.yml (10)
22-32: Use purpose-built Rust cache to avoid stale/poisoned targets and speed up CI.Manual caching of target/registry can cause incompatibilities across rustc/toolchains. Swatinem/rust-cache handles keys per toolchain/target/features automatically.
Apply this diff:
- - name: Cache dependencies - uses: actions/cache@v4 - with: - path: | - ~/.cargo/registry - ~/.cargo/git - target - key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }} - restore-keys: | - ${{ runner.os }}-cargo- + - name: Cache cargo build artifacts + uses: Swatinem/rust-cache@v2 + with: + cache-on-failure: true
36-41: Lint and check the entire workspace, all targets, all features.Clippy/check currently miss benches/examples/tests/feature combos.
Apply this diff:
- - name: Run clippy - run: cargo clippy -- -D warnings + - name: Run clippy (workspace, all targets/features) + run: cargo clippy --workspace --all-targets --all-features -- -D warnings - - - name: Check compilation - run: cargo check + - name: Check compilation (workspace, all targets) + run: cargo check --workspace --all-targets
42-53: Collapse redundant test runs.You’re running unit, integration, and “all tests” back-to-back; the latter already includes the former. Keep two passes (default and all-features).
Apply this diff:
- - name: Run unit tests - run: cargo test --lib --verbose - - - name: Run integration tests - run: cargo test --test '*' --verbose - - - name: Run all tests - run: cargo test --verbose - - - name: Run tests with all features - run: cargo test --all-features --verbose + - name: Run tests (default features) + run: cargo test --workspace --verbose + + - name: Run tests (all features) + run: cargo test --workspace --all-features --verbose
54-59: Pin cargo-audit install; consider action-based audit.Unpinned installs can break; use --locked at minimum. Alternatively, use rustsec/audit-check.
Minimal hardening:
- - name: Install cargo-audit - run: cargo install cargo-audit + - name: Install cargo-audit + run: cargo install --locked cargo-auditOr replace both steps with:
- - name: Install cargo-audit - run: cargo install cargo-audit - - - name: Run security audit - run: cargo audit + - name: Security audit + uses: rustsec/audit-check@v2 + with: + token: ${{ secrets.GITHUB_TOKEN }}
1-8: Harden workflow: restrict token permissions and cancel superseded runs.Improves supply-chain posture and saves CI minutes.
Apply this diff:
name: CI on: push: branches: [ main, master ] pull_request: branches: [ main, master ] + +permissions: + contents: read + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true
60-89: Avoid duplication betweentestandtest-stable.The
testjob already runs on stable (no explicit channel). Either droptest-stableor maketestexplicitly stable and repurpose this job for MSRV or nightly.
90-114: Parity across OSes: add caching and clippy.Windows/macOS jobs don’t cache builds or run clippy, slowing feedback and letting lints slip by OS-specific cfgs. Consider adding
Swatinem/rust-cache@v2and a clippy step mirroring Linux.
97-101: Optional: add job timeouts.Prevents hanged runs from burning minutes.
Example patch for each job block:
test-windows: name: Test (Windows) runs-on: windows-latest + timeout-minutes: 30Also applies to: 110-114
15-16: Pin actions to commit SHAs.Best practice for supply-chain security; major tags can be retagged.
Example:
- uses: actions/checkout@v4 + uses: actions/checkout@3df4ab11eba7bda6032a0b82a6bb43b11571feac # v4.x(Repeat for other actions.)
Also applies to: 18-19, 23-24, 65-66, 68-69, 80-81, 83-84, 95-96, 98-99, 108-109, 111-112
114-114: Add a trailing newline.Satisfies linters and POSIX text-file convention.
Apply this (ensure file ends with a newline character).
- run: cargo test --verbose + run: cargo test --verbose +
📜 Review details
Configuration used: CodeRabbit UI
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 (1)
.github/workflows/ci.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 114-114: no new line character at the end of file
(new-line-at-end-of-file)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
34-36: Prefer running rustfmt on stable or drop+nightly(nightly is already installed).You install the nightly toolchain above, so this won’t fail, but formatting on stable avoids nightly churn and simplifies the step. Either remove
+nightlyor switch the toolchain for fmt to stable.- - name: Check formatting - run: cargo +nightly fmt --all -- --check + - name: Check formatting + run: cargo fmt --all -- --check
🧹 Nitpick comments (5)
.github/workflows/ci.yml (5)
23-33: Harden cache key: include rustc version to avoid stale/invalid cache across toolchains.Nightly changes frequently; tying the cache to the compiler version reduces cache poisoning and needless rebuilds.
+ - name: Capture rustc version + id: rustc_version + run: echo "version=$(rustc -V)" >> $GITHUB_OUTPUT + - name: Cache dependencies uses: actions/cache@v4 with: path: | ~/.cargo/registry ~/.cargo/git target - key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }} + key: ${{ runner.os }}-cargo-${{ steps.rustc_version.outputs.version }}-${{ hashFiles('**/Cargo.lock') }} restore-keys: | ${{ runner.os }}-cargo-
37-39: Expand clippy coverage to all targets/features.Catches issues in benches, examples, tests, and feature combos.
- - name: Run clippy - run: cargo clippy -- -D warnings + - name: Run clippy + run: cargo clippy --workspace --all-targets --all-features -- -D warnings
43-54: Avoid redundant test runs to speed up CI.Running unit, integration, then “all tests” duplicates work. Keep “all tests” and “all-features” only (or keep unit/integration if you need separation).
- - name: Run unit tests - run: cargo test --lib --verbose - - - name: Run integration tests - run: cargo test --tests --verbose - - name: Run all tests run: cargo test --verbose - name: Run tests with all features run: cargo test --all-features --verbose
55-57: Makecargo-auditinstall reproducible.Pin via
--lockedto respect Cargo.lock of the crate and avoid supply-chain drift.- - name: Install cargo-audit - run: cargo install cargo-audit + - name: Install cargo-audit + run: cargo install cargo-audit --locked
115-115: Add trailing newline.YAML lint warns: no newline at EOF. Add a final newline to satisfy linters.
📜 Review details
Configuration used: CodeRabbit UI
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 (1)
.github/workflows/ci.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 115-115: no new line character at the end of file
(new-line-at-end-of-file)
⏰ 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: Test (Windows)
🔇 Additional comments (1)
.github/workflows/ci.yml (1)
46-48: Fix:cargo test --test '*'is invalid; use--tests.
--testexpects an exact target name; the literal'*'won’t match and will fail. Use--teststo run all integration tests.Apply:
- - name: Run integration tests - run: cargo test --test '*' --verbose + - name: Run integration tests + run: cargo test --tests --verboseLikely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Cargo.toml (1)
4-4: Fix the invalid Rust edition.The edition "2024" is not a valid Rust edition. Valid editions are "2015", "2018", and "2021".
Apply this fix:
-edition = "2024" +edition = "2021"
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
34-36: Re: fmt with nightly — previous concern now moot.You install nightly with rustfmt earlier, so
cargo +nightly fmtis fine. If you want to decouple from nightly, drop+nightlyas previously suggested; otherwise leave as-is.
🧹 Nitpick comments (15)
.github/workflows/ci.yml (5)
37-38: Broaden clippy coverage to workspace/all targets.Catches lints in benches, examples, tests, and across members.
- - name: Run clippy - run: cargo clippy -- -D warnings + - name: Run clippy (workspace, all targets) + run: cargo clippy --workspace --all-targets --all-features -- -D warnings
23-33: Simplify and harden Cargo caching.Use Swatinem/rust-cache for correct keying (toolchain-aware) and less YAML.
- - name: Cache dependencies - uses: actions/cache@v4 - with: - path: | - ~/.cargo/registry - ~/.cargo/git - target - key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }} - restore-keys: | - ${{ runner.os }}-cargo- + - uses: Swatinem/rust-cache@v2 + with: + shared-key: ${{ runner.os }}-cargo
61-66: Install cargo-audit without compiling every run.Use a prebuilt installer (faster, reproducible), or at least add --locked.
Preferred:
- - name: Install cargo-audit - run: cargo install cargo-audit + - name: Install cargo-audit + uses: taiki-e/install-action@v2 + with: + tool: cargo-auditAlternative:
- run: cargo install cargo-audit + run: cargo install cargo-audit --locked
110-121: Parity: run macOS tests with test-internals too (like Windows).Keeps platform coverage consistent.
- name: Run tests - run: cargo test --verbose + run: cargo test --features test-internals --verbose
120-121: Add trailing newline to fix YAMLlint error.Currently fails “no new line at end of file”.
- run: cargo test --verbose + run: cargo test --verbose +Cargo.toml (1)
32-34: Consider documenting thetest-internalsfeature.The feature flag is well-structured for exposing internal APIs during testing. Consider adding a comment to explain its purpose for future maintainers.
[features] +# Exposes internal APIs for integration and end-to-end testing test-internals = []src/tests/registration_tests.rs (2)
36-37: Test may be flaky due to timing assumptions.The assertion
assert!(reg.reg1_next_send_at_ms() <= current_time + 100)could fail under high system load when there's significant delay betweennow_ms()calls. Consider using a more robust timing check or mocking time.let current_time = now_ms(); -assert!(reg.reg1_next_send_at_ms() <= current_time + 100); // Allow CI scheduler skew +// Verify that next send time is set (non-zero) rather than checking exact timing +assert!(reg.reg1_next_send_at_ms() > 0); +assert!(reg.reg1_next_send_at_ms() <= current_time + 1000); // More generous margin
240-246: Consider making the uniqueness test more deterministic.The test for ID uniqueness could theoretically fail if the random number generator produces identical values. While extremely unlikely with a proper RNG, consider using a set to check for duplicates more explicitly.
#[test] fn test_id_generation_uniqueness() { - let ids: Vec<_> = (0..8) + use std::collections::HashSet; + let ids: HashSet<_> = (0..8) .map(|_| SrtlaRegistrationManager::new().srtla_id) .collect(); - let all_same = ids.windows(2).all(|w| w[0] == w[1]); - assert!(!all_same, "All generated IDs were identical unexpectedly"); + assert_eq!(ids.len(), 8, "Generated IDs should be unique"); }src/tests/connection_tests.rs (1)
269-270: Potential timing issue in keepalive timestamp validation.The assertion
assert!((now.saturating_sub(timestamp)) < 1000)could fail if there's a delay between packet creation and extraction. Consider using a more generous margin.// Timestamp should be recent (within last second) let now = now_ms(); -assert!((now.saturating_sub(timestamp)) < 1000); +assert!((now.saturating_sub(timestamp)) < 5000, "Timestamp should be within 5 seconds");src/sender.rs (5)
133-133: Redundant empty lines in async blocks.These empty lines don't add value and could be removed for cleaner code.
- let classic = toggles.classic_mode.load(std::sync::atomic::Ordering::Relaxed);Also applies to: 166-166
285-287: Improve multi-line comment formatting.The comment formatting could be more readable with proper line breaks.
- // Process SRTLA ACKs: first find specific packet, then apply global +1 to all - // connections This matches the original implementation's - // register_srtla_ack behavior + // Process SRTLA ACKs: first find specific packet, then apply global +1 to all + // connections. This matches the original implementation's register_srtla_ack behavior
460-476: Consider reducing debug log verbosity.The quality penalty logging could become noisy in production environments with frequent NAKs.
Consider using trace level or adding a throttling mechanism:
- // Log quality analysis for debugging (only for low scores to avoid spam) - if quality_mult < 1.0 { - debug!( + // Log quality analysis for debugging (only for low scores to avoid spam) + if quality_mult < 1.0 { + trace!(
522-522: Remove redundant empty lines.Several empty lines don't improve readability and could be removed.
let current_labels: HashSet<String> = connections.iter().map(|c| c.label.clone()).collect(); - let desired_labels: HashSet<String> = new_ipsAlso applies to: 538-538, 541-541, 553-555, 561-561, 567-567
586-589: Improve multi-line error message formatting.The warning message formatting could be cleaner.
- Err(e) => warn!( - "failed to add uplink {} -> {}:{}: {}", - ip, receiver_host, receiver_port, e - ), + Err(e) => warn!("failed to add uplink {} -> {}:{}: {}", ip, receiver_host, receiver_port, e),src/connection.rs (1)
443-445: Improve multi-line comment formatting consistency.The comment formatting should be consistent throughout the file.
- // This mimics the first phase of the original implementation's - // register_srtla_ack + // This mimics the first phase of the original implementation's register_srtla_ack- // Global +1 window increase for active connections (from original - // implementation) This is the second phase applied to ALL connections - // for each SRTLA ACK + // Global +1 window increase for active connections (from original implementation). + // This is the second phase applied to ALL connections for each SRTLA ACK.Also applies to: 456-456, 477-479
📜 Review details
Configuration used: CodeRabbit UI
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 (17)
.github/workflows/ci.yml(1 hunks)Cargo.toml(1 hunks)build.rs(1 hunks)src/connection.rs(6 hunks)src/lib.rs(1 hunks)src/main.rs(2 hunks)src/registration.rs(2 hunks)src/sender.rs(12 hunks)src/test_helpers.rs(1 hunks)src/tests/connection_tests.rs(1 hunks)src/tests/protocol_tests.rs(1 hunks)src/tests/registration_tests.rs(1 hunks)src/tests/sender_tests.rs(1 hunks)src/tests/toggles_tests.rs(1 hunks)src/utils.rs(1 hunks)tests/end_to_end_tests.rs(1 hunks)tests/integration_tests.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- build.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- src/main.rs
- tests/end_to_end_tests.rs
- src/test_helpers.rs
- src/tests/protocol_tests.rs
- src/tests/sender_tests.rs
- tests/integration_tests.rs
- src/tests/toggles_tests.rs
- src/registration.rs
🧰 Additional context used
🧬 Code graph analysis (5)
src/lib.rs (1)
src/utils.rs (1)
now_ms(6-11)
src/tests/connection_tests.rs (4)
src/test_helpers.rs (1)
create_test_connection(12-46)src/utils.rs (1)
now_ms(6-11)src/connection.rs (8)
get_score(169-174)nak_burst_count(615-617)needs_keepalive(524-529)needs_rtt_measurement(495-500)should_attempt_reconnect(619-637)is_timed_out(595-597)total_nak_count(611-613)time_since_last_nak_ms(603-609)src/protocol.rs (3)
create_keepalive_packet(75-83)get_packet_type(42-47)extract_keepalive_timestamp(85-97)
src/connection.rs (1)
src/utils.rs (1)
now_ms(6-11)
src/tests/registration_tests.rs (4)
src/test_helpers.rs (1)
create_test_connection(12-46)src/utils.rs (1)
now_ms(6-11)src/registration.rs (9)
new(22-35)active_connections(180-182)has_connected(184-186)broadcast_reg2_pending(188-190)pending_reg2_idx(172-174)reg1_target_idx(192-194)srtla_id(168-170)reg1_next_send_at_ms(196-198)pending_timeout_at_ms(176-178)src/protocol.rs (1)
create_reg2_packet(68-73)
src/sender.rs (1)
src/connection.rs (3)
total_nak_count(611-613)time_since_last_nak_ms(603-609)nak_burst_count(615-617)
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 121-121: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (13)
src/lib.rs (1)
1-26: LGTM! Well-structured library module organization.The module structure is clean and the re-exports provide a convenient public API. The separation of test helpers behind
#[cfg(test)]is appropriate.src/tests/connection_tests.rs (2)
7-25: LGTM! Comprehensive connection score testing.The test thoroughly covers scoring scenarios including initial state, in-flight adjustments, and disconnected state.
298-313: LGTM! Excellent edge case testing for packet log wraparound.The test properly validates the circular buffer behavior and ensures packets can still be acknowledged after wraparound.
src/sender.rs (7)
20-22: LGTM! Constants are properly exposed for test access.These constants are being used in tests and the public exposure is appropriate for the test suite functionality.
24-28: LGTM! Public API structure for connection changes.The
PendingConnectionChangesstruct provides a clean interface for managing connection updates with proper field visibility.
389-416: LGTM! Quality multiplier calculation is well-implemented.The function provides good scoring logic with appropriate penalties for NAK history and bursts. The multiplier range (0.05-1.2) provides sensible bounds and the bonus for zero NAKs encourages good connections.
418-495: LGTM! Enhanced connection selection with comprehensive scoring.The function properly handles both classic and enhanced modes with good separation of concerns. The quality scoring integration and exploration logic are well-implemented.
497-511: LGTM! IP list parsing with proper validation.The function correctly handles empty lines, trims whitespace, and provides appropriate warnings for invalid entries.
513-572: LGTM! Connection management with proper state handling.The function correctly manages connection lifecycle, preserves sequence tracking integrity, and handles index updates properly when connections are removed.
574-593: LGTM! Connection creation with proper error handling.The function provides good error messaging and handles connection failures gracefully.
src/connection.rs (3)
12-12: LGTM! Proper utility function import.The centralization of
now_msin the utils module is a good architectural decision, and the import is correctly placed.
23-128: Well-designed feature-gated field visibility.The use of
cfg(feature = "test-internals")provides controlled access to internal fields for testing while maintaining encapsulation in production builds. This addresses the previous concern about breaking encapsulation.
296-296: LGTM! Method properly exposed for testing.Making
register_packetpublic enables test access while maintaining the existing functionality.
Summary by CodeRabbit
New Features
Tests
Chores
Documentation