DataGrid - stabilize tests - #34642
Conversation
There was a problem hiding this comment.
Pull request overview
This PR modifies DataGrid TestCafe E2E tests in an attempt to improve stability around hover rendering and React header template rendering checks.
Changes:
- Reworked the “row hover with fixed columns” sticky-columns test to hover a specific cell rather than the whole row.
- Added explicit readiness/content assertions before taking screenshots in the Select All +
headerCellTemplate(React) test. - Introduced repeated (looped) execution patterns for the affected tests (and a
test.onlyin one file), which currently makes the suite heavier and risks altering what runs in CI.
Reviewed changes
Copilot reviewed 2 out of 102 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| e2e/testcafe-devextreme/tests/dataGrid/sticky/common/stickyColumns.ts | Changes hover target for the fixed-columns row-hover screenshot test; currently introduces repeated execution and test.only. |
| e2e/testcafe-devextreme/tests/dataGrid/common/selection.ts | Adds readiness assertions for the React header template scenario; currently runs the same screenshot test many times with varying screenshot names. |
Suppressed comments (1)
e2e/testcafe-devextreme/tests/dataGrid/common/selection.ts:61
- The screenshot name is currently parameterized with
i, which forces maintaining multiple etalons for the same scenario and makes the test harder to maintain. If the test is stable, prefer a single canonical screenshot name.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 104 changed files in this pull request and generated no new comments.
Suppressed comments (6)
e2e/testcafe-devextreme/tests/dataGrid/common/keyboardNavigation/keyboardNavigation.functional.ts:1792
test.only(...)will cause TestCafe to run only this test and skip the rest of the suite, which is almost never intended in committed code.
e2e/testcafe-devextreme/tests/dataGrid/common/keyboardNavigation/keyboardNavigation.functional.ts:1821- Leftover debugging comment (
// fails here) should be removed before merging to keep the test suite clean and avoid implying a known failing assertion.
e2e/testcafe-devextreme/tests/dataGrid/sticky/common/stickyColumns.ts:92 - Defining the same visual test 50 times via a for-loop will multiply E2E runtime and screenshots work by 50x. If this was intended for local flakiness investigation, it shouldn’t be committed as-is; prefer a single test and use the runner’s retry/quarantine mechanisms instead (runner.ts treats meta.unstable as quarantined).
e2e/testcafe-devextreme/tests/dataGrid/common/selection.ts:48 - This previously ran as a single quarantined/unstable visual test; the new for-loop defines 50 separate tests, which will significantly increase suite runtime and still won’t provide a stable pass/fail signal. Consider keeping this as one test and relying on meta.unstable (runner.ts treats it as quarantined) and/or retryAttempts instead of cloning tests.
e2e/testcafe-devextreme/tests/dataGrid/common/keyboardNavigation/startEditing.functional.ts:127 - The new for-loop defines this same keyboard-navigation test 100 times, which will dramatically increase CI time and load without changing coverage. This looks like local flakiness debugging code and should be reduced back to a single test (use retries/quarantine if needed).
e2e/testcafe-devextreme/tests/dataGrid/common/keyboardNavigation/keyboardNavigation.functional.ts:1791 - This loop defines 100 copies of the same test, which will heavily slow down E2E runs and create redundant work. If you need repeated runs for flakiness investigation, prefer runner retries/quarantine instead of duplicating tests.
This issue also appears in the following locations of the same file:
- line 1792
- line 1821
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 104 changed files in this pull request and generated no new comments.
Suppressed comments (5)
e2e/testcafe-devextreme/tests/dataGrid/common/keyboardNavigation/keyboardNavigation.functional.ts:1821
- Leftover debugging comment (
// fails here) should be removed before merging; it doesn’t provide actionable context and will go stale as the test changes.
e2e/testcafe-devextreme/tests/dataGrid/sticky/common/stickyColumns.ts:96 - Wrapping this screenshot test in a 50-iteration loop multiplies runtime and adds 50 near-duplicate test cases. The e2e runner already supports rerunning failed tests (e2e/testcafe-devextreme/runner.ts:395-430), so this repetition should not be committed as a stabilization mechanism.
Reduce this to a single test case (or rely on the runner retries/quarantine mechanisms) to avoid bloating CI time and reporting.
e2e/testcafe-devextreme/tests/dataGrid/common/selection.ts:52
- This adds 50 duplicate TestCafe tests, which will significantly increase the DataGrid e2e runtime and clutter the test report. If the goal is flake mitigation, prefer the existing retry/quarantine mechanisms in the runner instead of multiplying test definitions.
Please keep a single test case here.
e2e/testcafe-devextreme/tests/dataGrid/common/keyboardNavigation/startEditing.functional.ts:128
- Repeating this functional test 100 times will heavily inflate suite runtime without improving determinism (each run still has to pass). Please keep this as a single test and use the runner’s retry/quarantine mechanisms if flakiness remains.
e2e/testcafe-devextreme/tests/dataGrid/common/keyboardNavigation/keyboardNavigation.functional.ts:1794 - This introduces 100 duplicate tests per edit mode, which will drastically increase execution time and make failures harder to triage. The existing retry/quarantine mechanisms are a better fit for stabilizing flakes.
Please keep this as a single test case per edit mode.
This issue also appears on line 1821 of the same file.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 105 changed files in this pull request and generated no new comments.
Suppressed comments (5)
e2e/testcafe-devextreme/tests/dataGrid/common/keyboardNavigation/keyboardNavigation.functional.ts:1821
- Leftover debug comment
// fails hereshould not be committed; it adds noise without actionable information. If this assertion is flaky, stabilize it with an assertion timeout or a deterministic wait instead.
e2e/testcafe-devextreme/tests/dataGrid/common/selection.ts:51 - This change repeats the same screenshot assertion 50 times by generating 50 distinct tests and 50 screenshot names. That will substantially slow the suite and makes the test harder to triage. It also drops the previous
test.meta({ unstable: true }), which is how the e2e runner quarantines flaky tests (seee2e/testcafe-devextreme/runner.ts). Prefer a single test with an explicit wait for the async header template (as you already added) and keep it quarantined (unstable meta) if it is still flaky, rather than duplicating the test.
e2e/testcafe-devextreme/tests/dataGrid/common/keyboardNavigation/keyboardNavigation.functional.ts:1794 - The
for (let i = 0; i < 100; i++)loop will create 100 extra tests pereditModevalue. Given this file is already parameterized by edit mode, this can balloon to hundreds of tests, significantly extending e2e runtime and increasing memory/state-leak risk underdisablePageReloadspatterns elsewhere. Prefer stabilizing the single test via proper waits/timeouts (e.g.,t.expect(...).ok({ timeout: ... })) and/or relying on the runner’s retry/quarantine features rather than duplicating the test.
This issue also appears on line 1821 of the same file.
e2e/testcafe-devextreme/tests/dataGrid/common/keyboardNavigation/startEditing.functional.ts:139
- Wrapping this scenario in
for (let i = 0; i < 100; i++)creates 100 identical tests, each recreating the widget in a.beforehook. This will drastically increase runtime and can hide the underlying flakiness rather than fixing it. Prefer a single test and stabilize it (e.g., by using assertion timeouts on focus/editor readiness) and/or rely on the existing e2e runner retry/quarantine mechanisms instead of test duplication.
e2e/testcafe-devextreme/tests/dataGrid/sticky/common/stickyColumns.ts:96 - The new
for (let i = 0; i < 50; i++)loop turns one visual test into 50 separate TestCafe tests (each running its own.before(createWidget(...))). This will significantly increase suite runtime and can amplify state leakage because the fixture usesdisablePageReloads. It also removes the previousunstable: truemeta, which the repo’s e2e runner uses to quarantine flaky tests (seee2e/testcafe-devextreme/runner.tslogic that treatstestMeta.unstableas quarantined). Prefer keeping a single test and stabilizing it via deterministic waits/timeouts (or leaving it quarantined) rather than multiplying test cases.
| })); | ||
|
|
||
| test(`${editMode} mode - Cells in a modified row should be updated on Tab (T898356)`, async (t) => { | ||
| test(`${i}: ${editMode} mode - Cells in a modified row should be updated on Tab (T898356)`, async (t) => { |
No description provided.