drivaspreset commented on PR #43004: URL: https://github.com/apache/superset/pull/43004#issuecomment-5961057402
@sadpandajoe — before you re-review, a note on something I did between rounds, so you aren't re-deriving it. Rather than keep fixing reported lines one at a time, I went back over all of your review comments on this PR and grouped them. They fall into four themes, and one of them is 11 of the last 12: | Theme | Status | |---|---| | Reuse / DRY — use the existing API helpers, extract duplicated fixture setup, split the file | resolved | | CI plumbing — required matrix, `if: success()`, change detector, zizmor, GHCR mirrors, shared test config | resolved | | **Assertions that cannot fail** | the recurring one | | Comments that contradict the code | resolved | The third theme is one defect in four shapes: 1. **Asserting state that was already true before the action** — the multi-chart text comparison; `errorAlert` before and after `forceRefresh()`; the pre-filter `expectedGirlText` baseline. 2. **Not identifying which of several in-flight requests is being asserted** — `submitStatusFor` returning whichever responded first; `toBeDefined()` accepting a 4xx. 3. **Asserting something the implementation makes unreachable** — the anti-clobber wait, since `chartAction.ts` aborts the superseded request. 4. **Leaving a component that can fail silently unasserted** — the chart in the filter-dropdown test, which had been erroring on every run unnoticed. So I audited every test in the suite for the same shapes instead of waiting for the next report. Three sites had it and were not yet reported: - **`forced dashboard refresh goes through the GAQ 202 -> poll -> done cycle`** — asserted the chart was visible and showed a digit *after* refreshing. Both were already true beforehand, on static data that reproduces the same number: the same defect you reported for the multi-chart test. Its chart now renders the query clock, so the refresh has to advance a value that cannot advance on its own. - **`reloading an already-cached dashboard...`** — asserted only "a digit" after reloading. A cache hit has to reproduce the *cached result*, so it now asserts that exact value. - **`navigating away mid-load and back...`** — same: returning must show the value it had before leaving, not merely some number. The query-clock dataset and chart spec were inline in the multi-chart test; they are now shared helpers (`createQueryClockDataset`, `BIG_NUMBER_QUERY_CLOCK_SPEC`, `readQueryClock`) used by both, rather than duplicating fixture setup the way the August comments called out. I also audited the four surviving `toHaveText(/\d/)` uses and deliberately kept them: two capture a baseline for a later exact-value assertion, one proves a cold first load rendered *from nothing*, and one proves recovery from an error alert to a value. All can fail. Everything above is green — 80 checks, and the suite reports `10 passed, 0 failed, 0 flaky, 0 skipped` on both matrix legs. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
