Skip to content

Conversation

@aniketkatkar97
Copy link
Member

This pull request refactors and improves Playwright E2E tests for the OpenMetadata UI, focusing on code maintainability, test reliability, and clarity. The main changes include replacing UI-based entity setup with direct API patching, removing unnecessary cleanup steps, improving test synchronization with API responses, and refactoring repeated string constructions for clarity and maintainability.

Test setup and teardown improvements:

  • In ExploreQuickFilters.spec.ts, entity setup in beforeAll now uses direct API patching for tags, tier, and domain assignments, replacing UI interactions for faster and more reliable test initialization. Unnecessary cleanup steps in afterAll are removed.
  • In ImpactAnalysis.spec.ts, redundant afterAll cleanup code is removed since entity lifecycle is now managed more efficiently.

Test reliability and synchronization:

  • Added explicit waits for upstream lineage API responses in ImpactAnalysis.spec.ts to ensure assertions only occur after data is loaded, reducing flakiness. [1] [2] [3]

Code clarity and maintainability:

  • Refactored string construction for column FQNs in Lineage.spec.ts by introducing variables and using them in edge ID generation, reducing repetition and improving readability. [1] [2] [3] [4] [5] [6]

Minor test logic fixes:

  • Updated filter value replacement logic to use replaceAll for consistency in ExploreQuickFilters.spec.ts.
  • Fixed filter variable usage and assertion placement in quick filter tests for better accuracy. [1] [2]
  • Ensured safe handling of potentially undefined values in global search filter persistence test.
Screenshot 2026-01-22 at 6 48 00 PM Screenshot 2026-01-22 at 7 19 48 PM

@github-actions
Copy link
Contributor

github-actions bot commented Jan 22, 2026

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 65%
65.8% (55627/84538) 44.83% (28695/64002) 47.68% (8738/18325)

@gitar-bot
Copy link

gitar-bot bot commented Jan 27, 2026

🔍 CI failure analysis for fe64724: CI run had same 3 ColumnBulkOperations hard failures (lines 936, 1075, 1134) persisting across multiple commits + 8 flaky tests; 550 of 561 tests passed in 52.3 min

Issue

The playwright-ci-postgresql (2, 6) CI job failed with 3 hard failures and 8 flaky tests. Of 561 total tests, 550 passed in 52.3 minutes.

Root Cause

CRITICAL: Same 3 tests continue to fail consistently across multiple commits:

All 3 hard failures are in ColumnBulkOperations.spec.ts:

  1. Line 936 - "should select multiple columns and bulk edit"
  2. Line 1075 - "should open edit drawer when clicking on aggregate row"
  3. Line 1134 - "should apply multiple filters together"

These failures have persisted through:

  • Commit 91aa272 ("Fix flaky tests")
  • Commit 0635aa4 (Merge with main)
  • Commit fe64724 ("revert playwright config")

This proves these are NOT flaky tests but consistent, deterministic failures requiring code fixes.

From previous analysis:

  • Line 936: Timeout waiting for schema element to become clickable (60s exceeded)
  • Line 1075: Strict mode violation - selector matched 11 elements instead of 1
  • Line 1134: Combined filter application issues (similar patterns)

Flaky Tests (8 total, all passed on retry):

  1. BulkEditEntity.spec.ts:99 - Database service
  2. BulkEditEntity.spec.ts:740 - Glossary Term (Nested)
  3. BulkImportWithDotInName.spec.ts:619 - Bulk edit with dot in service name
  4. ColumnBulkOperations.spec.ts:2022 - Selection Edge Cases
  5. DataProductRename.spec.ts:221 - Multiple consecutive renames
  6. IncidentManagerDateFilter.spec.ts:193 - Date filter persistence
  7. GlossaryAssets.spec.ts:313 - Remove glossary term tag
  8. GlossaryHierarchy.spec.ts:121 - Move term with children

Details

Key Observations:

  1. Full test suite restored: 561 tests (vs ~188 in reduced mode), confirming "revert playwright config" worked
  2. Normal execution time: 52.3 minutes is expected for this test count
  3. Acceptable flaky rate: 8 flaky tests (1.4%) is within normal range
  4. Unresolved core issues: The ColumnBulkOperations problems are test implementation bugs, not configuration issues

Pattern: The recent commits focused on configuration changes but did not address the actual test code issues in ColumnBulkOperations.spec.ts.

Code Review 👍 Approved with suggestions 9 resolved / 10 findings

Solid test refactoring PR improving reliability with API-based setup and better synchronization. The previous finding about potential undefined displayName remains partially unresolved - while lodash's get() is now used for safer access, no fallback value is provided.

💡 Edge Case: verifyPipelineDataInDrawer may fail if displayName undefined

📄 openmetadata-ui/src/main/resources/ui/playwright/utils/lineage.ts:434-443

In lineage.ts, the verifyPipelineDataInDrawer function now uses get() from lodash to safely access fromNode.entity.displayName and toNode.entity.displayName. However, if either value is undefined, the assertion toHaveText(fromNodeDisplayName) will fail with "Expected string, received undefined".

const fromNodeDisplayName = get(fromNode.entity, 'displayName');
const toNodeDisplayName = get(toNode.entity, 'displayName');
...
await expect(
  page.locator('.overview-section').getByTestId('Source-value')
).toHaveText(fromNodeDisplayName);  // Will fail if undefined

Consider providing a default value:

const fromNodeDisplayName = get(fromNode.entity, 'displayName', '');
const toNodeDisplayName = get(toNode.entity, 'displayName', '');

Or using optional chaining with nullish coalescing like other parts of the PR do (nodeFqn ?? '').

✅ 9 resolved
Bug: Missing await on visitEntityPage call

📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Flow/ServiceCreationPermissions.spec.ts:301
In ServiceCreationPermissions.spec.ts, the original code called userOwnedService.visitEntityPage(otherPage) without await. The fix correctly adds await, but this was a pre-existing bug that's being fixed in this PR.

The line at 301 shows the fix:

-    userOwnedService.visitEntityPage(otherPage)
+    await userOwnedService.visitEntityPage(otherPage);

This is actually a good fix and improves reliability.

Bug: Removed sqlQuery validation from custom property test

📄 openmetadata-ui/src/main/resources/ui/playwright/utils/customProperty.ts:1398 📄 openmetadata-ui/src/main/resources/ui/playwright/utils/customProperty.ts:1414
In customProperty.ts, the sqlQuery validation case was removed from updateCustomPropertyInRightPanel:

// Removed code:
} else if (propertyType === 'sqlQuery') {
  await waitForAllLoadersToDisappear(page);
  await expect(container.getByTestId('value')).toContainText(value);
}

Instead, 'sqlQuery' was added to the exclusion list:

if (
  ![
    'entityReference',
    'entityReferenceList',
    'date-cp',
    'dateTime-cp',
    'sqlQuery',  // <-- added here
  ].includes(propertyType)
)

This means sqlQuery custom properties will now silently pass validation without actually checking the value. If sqlQuery validation is intentionally being skipped (perhaps it requires special handling that's not needed), this should be documented. Otherwise, this could mask test failures for sqlQuery custom properties.

If sqlQuery doesn't require special validation, consider removing it from the exclusion list and letting it fall through to the default validation logic.

Bug: Missing await on async visitEntityPage call

📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Flow/ServiceCreationPermissions.spec.ts:285
In ServiceCreationPermissions.spec.ts, the original code was missing await before userOwnedService.visitEntityPage(otherPage). The PR correctly adds the await keyword:

// Before (broken):
userOwnedService.visitEntityPage(otherPage)

// After (fixed):
await userOwnedService.visitEntityPage(otherPage);

This is a legitimate bug fix - without the await, the test would continue immediately without waiting for the page navigation to complete, potentially causing flaky test failures or incorrect assertions.

This is a positive change that fixes an existing bug.

Bug: Filtering without assertion in verifyPipelineDataInDrawer

📄 openmetadata-ui/src/main/resources/ui/playwright/utils/lineage.ts:424 📄 openmetadata-ui/src/main/resources/ui/playwright/utils/lineage.ts:388
In lineage.ts, the original code had an issue where it was calling .filter() without any assertion:

// Before (broken):
await page
  .locator('.edge-info-drawer [data-testid="Edge"] a')
  .filter({ hasText: pipelineName });

// After (fixed):
await expect(
  page.getByTestId('Edge-value').filter({ hasText: pipelineName })
).toBeAttached();

The old code created a filtered locator but never asserted anything on it, making the check a no-op. The fix properly wraps it with expect().toBeAttached() to actually verify the element exists. This is a good bug fix that ensures the pipeline data is actually validated.

However, note that the selector also changed from .edge-info-drawer [data-testid="Edge"] a to [data-testid="Edge-value"]. Ensure this new selector correctly targets the same element in the UI.

Edge Case: Potential undefined access on displayName in drawer verification

📄 openmetadata-ui/src/main/resources/ui/playwright/utils/lineage.ts:431 📄 openmetadata-ui/src/main/resources/ui/playwright/utils/lineage.ts:432
In lineage.ts, the code now uses get() from lodash to safely access displayName:

const fromNodeDisplayName = get(fromNode.entity, 'displayName');
const toNodeDisplayName = get(toNode.entity, 'displayName');
// ...
).toHaveText(fromNodeDisplayName);

However, get() will return undefined if the property doesn't exist, and this undefined value is then passed to .toHaveText(). Playwright's toHaveText() expects a string. If displayName is missing, this could cause unexpected test failures or assertion errors.

Consider providing a default value:

const fromNodeDisplayName = get(fromNode.entity, 'displayName', '');

Or add validation that these values exist before the assertion.

...and 4 more resolved from earlier reviews

Tip

Comment Gitar fix CI or enable auto-apply: gitar auto-apply:on

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqubecloud
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

safe to test Add this label to run secure Github workflows on PRs UI UI specific issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants