Repository navigation
Fix Flaky Tests - #4041
Fix Flaky Tests#4041brandonschabell wants to merge 1 commit into
Conversation
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The regression test can pass without exercising the debounce race, and the changelog omits the user-facing fix.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Stabilizes flaky integration tests, fixes debounced number-input stepper behavior, and improves benchmark reporting.
Changes:
- Replaces timing sleeps and immediate assertions with condition-based waits.
- Fixes
dcc.Inputstepper updates during debounce and adds regression coverage. - Publishes benchmark results through job summaries, including fork PRs.
| File | Description |
|---|---|
tests/websocket/test_ws_stream.py |
Synchronizes streamed frame assertions. |
tests/websocket/test_ws_get_prop.py |
Waits for the target element. |
tests/streaming/test_stream_callbacks_integration.py |
Synchronizes progressive rendering. |
tests/integration/test_csp.py |
Waits for callback output. |
tests/integration/renderer/test_dependencies.py |
Replaces retries with explicit waits. |
tests/integration/devtools/test_callback_validation.py |
Waits for visible error titles. |
tests/integration/callbacks/test_basic_callback.py |
Synchronizes callbacks per keystroke. |
tests/conftest.py |
Delays forced exit until pytest teardown. |
tests/background_callback/test_basic_long_callback008.py |
Avoids problematic disk-cache locking. |
components/dash-table/tests/selenium/test_pagination.py |
Waits for pagination state. |
components/dash-core-components/tests/integration/store/test_component_props.py |
Uses timestamp bounds instead of tolerance. |
components/dash-core-components/tests/integration/input/test_number_input.py |
Adds debounce-stepper regression coverage. |
components/dash-core-components/tests/integration/dropdown/test_dropdown_search_order.py |
Synchronizes dropdown focus and filtering. |
components/dash-core-components/tests/integration/dropdown/test_a11y.py |
Replaces retries with focus-aware waits. |
components/dash-core-components/src/components/Input.tsx |
Prevents stale debounced stepper updates. |
CHANGELOG.md |
Records test stabilization. |
.github/workflows/benchmarks.yml |
Writes benchmark results to job summaries. |
.ai/PERFORMANCE.md |
Documents benchmark reporting behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| for _ in range(5): | ||
| increment_btn.click() | ||
| dash_dcc.wait_for_text_to_equal("#div-fast", "103") | ||
| decrement_btn.click() | ||
| dash_dcc.wait_for_text_to_equal("#div-fast", "100") |
| - [#3986](https://github.com/plotly/dash/pull/3986) Adjust `_run_before_hooks` in the `fastapi` backend to honor a response returned by a `before_request` function, matching the `flask` backend's behavior. | ||
|
|
||
| ### Fixed | ||
| - [#4041](https://github.com/plotly/dash/pull/4041) Fix flaky integration tests by replacing fixed sleeps and immediate asserts with waits, and remove their `@flaky` retry markers. |
T4rk1n
left a comment
There was a problem hiding this comment.
The Input.tsx fix looks right. Calling onEvent() from the stepper reset pendingEvent to undefined, so the next debounceEvent couldn't clear the old timer and that timer later reverted the value. test_inni012 covers this case. The background, streaming, websocket and conftest changes also look fine.
One thing I'd want fixed before merge: test_a11y008 and test_ddso002 lose @flaky but still send some keys on a fixed sleep. In test_a11y008 there's also an immediate assert after HOME. These are the same focus races the PR removes elsewhere, and now nothing retries them. The Input fix also needs its own CHANGELOG line.
| send_keys(Keys.ARROW_DOWN) # Move to fourth option | ||
| sleep(0.05) | ||
| assert get_focused_option_text() == "Option 3" | ||
| wait_for_focus(dash_duo, "Option 3") |
There was a problem hiding this comment.
The assert at 437 runs right after HOME, but focus moves in a rAF, so under load it reads the old option and fails. With @flaky gone that's a hard failure. Can you use wait_for_focus(dash_duo, "search") here? Also please swap the sleep(0.05) calls at 427/429/431 for waits on Option 0/1/2, and add a wait for "search" after the HOME at 458 and for "Option 9" in place of the sleep(0.1) at 460. If a key lands before focus does, it goes to the wrong element, and the later wait just times out.
|
|
||
| # Select '12' (second option) | ||
| send_keys(Keys.ARROW_DOWN) | ||
| sleep(0.2) |
There was a problem hiding this comment.
Same issue: the sleep(0.2) calls at 86, 95 and 104 still gate keys on timing. Can you replace them with wait_for_focus on the first option and on "search" after HOME? Also, wait_for_focus is now copied in two files. Could it go into a shared helper?
|
|
||
|
|
||
| @pytest.hookimpl(trylast=True) | ||
| _session_exitstatus = 0 |
There was a problem hiding this comment.
This defaults to 0. If pytest fails in pytest_configure or pytest_sessionstart, pytest_sessionfinish never runs but pytest_unconfigure still does. Then os._exit(0) turns a broken run into a green job. Can you default it to a non-zero code, or skip the forced exit when sessionfinish never ran?
| - [#3986](https://github.com/plotly/dash/pull/3986) Adjust `_run_before_hooks` in the `fastapi` backend to honor a response returned by a `before_request` function, matching the `flask` backend's behavior. | ||
|
|
||
| ### Fixed | ||
| - [#4041](https://github.com/plotly/dash/pull/4041) Fix flaky integration tests by replacing fixed sleeps and immediate asserts with waits, and remove their `@flaky` retry markers. |
There was a problem hiding this comment.
The stepper fix in dcc.Input changes what users see, but this entry only talks about tests. Can you add a separate Fixed line, e.g. "Fix dcc.Input number stepper reverting to a previous value when clicked during debounce."?
Fix flaky tests. Fix issue with input reverting to previous value during debounce
Contributor Checklist
optionals
CHANGELOG.md