test: re-enable undefined-size grid scroll regression checks#9760
test: re-enable undefined-size grid scroll regression checks#9760web-padawan wants to merge 2 commits into
Conversation
Re-enable the disabled steps in the ItemCountUnknownGridIT scroll-to-end test and drop the stale #1038 / #9166 FIXME. The grid no longer fetches extra earlier ranges after the item count shrinks from an estimate to the real size, so assert that scrolling around after the size is adjusted keeps the size and does not trigger unnecessary fetches. Part of #9166 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
vaadin-review-bot
left a comment
vaadin-review-bot
left a comment
There was a problem hiding this comment.
Reviewed the changes — left 1 comment.
| Finding | |
|---|---|
| 👀 | The re-enabled regression assertion uses a coarse total-fetch-count bound that may not actually fail on the pre-fix behavior it targets. |
| Assert.assertTrue( | ||
| "Scrolling after the size was adjusted must not trigger " | ||
| + "unnecessary fetches for earlier ranges", | ||
| getFetchQueryCount() - fetchCountAfterEnd <= 6); |
There was a problem hiding this comment.
👀 The re-enabled regression assertion uses a coarse total-fetch-count bound that may not actually fail on the pre-fix behavior it targets.
The assertion checks getFetchQueryCount() - fetchCountAfterEnd <= 6, where the delta is the total number of fetches across both doScroll(0, ...) and doScroll(450, ...). This mixes the two legitimate viewport fetches (top and around row 450, whose cached ranges may have been evicted) with the buggy extra earlier-range fetches the test is meant to catch. The comment states the observed delta is 4, leaving only two fetches of headroom.
If the fix ever partially regresses and the grid refetches only a few earlier ranges (delta 5 or 6), the assertion still passes, so the guard against #1038 / flow #9166 is weak. Consider asserting the specific earlier ranges are not refetched (e.g. re-purpose verifyFetchForUndefinedSizeCallback / check no log- entries for offsets below the visited viewports) rather than a magic total-count ceiling; conversely, if viewport or row-height variance in CI pushes the delta to 7, the same bound makes the test flaky.
ItemCountUnknownGridIT.java:66 · uncategorized · plausible
There was a problem hiding this comment.
Updated to assert the specific ranges as suggested.
Replace the total fetch-count ceiling with a check that scrolling after the size is adjusted only fetches the visited viewports, never the ranges in between. This catches an off-viewport refetch regardless of count and avoids flakiness from viewport variance. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
2 participants
Description
Re-enable the disabled steps in the ItemCountUnknownGridIT scroll-to-end test and drop the stale FIXME. The grid no longer fetches extra earlier ranges after the item count shrinks from an estimate to the real size, so assert that scrolling around after the size is adjusted keeps the size and does not trigger unnecessary fetches.
Related to vaadin/flow#9166
Type of change