[DeepTest] Add unit tests for BBR congestion control#5887
[DeepTest] Add unit tests for BBR congestion control#5887
Conversation
saikat107
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5887 +/- ##
==========================================
- Coverage 86.23% 85.85% -0.38%
==========================================
Files 60 60
Lines 18732 18732
==========================================
- Hits 16153 16082 -71
- Misses 2579 2650 +71 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // Scenario: Sends 5000 bytes via OnDataSent, then triggers a loss event of 1200 bytes | ||
| // with LargestPacketNumberLost=5 and LargestSentPacketNumber=10. BBR should enter | ||
| // CONSERVATIVE recovery on the first loss event. Asserts that RecoveryState == | ||
| // CONSERVATIVE, EndOfRecoveryValid is TRUE, and BytesInFlight decrements from |
There was a problem hiding this comment.
nit: The comment about the assert is rather redundant, it describes exactly what the code does.
I find the sentence before describing what the test does and expect at a high level useful, but the last sentence doesn't add value.
Same for the other tests.
| Settings.InitialWindowPackets = WindowPackets; | ||
| InitBbrMockConnection(Connection, Mtu); | ||
| Connection.Settings.PacingEnabled = PacingEnabled ? TRUE : FALSE; | ||
| Connection.Settings.NetStatsEventEnabled = NetStatsEnabled ? TRUE : FALSE; |
There was a problem hiding this comment.
When NetStatsEventEnabled is true, it would be interesting to set a dummy callback and to potentially use it to validate the notification is emitted when expected.
| // | ||
| // Test: GetSendAllowance - MinRtt at UINT64_MAX With Pacing | ||
| // Scenario: Initializes with PacingEnabled=TRUE. MinRtt starts at UINT64_MAX (no RTT | ||
| // sample yet). Due to BUG-1 (MinRtt compared to UINT32_MAX not UINT64_MAX), the pacing |
There was a problem hiding this comment.
What is BUG-1? It sounds like an issue we should potentially fix, but not mention in test comments (and the test should be adapted if a fix is needed of course)
| // Helper: pump a bandwidth sample into the BBR filter via OnDataAcknowledged. | ||
| // Creates a packet metadata with HasLastAckedPacketInfo so that | ||
| // BbrBandwidthFilterOnPacketAcked computes a delivery rate. | ||
| // Returns the TimeNow used so callers can build on it. | ||
| // | ||
| // | ||
| // Pump a bandwidth sample into the BBR filter via OnDataAcknowledged. | ||
| // Constructs packet metadata so that BbrBandwidthFilterOnPacketAcked | ||
| // computes DeliveryRate = BW_UNIT * SendRate_BytesPerSec. |
There was a problem hiding this comment.
This comment says the same thing twice.
| // near zero after the helper, the DRAIN to PROBE_BW transition fires immediately. | ||
| // Asserts BtlbwFound == TRUE and BbrState == PROBE_BW. | ||
| // | ||
| TEST_F(BbrTest, OnDataAcknowledged_StartupToDrain) |
There was a problem hiding this comment.
This test is a duplicate of the next one.
Assuming the comment is correct and the transition to PROBE_BW is due to a low number of bytes in flight, we should set it up to pretend that more bytes are in flight.
2 participants
Summary
Adds 84 GoogleTest unit tests for
src/core/bbr.c, covering all 33 BBR functions. Tests exercise the real BBR implementation with hand-crafted input structs — no mocks or state mutation.Changes
src/core/unittest/BbrTest.cppsrc/core/unittest/CMakeLists.txtBbrTest.cppto build.gitignore.deeptest/directoryTest Organization
Following the
CubicTest.cppconvention, tests are grouped into three sections:Test Infrastructure
BbrTestfixture withInitializeWithDefaults()— allocates a realQUIC_CONNECTIONon-stackMakeBbrPacket/MakeBbrAckEvent/MakeBbrLossEvent— factory helpers for input structsPumpBandwidthSample— injects a bandwidth sample with controlled delivery rateDriveToBtlbwFound/EnterRecovery— multi-step helpers for reaching specific BBR statesBBR_STATE,RECOVERY_STATE) mirrored frombbr.cfor readable assertionsAssertion Policy
ASSERT_EQwith exact expected valuesASSERT_TRUE/ASSERT_FALSEASSERT_LE,ASSERT_GT, etc.)Coverage (measured via OpenCppCoverage)
PROBE_BWpacing gain cycle logic that depends on a randomPacingCycleIndex(set viaCxPlatRandom), making deterministic testing infeasible without mocking the RNG