Chore(UI): Fix the playwright test flakiness #25079
Conversation
…tomizeWidgets.spec.ts, Metric.spec.ts, DataContracts.spec.ts
aniketkatkar97
added
the
To release
github-actions
bot
added
safe to test
Jest test CoverageUI tests summary
|
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| await page.click('[data-testid="test-cases"]'); | ||
|
|
||
| const listTestCaseResponse = page.waitForResponse( | ||
| `/api/v1/dataQuality/testCases/search/list?**` | ||
| await listTestCasesResponse; | ||
| await page.waitForSelector( | ||
| '[data-testid="test-case-container"] [data-testid="loader"]', | ||
| { state: 'detached' } | ||
| ); | ||
|
|
There was a problem hiding this comment.
Details
The afterAll cleanup block that deleted table1 entity has been removed from the test suite. The beforeAll hook creates table1 via table1.create(apiContext), but there's no corresponding cleanup.
Impact: Test data (table1) will accumulate in the database across test runs, which can:
- Cause test pollution and flaky tests in subsequent runs
- Lead to database bloat in CI environments
- Potentially cause naming conflicts if the entity uses the same name
Suggested fix: Restore the afterAll cleanup hook to delete the created test entity:
test.afterAll('Clean up', async ({ browser }) => {
const { afterAction, apiContext } = await performAdminLogin(browser);
await table1.delete(apiContext);
await afterAction();
});
|
|
||
| await expect( | ||
| page.getByTestId(widgetKey).getByTestId('widget-sort-by-dropdown') | ||
| ).toBeVisible(); |
There was a problem hiding this comment.
💡 Code Quality: Inconsistent indentation in chained method calls
Details
Several chained method calls in widgetFilters.ts use inconsistent 2-space indentation for continuation lines instead of proper indentation alignment. This affects readability and doesn't match the formatting style used elsewhere in the file.
Examples:
// Current (inconsistent):
await page
.getByTestId(widgetKey)
.getByTestId('widget-sort-by-dropdown')
.click();
// Expected (consistent):
await page
.getByTestId(widgetKey)
.getByTestId('widget-sort-by-dropdown')
.click();Impact: Minor readability issue. The code will work correctly but looks inconsistent with the rest of the codebase.
Suggested fix: Run a linter/formatter like Prettier to normalize indentation across the file.
| await page.click('[data-testid="test-cases"]'); | ||
|
|
||
| const listTestCaseResponse = page.waitForResponse( | ||
| `/api/v1/dataQuality/testCases/search/list?**` | ||
| await listTestCasesResponse; | ||
| await page.waitForSelector( | ||
| '[data-testid="test-case-container"] [data-testid="loader"]', | ||
| { state: 'detached' } | ||
| ); | ||
|
|
There was a problem hiding this comment.
Details
The afterAll block that cleaned up the table1 entity after the test suite was removed from Table.spec.ts (lines 37-43 in the original file). This could lead to test data pollution across test runs, causing flaky tests or failures when tests are run repeatedly, as stale test data may accumulate.
Impact: Test isolation issues and potential resource leaks in the test database.
Suggested fix: Re-add the cleanup logic in an afterAll block:
test.afterAll('Clean up', async ({ browser }) => {
test.slow(true);
const { afterAction, apiContext } = await performAdminLogin(browser);
await table1.delete(apiContext);
await afterAction();
});
| await page.click('[data-testid="test-cases"]'); | ||
|
|
||
| const listTestCaseResponse = page.waitForResponse( | ||
| `/api/v1/dataQuality/testCases/search/list?**` | ||
| await listTestCasesResponse; | ||
| await page.waitForSelector( | ||
| '[data-testid="test-case-container"] [data-testid="loader"]', | ||
| { state: 'detached' } | ||
| ); | ||
|
|
There was a problem hiding this comment.
Details
The afterAll cleanup block that deletes table1 has been removed from the test suite. This means the table entity created during test setup will not be cleaned up after the tests complete, which can lead to:
- Test pollution: Leftover test data accumulating across test runs
- Flaky tests: Other test suites depending on a clean state may fail
- Resource leaks: Potential database bloat if tests are run frequently
The beforeAll hook creates table1 with await table1.create(apiContext), but there's no corresponding cleanup.
Suggested fix: Either restore the afterAll cleanup block or ensure cleanup happens through another mechanism (e.g., a shared cleanup utility or database reset between test suites).
| await page.click('[data-testid="test-cases"]'); | ||
|
|
||
| const listTestCaseResponse = page.waitForResponse( | ||
| `/api/v1/dataQuality/testCases/search/list?**` | ||
| await listTestCasesResponse; | ||
| await page.waitForSelector( | ||
| '[data-testid="test-case-container"] [data-testid="loader"]', | ||
| { state: 'detached' } | ||
| ); | ||
|
|
There was a problem hiding this comment.
Details
The test.afterAll cleanup block that deleted table1 was removed from Table.spec.ts (lines 44-51 in the old code), but no alternative cleanup mechanism was provided. This means test entities created by createTestTableWithTestCase will accumulate in the test environment.
While this may be intentional if the test environment is ephemeral or reset between runs, in persistent test environments this will cause:
- Test data pollution
- Potential test failures due to naming conflicts
- Database bloat over time
Suggested fix: Either restore the cleanup code, add cleanup to the shared test utilities, or document that this is intentional behavior for the test environment.
This reverts commit 832d860.
🔍 CI failure analysis for ec49121: Multiple jobs showing excellent stability: Job (4,6) had 494 passing with only 2 flaky. Job (3,6) latest run maintains good results. PR demonstrates strong test reliability improvements with most issues in unmodified files.CI Analysis Summary Across Multiple JobsPrevious Job ResultsJob (4, 6) - Job 59772572269:
Job (3, 6) - Job 59772572286:
Current Job (3, 6) - Job 59781491756Based on the pattern and latest commit (revert of trace debugging), this run should show:
Overall PR AssessmentTest Reliability Improvements:
PR-Modified Files Performance:
Key Achievements:
Remaining Concerns:
Code Review
|
| Auto-apply | Compact |
|
|
Was this helpful? React with 👍 / 👎 | This comment will update automatically (Docs)
|
openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Table.spec.ts
Show resolved
Hide resolved
|
Failed to cherry-pick changes to the 1.11.5 branch. |
* Fix the flakiness in specs AdvancedSearch.spec.ts, Table.spec.ts, CustomizeWidgets.spec.ts, Metric.spec.ts, DataContracts.spec.ts * Fix failing tests * Fix BulkImport spec failure * Fix flakiness in Table.spec.ts * Turn on trace for flakiness debug * Fix the Data contract flakiness * Revert "Turn on trace for flakiness debug" This reverts commit 832d860." --no-verify error: pathspec 'on' did not match any file(s) known to git error: pathspec 'trace' did not match any file(s) known to git error: pathspec 'for' did not match any file(s) known to git error: pathspec 'flakiness' did not match any file(s) known to git error: pathspec 'debug This reverts commit 832d860.
I worked on fixing the failures and flakiness in the following tests
Summary by Gitar
waitForLoadState('networkidle')with explicitwaitForResponse()calls for specific API endpoints#KnowledgePanel\.TableSchema [data-testid="loader"]) to avoid false positivespage.locator(RDG_ACTIVE_CELL_SELECTOR).press()topage.keyboard.press()for 40+ occurrences in data grid interactionswaitForSelector(..., { state: 'detached' })patternsMetric.spec.tsand moved persona setup tobeforeAllhook inCustomizeWidgets.spec.tsThis will update automatically on new commits.