Allow small number of buffer exhaustion recover failures. - #2302
Allow small number of buffer exhaustion recover failures.#2302davidmorgan wants to merge 1 commit into
Conversation
Package publishing
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
There was a problem hiding this comment.
Code Review
This pull request aims to improve the stability of a flaky test by allowing a small number of buffer exhaustion recovery failures. The approach of replacing an immediate test failure with a counter is sound. However, I've identified a critical issue where a new variable is not initialized, which will cause the test to fail. I've also included a suggestion to improve code maintainability by removing a magic number.
| late int eventsSeen; | ||
| late int recoveriesSeen; | ||
| late int totalRecoveriesSeen; | ||
| late int failedRecoveriesSeen; |
There was a problem hiding this comment.
The newly added variable failedRecoveriesSeen is declared as late but is never initialized. It should be initialized to 0 within the setUp block, similar to the other counters (eventsSeen, recoveriesSeen, and totalRecoveriesSeen). Without initialization, the test will fail with a LateInitializationError when the variable is first accessed at line 109 (++failedRecoveriesSeen).
| expect(totalRecoveriesSeen, lessThan(5)); | ||
| } else { | ||
| // Buffer exhaustion is likely without the isolate but not guaranteed. | ||
| expect(totalRecoveriesSeen, greaterThan(150)); | ||
| // Recovery is not 100% guaranteed on older SDKs. | ||
| expect(failedRecoveriesSeen, lessThan(5)); |
There was a problem hiding this comment.
The magic number 5 is used as a threshold in two separate expectations (lines 116 and 121). To improve readability and maintainability, consider extracting it into a named constant at a suitable scope, for example: const maxAllowedFailures = 5;. This would make the intent of the threshold clearer and easier to modify in the future.
PR HealthLicense Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
This check can be disabled by tagging the PR with API leaks ✔️The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
This check can be disabled by tagging the PR with Breaking changes ✔️
This check can be disabled by tagging the PR with Coverage ✔️
This check for test coverage is informational (issues shown here will not fail the PR). This check can be disabled by tagging the PR with Changelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. This check can be disabled by tagging the PR with |
aceb842 to
b087ff3
Compare
b087ff3 to
62f770c
Compare
Still looking into some test flakiness.