test(review): stop RescuedFindingLostWriteTest sleeping through the content-creation floor - #752
Merged
Conversation
…loor The fixture threw a content-creation block with `Retry-After: 0` and documented that as "so the test does not sleep". #738 then made the 30-second content-creation floor apply however the delay was derived, an explicit `Retry-After` included — so every attempt of every exhausted route now waited the floor, and the premise the fixture was built on stopped holding in the same release that shipped it. What these tests need from the throttle is that `isThrottled` says yes and that the route burns `GitHubWriteRetry.MAX_ATTEMPTS`; a plain secondary rate limit does both, and no assertion here reads the block wording. Swapping the body keeps all three tests and their assertions exactly as they are while dropping the class from 461.05 s to 25.97 s, and the single-test case from 94.01 s to 6.95 s. A full `clean test` goes from 12:22 to 04:40. Fixes #749
Contributor
🤖 ThrillhouseBot PR SummaryWhat this PR doesThis PR changes the RescuedFindingLostWriteTest fixture from a GitHub content-creation block message to a plain secondary-rate-limit message, renaming BLOCK_BODY to THROTTLE_BODY and rewriting the surrounding comments to explain that the content-creation floor introduced in #738 no longer applies. All three tests and their assertions are unchanged; the goal is to keep the retry/accounting seam exercised while cutting the test class wall-clock time from ~461s to ~26s. Description vs. ImplementationNo mismatch found between the PR description and the change. Changes Overview
Changed Files
Risk Assessment
Everything's coming up Thrillhouse! 🎉 No issues found in this PR. Automated review by ThrillhouseBot. Reply with |
![thrillhousebot[bot]](https://avatars.githubusercontent.com/in/3996985?s=60&v=4){kind=small}
thrillhousebot
Bot
added
performance
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
1 participant
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.
What type of PR is this?
Description
RescuedFindingLostWriteTest's fixture threw GitHub's content-creation block withRetry-After: 0and documented that as "naming a deadline of 'now' so the test does not sleep". That was true when it was written. #738 then madeCONTENT_CREATION_BLOCK_MIN_DELAY(30 s) apply however the delay was derived, an explicitRetry-Afterincluded — and shipped in the same release. Every attempt of every exhausted route has waited the floor ever since: 3 sleeps per route, 6 exhausted routes across the class.Neither change is wrong. The fixture simply encodes an assumption that stopped being true.
What these tests actually need from the throttle is that
GitHubApiError.isThrottledsays yes and that the route burnsGitHubWriteRetry.MAX_ATTEMPTS. A plain secondary rate limit does both, and nothing in the class asserts on the block wording — the assertions are all about accounting. SoBLOCK_BODYbecomesTHROTTLE_BODY, a plain secondary-limit message, with a comment on why it is deliberately not a content-creation block so the next reader does not "restore" it. All three tests and every assertion in them are unchanged.Related Issues
Fixes #749
Proof: audit report AUDIT7-A, finding A3.
How Has This Been Tested?
This is a build-cost fix with no behavioural change, so there is no red/green proof to quote — the three tests pass before and after, on the same assertions, and that is the point. The evidence is the measured wall clock, before and after, on the same machine and JVM.
Before (
surefireXML, this branch's parent)After
461.05 s → 25.97 s for the class (−435 s, 17.7x), 94.01 s → 6.95 s for the single test. The 94 s figure was 3 x 30 s of floor plus overhead, to the second; what remains is the retry pacing the seam genuinely exercises.
A whole
clean teston this branch drops from 12:22 to 04:40.Gates
spotless:apply→clean compile spotbugs:check spotless:check: BugInstance size is 0, BUILD SUCCESSclean test: Tests run: 3308, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESSChecklist