Repository navigation
Fix flaky test_ash_end_to_end - #760
Merged
Merged
Conversation
The test scales the ASH ACK timeouts down to milliseconds and used wall-clock sleeps to decide how long a link stays down, so a CPU stall on a busy CI runner could flip its outcome: - The "host failure" and "ncp failure" steps unpaused the transport after a fixed sleep. If the retries had not all timed out yet, the next one got through and the expected TimeoutError never came. The link now stays down until the send has failed. - After sending RST, a fixed 10 ms sleep did not guarantee the RSTACK had been processed. Wait for the host to be connected again instead. - The excessive-NAK step raced each NAK against a sub-millisecond ACK timeout; if the final attempt timed out first, TimeoutError was raised instead of NotAcked. Give the host a 5 s ACK timeout and disable random frame loss for that step, so every attempt ends in a NAK.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #760 +/- ##
=======================================
Coverage 99.58% 99.58%
=======================================
Files 64 64
Lines 4330 4330
=======================================
Hits 4312 4312
Misses 18 18 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
test_ash_end_to_endfails intermittently on CI, most recently ondevin https://github.com/zigpy/bellows/actions/runs/36362650409 (Python 3.14,FakeTransportWithDelays:TimeoutErrorinstead ofNotAcked), and earlier in https://github.com/zigpy/bellows/actions/runs/34772581160 and https://github.com/zigpy/bellows/actions/runs/30421043625 (DID NOT RAISE TimeoutErrorat the "ncp failure" step, fixed by a rerun).The test scales the ASH ACK timeouts down to 0.4–3.2 ms and used wall-clock sleeps to decide how long a link stays down, so a short CPU stall on a busy runner changes the outcome. This is a test-only change;
bellows/ash.pyis untouched.sleep(t_rx_ack * 15). If the last of the 5 attempts had not been sent yet by then, it got through and noTimeoutErrorwas raised. The link now stays paused until the send has actually failed.sleep(0.01)after sending RST did not guarantee the RSTACK had been processed, so the next send could hitNcpFailure. The test now waits (bounded to 1 s) until the host is connected again.TimeoutErrorcame out instead ofNotAcked(the failure in the linked run). The host now gets a 5 s ACK timeout for this step, andFakeTransportRandomLossdrops nothing during it, so every attempt ends in a NAK. The NAK retry and final-NAK paths in_send_data_frameare still covered (ash.pycoverage unchanged).Stress results with
pytest-repeat+pytest-xdiston an 8-core machine,test_ash_end_to_endonly:dev-n 8, 6000 runs (CI-like)-n 24, 1600 runs (3× oversubscribed)Under even heavier load (
-n 32, 4000 runs) this PR still failed 2 runs. Both were sends that are expected to succeed but ran out of their ~14 ms retry budget. Fixing that would mean raising the patchedT_RX_ACK_MAX, which made each parametrization about 5× slower (≈0.3–0.45 s), so it is left out.