Skip to content

test: fix flaky e2e screenshot tests - #1406

Merged
dsmmcken merged 5 commits into
mainfrom
claude-flaky-e2e-fixes
Aug 27, 2026
Merged

test: fix flaky e2e screenshot tests#1406
dsmmcken merged 5 commits into
mainfrom
claude-flaky-e2e-fixes

Conversation

@dsmmcken

@dsmmcken dsmmcken commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Split out of #1402, which picked up a handful of e2e fixes that have nothing to do with the TradingView plugin.

Most of the flakiness is the same bug in different places: a screenshot gets taken before the widget's data has painted. The loading spinner clears when the widget mounts, not when it has data, and Playwright's auto-stabilization doesn't help — a blank plot is static, so two frames match and it locks onto the empty state.

  • waitForPlotlyData / waitForPlotlyIndicator in tests/utils, used by the express, ui_plotly and theme specs. Checks Plotly's data model rather than the DOM so WebGL traces are covered too. Indicators get their own helper because their value is a scalar, not an array.
  • ag_grid waits for a row, ui_table waits for waitForGridRender.
  • waitForGridRender samples bands instead of reading back the whole canvas — the full getImageData is ~8MB at 1080p and was wedging the page's main thread under a parallel run.
  • Page-load and Panels-button timeouts up to 45s; three browsers against one server regularly blow past the default 15s.
  • openPanel only counts .dh-panel when the caller passed a specific locator — the generic selector also matches the server's default panels, which race the count. Otherwise it checks the tab title.

Two others while in here:

  • ui_dialog screenshots the dialog instead of the page. The modal and fullscreen dialogs cover the viewport, so the full-page shots were also capturing IDE chrome that drifts run to run (console log lines, heap indicator), which made the baselines ping-pong. Popover and tray were already scoped and are untouched.
  • useDebouncedOnChange was dropping keystrokes. The server hands down a new on_change on every re-render, and useDebouncedCallback cancels its pending trailing call when the callback identity changes — so typing "world" quickly sends "wor", the re-render lands, and "world" never reaches Python. Now keeps one debounced instance and reads the latest callable from a ref.

t_programmatic_sort_abs_desc also had is_abs=True on a string column, which means nothing; pointed it at SepalLength, which has negatives.

Testing

tsc and eslint/prettier clean, useDebouncedOnChange unit tests pass. The regenerated baselines need a CI e2e run to confirm.

Don McKenzie and others added 4 commits August 27, 2026 11:23
Several screenshot tests capture a widget before its data has painted.
The loading spinner clears when the widget mounts, not when it has data,
and Playwright's auto-stabilization does not save us: a blank plot or an
empty grid is *static*, so two consecutive frames match and the
screenshot locks onto the empty state.

- Add `waitForPlotlyData` / `waitForPlotlyIndicator` to tests/utils and
  use them in express, ui_plotly and theme specs. Data is checked on
  Plotly's data model rather than the DOM so WebGL (`scattergl`) traces
  are covered, and indicators need their own helper because their value
  is a scalar, not an array.
- Wait for a rendered row before the ag_grid screenshots, and for
  `waitForGridRender` before the ui_table prop-change screenshot.
- Sample bands in `waitForGridRender` instead of reading back the whole
  canvas. A full getImageData is ~8MB at 1080p, and under a fully
  parallel run that readback wedges the page's main thread long enough
  for the poll to time out on a grid that painted fine.
- Raise the page-load and Panels-button timeouts to 45s. Three browser
  projects against one server routinely exceed the default 15s.
- Only trust `.dh-panel` counting in `openPanel` when the caller passed a
  widget-specific locator; the generic selector also matches the
  server's default-layout panels, which load asynchronously and race the
  count on slower browsers. Otherwise verify the Golden Layout tab title.
- Await the `toBeEnabled` assertion on the target panel button, which was
  a floating promise and asserted nothing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`my_modal`, `my_fullscreen` and `my_fullscreen_takeover` cover (or nearly
cover) the viewport, so their full-page screenshots also captured IDE
chrome that varies run to run — console history picking up server log
lines, the heap-usage indicator drifting. That made the baselines
ping-pong and baked in below-threshold noise that `--update-snapshots`
then refused to refresh. Scope the shot to the dialog, which is what the
tests are actually about. `my_popover` and `my_tray` were already scoped
to the widget panel and are unchanged.

`my_fullscreen_takeover` fills the whole viewport, so its baseline is
unchanged; the other two are regenerated.

Also give the dialog a 30s visibility timeout — a first-ever fixture
render on a cold server under full-suite load can exceed the default 15s.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`t_programmatic_sort_abs_desc` applied `is_abs=True` to `Name`, a string
column, where an absolute-value sort is meaningless. Point it at
`SepalLength` instead, which carries negative values and so actually
exercises abs sorting, and regenerate the baselines.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The server hands down a new `on_change` callable on every re-render,
including the re-render caused by its own previous `onChange`. Because
`useDebouncedCallback` cancels its pending trailing call whenever the
callback identity changes, keying the debounce on `propOnChange` silently
dropped whatever the user typed while a round trip was in flight: type
"world" quickly and "wor" fires, the re-render lands, and the trailing
"world" call is cancelled — the final keystrokes never reach Python.

Keep one stable debounced instance and resolve the latest callable from a
ref at fire time. Adds unit tests for the debounce, the identity-change
case, and cancel-on-unmount.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings August 27, 2026 18:04
@github-actions

Copy link
Copy Markdown

ui docs preview (Available for 14 days)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Stabilizes asynchronous E2E screenshots and prevents debounced UI changes from being dropped during server re-renders.

Changes:

  • Adds Plotly/grid rendering waits and more reliable panel-opening synchronization.
  • Scopes dialog screenshots and corrects the absolute-sort fixture.
  • Stabilizes debounced onChange callbacks with unit coverage.

Reviewed changes

Copilot reviewed 10 out of 19 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
plugins/ui/src/js/src/elements/hooks/useDebouncedOnChange.test.ts Tests debounced callback behavior.
plugins/ui/src/js/src/elements/hooks/useDebouncedOnChange.ts Preserves pending callbacks across re-renders.
tests/ag_grid.spec.ts Waits for rendered rows.
tests/app.d/ui_table.py Uses a numeric absolute-sort column.
tests/express.spec.ts Waits for Plotly data and indicators.
tests/theme.spec.ts Waits for theme chart data.
tests/ui_dialog.spec.ts Captures dialog elements directly.
tests/ui_dialog.spec.ts-snapshots/UI-dialog-components-my-fullscreen-1-chromium-linux.png Updates Chromium fullscreen baseline.
tests/ui_dialog.spec.ts-snapshots/UI-dialog-components-my-fullscreen-1-firefox-linux.png Updates Firefox fullscreen baseline.
tests/ui_dialog.spec.ts-snapshots/UI-dialog-components-my-fullscreen-1-webkit-linux.png Updates WebKit fullscreen baseline.
tests/ui_dialog.spec.ts-snapshots/UI-dialog-components-my-modal-1-chromium-linux.png Updates Chromium modal baseline.
tests/ui_dialog.spec.ts-snapshots/UI-dialog-components-my-modal-1-firefox-linux.png Updates Firefox modal baseline.
tests/ui_dialog.spec.ts-snapshots/UI-dialog-components-my-modal-1-webkit-linux.png Updates WebKit modal baseline.
tests/ui_plotly.spec.ts Waits for Plotly data.
tests/ui_table.spec.ts Adds grid rendering synchronization.
tests/ui_table.spec.ts-snapshots/UI-table-t-programmatic-sort-abs-desc-1-chromium-linux.png Updates Chromium sort baseline.
tests/ui_table.spec.ts-snapshots/UI-table-t-programmatic-sort-abs-desc-1-firefox-linux.png Updates Firefox sort baseline.
tests/ui_table.spec.ts-snapshots/UI-table-t-programmatic-sort-abs-desc-1-webkit-linux.png Updates WebKit sort baseline.
tests/utils.ts Adds rendering waits and panel synchronization improvements.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread tests/ui_table.spec.ts
Comment thread tests/theme.spec.ts
dsmmcken pushed a commit to jnumainville/deephaven-plugins that referenced this pull request Aug 27, 2026
These fixes had nothing to do with the TradingView plugin — they were
picked up along the way while getting the e2e suite green. They now live
in deephaven#1406 so they can land and be reviewed on their own:

- ag_grid / express / theme / ui_plotly / ui_table screenshot waits
- the ui_dialog dialog-scoped screenshots and their baselines
- the `t_programmatic_sort_abs_desc` numeric-column fixture and baselines
- the `useDebouncedOnChange` pending-call fix and its unit tests
- the Plotly wait helpers and the `waitForGridRender` band sampling

`tests/utils.ts` is left byte-identical to deephaven#1406 plus `waitForTvlSettled`,
since the tvl specs call `openPanel` without a locator and so depend on
its tab-title path. Once deephaven#1406 merges, merging main here collapses that
file's diff down to `waitForTvlSettled` alone.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@dsmmcken dsmmcken changed the title test: Fix flaky e2e screenshot tests, split out of #1402 test: Fix flaky e2e screenshot tests Aug 27, 2026
@dsmmcken dsmmcken changed the title test: Fix flaky e2e screenshot tests test: fix flaky e2e screenshot tests Aug 27, 2026
Both spotted in review on #1406.

ui_table's prop-change test only waited for the initial paint. Each
toggle round-trips to the server, so wait for the button to relabel
itself before each subsequent screenshot — the label and the table's new
props land in the same render.

theme's screenshot covers the whole page, which includes the demo's
"Stocks Table" grid as well as the plot, so waiting on Plotly alone left
the grid racing. Wait for both.

Neither shows up as a random failure: against an existing baseline a
stale frame just retries. They bite at --update-snapshots time, where
there is nothing to compare against and a static stale frame gets
written as the new baseline.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 27, 2026 18:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 19 changed files in this pull request and generated no new comments.

@github-actions

Copy link
Copy Markdown

ui docs preview (Available for 14 days)

@dsmmcken
dsmmcken requested a review from jnumainville August 27, 2026 18:38
Comment thread tests/ui_table.spec.ts

const locator = page.locator(SELECTORS.WIDGET_LOADER_ELEMENT_VISIBLE);

// Wait for the grid to paint its data so the screenshot isn't of a blank grid

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little overly verbose

@dsmmcken
dsmmcken merged commit 2b7aa63 into main Aug 27, 2026
25 checks passed
@dsmmcken
dsmmcken deleted the claude-flaky-e2e-fixes branch August 27, 2026 18:57
mofojed pushed a commit that referenced this pull request Aug 27, 2026
Follow-up to #1406.

## Why the ref is there at all

`useDebouncedCallback` cancels its pending call whenever the callback
identity changes.

A handler the user doesn't memoize is a new Python function on every
render, which gets a new callable id, which reaches the client as a new
`onChange`. So when a server re-render lands while a debounced change is
still waiting to fire, that change is cancelled and those keystrokes
never reach Python.

That is what made `ui_events.spec.ts` flaky. Type "world" quickly:

```
type "w" "o" "r"      debounce armed with "wor"
250ms                 "wor" sent to Python, handler appends to the log
type "l" "d"          debounce re-armed with "world"
server render lands   new handler -> new onChange -> pending call CANCELLED
                      "change:world" never arrives, the test times out
```

Locally all five keystrokes land inside one 250ms window, so nothing
re-renders mid-typing and it passes. Under CI load the gaps stretch, the
re-render lands in the wrong spot, and it fails.

The fix in #1406 keeps one debounced instance for the life of the
component and reads the current handler from a ref when it fires, so
nothing cancels it.

## What this PR changes

The ref was being updated in a `useEffect`, so it lagged a render
behind. The value is already available at render time, so it is now
assigned there. This matches how the rest of the repo keeps a
latest-value ref (`usePivotBuilderMiddlewareCore`,
`useWaitForWorkerVariables`, `CreatePivotPage`).

Also shortened the comment. It said the server sends a new `on_change`
on every render, which is only true for handlers the user doesn't
memoize. A memoized one keeps a stable callable id, and so does a
`use_state` setter since #1386.

No behavior change.

## Testing

Unit tests pass; `tsc`, eslint and prettier clean.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Don McKenzie <donmckenzie@illumon.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants