Skip to content

fallback for datacontract status check#27665

Open
shrabantipaul-collate wants to merge 4 commits intomainfrom
data-contract-status-check-failure
Open

fallback for datacontract status check#27665
shrabantipaul-collate wants to merge 4 commits intomainfrom
data-contract-status-check-failure

Conversation

@shrabantipaul-collate
Copy link
Copy Markdown
Contributor

@shrabantipaul-collate shrabantipaul-collate commented Apr 23, 2026

Describe your changes:

DataQuality fallback for quality validation timeout

The test suite executes and has results, but the contract's latestResult is sometimes not propagated back in time, causing waitForDataContractExecution to time out at 10 minutes.

Added waitForContractExecutionWithFallback in playwright/utils/dataContracts.ts: if the contract poll times out, it falls back to the DataQuality → Bundle Suites page, finds the Data Contract - test suite, clicks into it, and verifies the test case statuses directly from the API response — asserting a terminal status (Success/Failed/Aborted) to confirm execution completed.
Fixes

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

  • Refactoring:
    • Switched multiple test occurrences from user.responseData.displayName to user.getUserDisplayName() for consistent user data retrieval.
  • Test Infrastructure:
    • Increased test.setTimeout from 12 minutes to 15 minutes (900_000ms) in DataContracts.spec.ts to accommodate extended execution times.

This will update automatically on new commits.

@shrabantipaul-collate shrabantipaul-collate requested a review from a team as a code owner April 23, 2026 11:03
@shrabantipaul-collate shrabantipaul-collate added the safe to test Add this label to run secure Github workflows on PRs label Apr 23, 2026
…nnotations

Co-authored-by: shrabantipaul-collate <253027805+shrabantipaul-collate@users.noreply.github.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 23, 2026

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 61%
61.98% (61732/99597) 42.09% (32990/78372) 45.14% (9763/21626)

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 23, 2026

🔴 Playwright Results — 4 failure(s), 21 flaky

✅ 3941 passed · ❌ 4 failed · 🟡 21 flaky · ⏭️ 86 skipped

Shard Passed Failed Flaky Skipped
🟡 Shard 1 297 0 1 4
🟡 Shard 2 749 0 10 8
🟡 Shard 3 729 0 3 7
🟡 Shard 4 750 0 2 18
✅ Shard 5 687 0 0 41
🔴 Shard 6 729 4 5 8

Genuine Failures (failed on all attempts)

Pages/Glossary.spec.ts › Glossary & terms creation for reviewer as user (shard 6)
Error: To get the last run execution status as success

�[2mexpect(�[22m�[31mreceived�[39m�[2m).�[22mtoContain�[2m(�[22m�[32mexpected�[39m�[2m) // indexOf�[22m

Expected value: �[32m"PW.b7e0b5cd%Koalac38bb932"�[39m
Received array: �[31m[]�[39m

Call Log:
- Test timeout of 180000ms exceeded
Pages/Glossary.spec.ts › Glossary & terms creation for reviewer as team (shard 6)
Error: To get the last run execution status as success

�[2mexpect(�[22m�[31mreceived�[39m�[2m).�[22mtoContain�[2m(�[22m�[32mexpected�[39m�[2m) // indexOf�[22m

Expected value: �[32m"PW.142e829e%Leopard380203cf"�[39m
Received array: �[31m[]�[39m

Call Log:
- Test timeout of 180000ms exceeded
Pages/Glossary.spec.ts › Approve and reject glossary term from Glossary Listing (shard 6)
Error: To get the last run execution status as success

�[2mexpect(�[22m�[31mreceived�[39m�[2m).�[22mtoContain�[2m(�[22m�[32mexpected�[39m�[2m) // indexOf�[22m

Expected value: �[32m"PW.36952701%Lion075b7798"�[39m
Received array: �[31m[]�[39m

Call Log:
- Test timeout of 180000ms exceeded
Pages/Users.spec.ts › Check permissions for Data Steward (shard 6)
ReferenceError: getApiContext is not defined
🟡 21 flaky test(s) (passed on retry)
  • Pages/UserCreationWithPersona.spec.ts › Create user with persona and verify on profile (shard 1, 1 retry)
  • Features/ActivityAPI.spec.ts › Activity event is created when description is updated (shard 2, 1 retry)
  • Features/ActivityAPI.spec.ts › Activity event shows the actor who made the change (shard 2, 1 retry)
  • Features/BulkEditEntity.spec.ts › Database (shard 2, 1 retry)
  • Features/BulkEditEntity.spec.ts › Glossary (shard 2, 1 retry)
  • Features/BulkImport.spec.ts › Keyboard Delete selection (shard 2, 1 retry)
  • Features/DataQuality/ColumnLevelTests.spec.ts › Column Value Min To Be Between (shard 2, 1 retry)
  • Features/DomainFilterQueryFilter.spec.ts › Quick filters should persist when domain filter is applied and cleared (shard 2, 2 retries)
  • Features/ExploreQuickFilters.spec.ts › tier with assigned asset appears in dropdown, tier without asset does not (shard 2, 1 retry)
  • Features/Glossary/GlossaryHierarchy.spec.ts › should move nested term to root level of same glossary (shard 2, 1 retry)
  • Features/Glossary/GlossaryHierarchy.spec.ts › should move term with children to different glossary (shard 2, 1 retry)
  • Features/RTL.spec.ts › Verify Following widget functionality (shard 3, 1 retry)
  • Features/TableSorting.spec.ts › should have sorting on name column (shard 3, 1 retry)
  • Features/TableSorting.spec.ts › should have sorting on name column (shard 3, 1 retry)
  • Pages/CustomProperties.spec.ts › Should clear search and show all properties for apiCollection in right panel (shard 4, 1 retry)
  • Pages/DataContracts.spec.ts › Create Data Contract and validate for Pipeline (shard 4, 1 retry)
  • Pages/Glossary.spec.ts › Add and Remove Assets (shard 6, 1 retry)
  • Pages/InputOutputPorts.spec.ts › Lineage section collapse/expand (shard 6, 1 retry)
  • Pages/Lineage/DataAssetLineage.spec.ts › Column lineage for dashboardDataModel -> mlModel (shard 6, 1 retry)
  • Pages/Lineage/LineageFilters.spec.ts › Verify Impact Analysis service filter selection (shard 6, 1 retry)
  • Pages/Lineage/LineageRightPanel.spec.ts › Verify custom properties tab IS visible for supported type: searchIndex (shard 6, 1 retry)

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

@gitar-bot
Copy link
Copy Markdown

gitar-bot Bot commented Apr 23, 2026

Code Review ⚠️ Changes requested 1 resolved / 3 findings

Adds a datacontract status check fallback while resolving a duplicate type annotation. The implementation currently ignores non-timeout errors in the catch block and incorrectly retains a 'Running' status when test cases lack results.

⚠️ Edge Case: Catch-all in fallback swallows non-timeout errors silently

📄 openmetadata-ui/src/main/resources/ui/playwright/utils/dataContracts.ts:148-155

The catch block in waitForContractExecutionWithFallback catches all errors from waitForDataContractExecution, not just timeouts. That function can also throw when maxConsecutiveErrors is exceeded (indicating persistent API failures). By catching everything and falling back to the DQ page, a real infrastructure or API issue could be masked, making the test pass when it shouldn't.

Consider narrowing the catch to timeout-related errors only, or at minimum logging the caught error so flaky-test investigations have context.

Suggested fix
} catch (error) {
  // Only fall back for timeout-related errors; re-throw unexpected failures.
  if (
    !(error instanceof Error) ||
    !error.message.includes('Timeout')
  ) {
    throw error;
  }
  // The test suite has results but the contract's latestResult was not updated in time.
💡 Edge Case: suiteStatus stays 'Running' if all test cases lack results

📄 openmetadata-ui/src/main/resources/ui/playwright/utils/dataContracts.ts:187-199

If testCases is non-empty but none have a testCaseResult with a terminal status, suiteStatus remains 'Running', which doesn't match the terminalStatusPattern regex — causing the assertion to fail. This is probably the desired behavior (execution not complete), but it's worth confirming this edge case won't produce a confusing error message. A more descriptive assertion message would help debugging.

Suggested fix
expect(suiteStatus).toEqual(
  expect.stringMatching(terminalStatusPattern)
);
// Consider adding a message:
// expect(suiteStatus, `Expected terminal status but got '${suiteStatus}' for contract '${contractName}'`).toMatch(terminalStatusPattern);
✅ 1 resolved
Quality: Duplicated type annotation for test case status check

📄 openmetadata-ui/src/main/resources/ui/playwright/utils/dataContracts.ts:174-185
The inline type { testCaseResult?: { testCaseStatus?: string } } is repeated three times (lines 175, 179, 183). Extract it into a local type alias to reduce duplication and improve readability.

🤖 Prompt for agents
Code Review: Adds a datacontract status check fallback while resolving a duplicate type annotation. The implementation currently ignores non-timeout errors in the catch block and incorrectly retains a 'Running' status when test cases lack results.

1. ⚠️ Edge Case: Catch-all in fallback swallows non-timeout errors silently
   Files: openmetadata-ui/src/main/resources/ui/playwright/utils/dataContracts.ts:148-155

   The `catch` block in `waitForContractExecutionWithFallback` catches all errors from `waitForDataContractExecution`, not just timeouts. That function can also throw when `maxConsecutiveErrors` is exceeded (indicating persistent API failures). By catching everything and falling back to the DQ page, a real infrastructure or API issue could be masked, making the test pass when it shouldn't.
   
   Consider narrowing the catch to timeout-related errors only, or at minimum logging the caught error so flaky-test investigations have context.

   Suggested fix:
   } catch (error) {
     // Only fall back for timeout-related errors; re-throw unexpected failures.
     if (
       !(error instanceof Error) ||
       !error.message.includes('Timeout')
     ) {
       throw error;
     }
     // The test suite has results but the contract's latestResult was not updated in time.

2. 💡 Edge Case: suiteStatus stays 'Running' if all test cases lack results
   Files: openmetadata-ui/src/main/resources/ui/playwright/utils/dataContracts.ts:187-199

   If `testCases` is non-empty but none have a `testCaseResult` with a terminal status, `suiteStatus` remains `'Running'`, which doesn't match the `terminalStatusPattern` regex — causing the assertion to fail. This is probably the desired behavior (execution not complete), but it's worth confirming this edge case won't produce a confusing error message. A more descriptive assertion message would help debugging.

   Suggested fix:
   expect(suiteStatus).toEqual(
     expect.stringMatching(terminalStatusPattern)
   );
   // Consider adding a message:
   // expect(suiteStatus, `Expected terminal status but got '${suiteStatus}' for contract '${contractName}'`).toMatch(terminalStatusPattern);

Options

Display: compact → Showing less information.

Comment with these commands to change:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqubecloud
Copy link
Copy Markdown

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.

2 participants