Skip to content

Conversation

@dhruvjsx
Copy link
Contributor

@dhruvjsx dhruvjsx commented Feb 1, 2026

Describe your changes:

Fixes
Screenshot 2026-02-01 at 7 07 30 PM

I worked on ... because ...

Type of change:

  • Bug fix
  • Improvement
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.

Summary by Gitar

  • Fixed race conditions:
    • Added loader detachment waits and dropdown visibility checks before clicking manage-button in BulkImport.spec.ts
  • Replaced arbitrary timeouts:
    • Changed file upload wait from fixed 500ms to deterministic upload-file-widget hidden state
  • Navigation stabilization:
    • Added networkidle wait and scoped locator in DatabaseSchemaClass.visitEntityPage() to prevent clicking breadcrumb elements

This will update automatically on new commits.


@github-actions
Copy link
Contributor

github-actions bot commented Feb 1, 2026

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 65%
65.9% (55873/84787) 44.98% (28849/64143) 47.93% (8811/18384)

@gitar-bot
Copy link

gitar-bot bot commented Feb 5, 2026

🔍 CI failure analysis for ba69bee: 1 failure (unrelated) and 11 flaky tests. EXCELLENT: No BulkImport tests failed or flaky after removing timeout - validates both loader wait and timeout removals were appropriate.

Issue

CI run after removing await page.waitForTimeout(500) from importUtils.ts shows:

  • 1 failed test (unrelated to PR)
  • 11 flaky tests (passed on retry)
  • 625 tests passed

Root Cause

Failed Test (1 - Unrelated to PR)

Test: NotificationAlerts.spec.ts:245:1 - "Multiple Filters Alert"

This test file was NOT modified by this PR.

Flaky Tests (11 - None in BulkImport!)

EXCELLENT VALIDATION: NO BulkImport.spec.ts tests in the flaky list

Flaky tests (all unrelated):

  • ImpactAnalysis.spec.ts
  • GlossaryPermissions.spec.ts
  • ServiceEntityPermissions.spec.ts
  • QueryEntity.spec.ts
  • CustomPropertyAdvanceSeach.spec.ts
  • CustomizeWidgets.spec.ts
  • ExploreDiscovery.spec.ts
  • ServiceForm.spec.ts
  • AuditLogs.spec.ts
  • Customproperties-part1.spec.ts (2 tests)

Details

Impact of removing timeout:

Latest commit (ba69bee) removed the timeout from importUtils.ts:

// Line 118 - REMOVED:
await page.waitForTimeout(500);

This timeout was previously added to fix race conditions in BulkImport tests.

Result after removal:

  • Zero BulkImport tests failed
  • Zero BulkImport tests flaky
  • All BulkImport tests passed
  • No recurrence of race conditions

Validation through CI progression:

Change BulkImport Status Pass Rate
With all fixes Stable Good
Removed loader waits Stable 622 passed
Removed timeout Stable 625 passed

Why the removals work:

The remaining deterministic waits are sufficient:

  1. Dropdown visibility waits after manage button click
  2. API response waits using waitForResponse()
  3. Network idle waits in navigation
  4. Upload widget state waits
  5. Scoped locators preventing ambiguity

These eliminate the need for arbitrary timeouts or redundant loader waits.

Conclusion

Both removals (loader waits and timeout) are validated as appropriate. BulkImport tests are completely stable with excellent coverage. The core test stabilization improvements remain effective without the redundant waits.

Code Review ✅ Approved 1 resolved / 1 findings

Well-structured test stability improvements using proper waits for selectors, API responses, and load states instead of arbitrary timeouts. The changes follow Playwright best practices for eliminating flaky tests.

✅ 1 resolved
Bug: Missing await on page.waitForTimeout()

📄 openmetadata-ui/src/main/resources/ui/playwright/utils/importUtils.ts:87
The page.waitForTimeout(500) call on line 87 is missing the await keyword. Without await, this timeout has no effect - the code immediately proceeds to click the Users tab without waiting, defeating the purpose of adding this stabilization delay.

Impact: The test will not actually wait for the 500ms timeout before proceeding. This could lead to the same flaky behavior the PR is trying to fix.

Suggested fix:

//wait for small duration to stabilize the page
await page.waitForTimeout(500);

Note: While replacing waitForTimeout with explicit condition-based waits (like waitForSelector) is generally preferred (as done elsewhere in this PR), if a timeout is intentionally used here, it must be awaited to function correctly.

Rules 🎸 1 action taken

Gitar Rules

🎸 Flaky Test Retry: Triggered second retry of failed CI jobs due to flaky test patterns (retry 2/2)

2 rules not applicable. Show all rules by commenting gitar display:verbose.

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

sonarqubecloud bot commented Feb 5, 2026

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant