You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Please run make py_format and py_format_check in the root of your repository and commit the changes to this PR.
You can also use pre-commit to automate the Python code formatting.
You can install the pre-commit hooks with make install_test precommit_install.
Flaky test fixes addressed multiple issues including retry logic restoration and dead code removal, but three findings remain: parseInt may access the wrong field in DataAssetsCoveragePieChartWidget, entityIconMapping lost EntityType keys during refactoring, and AuthProvider login polling silently fails after 5 seconds.
⚠️Bug: entityIconMapping lost many EntityType keys during refactoring
The entityIconMapping was moved from inside getEntityIcon() to module scope and de-duplicated. However, many EntityType.* keys were removed (e.g., EntityType.DATABASE, EntityType.TABLE, EntityType.TOPIC, EntityType.DASHBOARD, EntityType.PIPELINE, EntityType.GLOSSARY, EntityType.GLOSSARY_TERM, EntityType.CHART, etc.) keeping only the SearchIndex.* variant. Similarly EntityType.TABLE_COLUMN (string 'tableColumn') was replaced by SearchIndex.COLUMN (string 'column') — these are different strings. Any caller of getEntityIcon passing an EntityType value that differs from its SearchIndex counterpart will get a missing icon. While many EntityType/SearchIndex pairs share the same string value, TABLE_COLUMN vs COLUMN is a confirmed mismatch, and MLMODEL_SERVICE casing may differ between enums.
Suggested fix
Restore the `EntityType.TABLE_COLUMN` entry alongside `SearchIndex.COLUMN`, and verify all removed EntityType keys have identical string values to their SearchIndex counterparts. For any mismatches, keep both entries:
```ts
[SearchIndex.COLUMN]: ColumnIcon,
[EntityType.TABLE_COLUMN]: ColumnIcon,
```
💡 Bug: parseInt on fullyQualifiedName field looks like wrong field access
In DataAssetsCoveragePieChartWidget, line 95 parses totalData[0].fullyQualifiedName as an integer, and line 94 parses coverageData[0].originEntityFQN as an integer. These field names typically hold string FQN values, not numeric counts. If the search aggregation API returns numeric values in these fields by convention, this code works but is fragile and confusing. If the API response format changes or different fields should be used, this would silently produce NaN.
The new polling mechanism in onLoginHandler retries up to 100 times (5 seconds) waiting for authenticatorRef.current to be available. If it never becomes available, the only feedback is setApplicationLoading(false) — the user sees the loading spinner disappear but gets no error message or redirect, leaving them on a broken login page with no indication of what happened.
Suggested fix
Show an error toast or redirect when max attempts are exhausted:
```ts
} else {
setApplicationLoading(false);
showErrorToast('Authentication service unavailable. Please refresh and try again.');
}
```
✅ 9 resolved✅ Bug: fetchIncidentId may return undefined, causing PATCH to wrong URL
📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/DataQuality/TestCaseIncidentPermissions.spec.ts:330-338
At line 330, fetchIncidentId is called but its return value is not checked. If the incident hasn't been created yet (async timing), incidentId remains undefined and the PATCH at line 332 will hit /api/v1/dataQuality/testCases/testCaseIncidentStatus/undefined — which likely returns 404, not the expected 403. This defeats the purpose of the permission test.
Notably, the other test at line 362-363 correctly guards with if (incidentId), but this test does not. The old code guaranteed incidentId was populated via polling in beforeAll; the new lazy approach needs a guard or assertion.
✅ Quality: Commented-out line should be removed, not left as dead code
📄 openmetadata-ui/src/main/resources/ui/playwright/utils/importUtils.ts:202
Line 202 in importUtils.ts has a commented-out click action (// await page.click('[data-testid="tag-selector"]');). If the click is no longer needed, the line should be removed entirely rather than left as a comment.
✅ Bug: visitUserProfilePage lost retry logic for search indexing lag
📄 openmetadata-ui/src/main/resources/ui/playwright/utils/user.ts:118-130
The original visitUserProfilePage used expect.poll() with a 60-second timeout and backoff intervals to handle cases where the search index hasn't caught up yet for newly created users. The new code (lines 118-130) makes a single search request and immediately asserts visibility with the default timeout (~5s). If the user was recently created and the search index hasn't been updated yet, the first search may return zero results and the test will fail immediately instead of retrying.
For a PR aimed at reducing flakiness, this removes a resilience mechanism that was specifically designed to handle eventual consistency in the search index.
✅ Quality: Inconsistent indentation in waitForResponse callback
📄 openmetadata-ui/src/main/resources/ui/playwright/utils/user.ts:118-122📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/AuditLogs.spec.ts:312-322
The waitForResponse callback body at lines 119-121 in user.ts is indented at the top level (no indentation relative to the arrow function), breaking the surrounding code's indentation convention. Similarly, lines 312-322 in AuditLogs.spec.ts have the callback body indented at the wrong level.
✅ Bug: Retry block with Enter key press may toggle popup open/close
📄 openmetadata-ui/src/main/resources/ui/playwright/utils/importUtils.ts:772-784
The expect().toPass() retry block at line 776-784 calls page.keyboard.press('Enter') on each retry attempt. If the first Enter press opens the certification dropdown but the visibility check fails (e.g., due to slow API response), the next retry's Enter press will close the already-open dropdown, making the radio button invisible again. This creates an alternating open/close pattern that will only succeed on odd-numbered attempts.
A more robust approach is to check whether the dropdown is already open before pressing Enter, or to re-click the active cell before each retry to reset state.
...and 4 more resolved from earlier reviews
🤖 Prompt for agents
Code Review: Flaky test fixes addressed multiple issues including retry logic restoration and dead code removal, but three findings remain: parseInt may access the wrong field in DataAssetsCoveragePieChartWidget, entityIconMapping lost EntityType keys during refactoring, and AuthProvider login polling silently fails after 5 seconds.
1. 💡 Bug: parseInt on fullyQualifiedName field looks like wrong field access
In `DataAssetsCoveragePieChartWidget`, line 95 parses `totalData[0].fullyQualifiedName` as an integer, and line 94 parses `coverageData[0].originEntityFQN` as an integer. These field names typically hold string FQN values, not numeric counts. If the search aggregation API returns numeric values in these fields by convention, this code works but is fragile and confusing. If the API response format changes or different fields should be used, this would silently produce NaN.
2. ⚠️ Bug: entityIconMapping lost many EntityType keys during refactoring
The `entityIconMapping` was moved from inside `getEntityIcon()` to module scope and de-duplicated. However, many `EntityType.*` keys were removed (e.g., `EntityType.DATABASE`, `EntityType.TABLE`, `EntityType.TOPIC`, `EntityType.DASHBOARD`, `EntityType.PIPELINE`, `EntityType.GLOSSARY`, `EntityType.GLOSSARY_TERM`, `EntityType.CHART`, etc.) keeping only the `SearchIndex.*` variant. Similarly `EntityType.TABLE_COLUMN` (string `'tableColumn'`) was replaced by `SearchIndex.COLUMN` (string `'column'`) — these are different strings. Any caller of `getEntityIcon` passing an EntityType value that differs from its SearchIndex counterpart will get a missing icon. While many EntityType/SearchIndex pairs share the same string value, `TABLE_COLUMN` vs `COLUMN` is a confirmed mismatch, and `MLMODEL_SERVICE` casing may differ between enums.
Suggested fix:
Restore the `EntityType.TABLE_COLUMN` entry alongside `SearchIndex.COLUMN`, and verify all removed EntityType keys have identical string values to their SearchIndex counterparts. For any mismatches, keep both entries:
```ts
[SearchIndex.COLUMN]: ColumnIcon,
[EntityType.TABLE_COLUMN]: ColumnIcon,
```
3. 💡 Edge Case: AuthProvider login polling silently fails after 5s timeout
The new polling mechanism in `onLoginHandler` retries up to 100 times (5 seconds) waiting for `authenticatorRef.current` to be available. If it never becomes available, the only feedback is `setApplicationLoading(false)` — the user sees the loading spinner disappear but gets no error message or redirect, leaving them on a broken login page with no indication of what happened.
Suggested fix:
Show an error toast or redirect when max attempts are exhausted:
```ts
} else {
setApplicationLoading(false);
showErrorToast('Authentication service unavailable. Please refresh and try again.');
}
```
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
safe to testAdd this label to run secure Github workflows on PRs
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
kshinde3110
added
the
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
Describe your changes:
Fixes
I worked on ... because ...
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>Summary by Gitar
reducedMotion: 'reduce'context option to reduce test flakinesswaitForAllLoadersToDisappear()helper in user search flowThis will update automatically on new commits.