Skip to content

Parameterize recovery module tests so they run against all congestion control algorithms. #2012

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Apr 28, 2025

Conversation

antoniovicente
Copy link
Contributor

As a pre-requisite, add Instant::now as an argument to get_packet_send_time so it can be driven by the test.

@antoniovicente antoniovicente requested a review from a team as a code owner April 10, 2025 04:00
@antoniovicente antoniovicente self-assigned this Apr 10, 2025
@@ -1301,7 +1324,7 @@ mod tests {
is_app_limited: false,
tx_in_flight: 0,
lost: 0,
has_data: false,
has_data: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bbr2_gcongestion ignores sends with has_data set to false.

@antoniovicente antoniovicente force-pushed the test/recovery_v2 branch 2 times, most recently from 13bb0de to c686262 Compare April 10, 2025 04:06
assert_eq!(
r.get_packet_send_time(now) - now,
Duration::from_secs_f64(12000.0 / pacing_rate as f64)
Duration::from_secs_f64(scale_factor * 12000.0 / pacing_rate as f64)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pacing rate for bbr2_gcongestion may be too conservative; it is sending 2.5x slower than all the other congestion control algorithms

@ghedo ghedo merged commit 0771d97 into master Apr 28, 2025
38 checks passed
@ghedo ghedo deleted the test/recovery_v2 branch April 28, 2025 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants