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.71% (55116/83881) 44.68% (28382/63529) 47.64% (8661/18181)

@gitar-bot
Copy link

gitar-bot bot commented Jan 25, 2026

🔍 CI failure analysis for 91aa272: Multiple CI jobs failed: Job (3,6) has 3 ColumnBulkOperations hard failures + 4 flaky; Job (5,6) had infrastructure failure (runner shutdown after 2h) with 6 test timeouts before termination

Issue

Multiple CI jobs from the same commit (91aa272 "Fix flaky tests") failed with different patterns.

Job 61394948970: playwright-ci-postgresql (3, 6) - 181/188 passed (30.4 min)
Job 61394948966: playwright-ci-postgresql (5, 6) - Infrastructure failure (runner shutdown after 2h 3m)

Root Cause

Job (3,6) - ColumnBulkOperations Issues (3 hard failures):

  1. Line 936: Test timeout (60s) waiting for schema element to be clickable
  2. Line 1075: Strict mode violation - selector matched 11 elements
  3. Line 1134: Combined filters test (similar issues)

All concentrated in one test file, suggesting systemic issues with bulk operations feature.

Job (5,6) - Infrastructure Failure:

Primary Issue: Runner received shutdown signal at 10:56:11 UTC after running for 2h 3m

  • Exit Code: 143 (SIGTERM - graceful termination)
  • Cause: Job exceeded GitHub Actions 2-hour timeout or runner resource exhaustion
  • State: 186 tests had passed when terminated

Test Failures Before Shutdown (6 tests):

  1. GlossaryWorkflow.spec.ts:593 - 48.6s timeout viewing workflow history
  2. ImpactAnalysis.spec.ts:377 - 60s timeout (upstream/downstream counts)
  3. ImpactAnalysis.spec.ts:580 - 60s timeout (entity popover card)
  4. DomainDataProductsWidgets.spec.ts:116 - 60s timeout (domain asset count update)
  5. FollowingWidget.spec.ts:77 - 0ms immediate failure (likely beforeAll/beforeEach hook)
  6. Permission.spec.ts:148 - 0ms immediate failure (passed on retry)

Flaky Tests in Job (3,6):

  • CustomizeDetailPage, DataProductRename, IncidentManagerDateFilter, RulesLibrary (all passed on retry)

Details

Critical Infrastructure Issue:
Job (5,6) running for over 2 hours is not normal. Typical Playwright test suites complete in 30-40 minutes. This indicates:

  • Severe performance degradation
  • Tests taking much longer than expected
  • Likely hitting GitHub Actions 2-hour job timeout limit

Common Patterns:

  1. ImpactAnalysis tests failing across multiple shards (lines 377, 580)
  2. Multiple 60-second timeouts suggest slow test execution
  3. Immediate failures (0ms) indicate hook failures, not test logic issues
  4. The "Fix flaky tests" commit has NOT resolved issues

Key Observation: Both jobs show different failure modes, suggesting widespread test stability and performance problems rather than isolated issues.

Code Review 👍 Approved with suggestions 8 resolved / 10 findings

Good test reliability improvements with API-based setup, proper async waits, and code clarity refactors. One minor edge case with undefined displayName handling.

💡 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.

💡 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 ?? '').

✅ 8 resolved
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.

Bug: Missing await on visitEntityPage call was fixed

📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Flow/ServiceCreationPermissions.spec.ts:318
The code change at line 318 fixes a previous bug where await was missing on userOwnedService.visitEntityPage(otherPage). The fixed version properly awaits the promise:\n\ntypescript\n// Before (broken):\nuserOwnedService.visitEntityPage(otherPage)\n\n// After (fixed):\nawait userOwnedService.visitEntityPage(otherPage);\n\n\nThis is actually a positive fix in this PR - it correctly awaits the async operation.

...and 3 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