-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Fix: Charts not visible in explore tree panel #25487
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
Co-authored-by: aniketkatkar97 <51777795+aniketkatkar97@users.noreply.github.com>
Co-authored-by: aniketkatkar97 <51777795+aniketkatkar97@users.noreply.github.com>
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
1 similar comment
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
|
@copilot, the added playwright test case does not verify the chart being shown in the explore tree in the UI. Add necessary assertions to verify the charts are displayed in the tree. |
Co-authored-by: aniketkatkar97 <51777795+aniketkatkar97@users.noreply.github.com>
| }); | ||
|
|
||
| test('Verify charts are visible in explore tree', async ({ page }) => { | ||
| await page.waitForLoadState('networkidle'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed both networkidle waits. The test now relies on explicit API response waits and Playwright's built-in auto-waiting.
Co-authored-by: aniketkatkar97 <51777795+aniketkatkar97@users.noreply.github.com>
Fixed the test failure by adding a page reload at the start of the test. The issue was that after the beforeEach creates the chart entity via API, the explore page needs to be refreshed to reflect the newly created data in the UI. The test now:
Commit: 6308f36 |
🔍 CI failure analysis for 6308f36: Multiple Playwright test failures detected across two CI jobs. The failures appear to be unrelated to the PR changes, which only add Charts to the explore tree UI.IssueTwo Playwright CI jobs failed with a total of 4 test failures and 22 flaky tests: Job 61355160554 (playwright-ci-postgresql 2/6):
Job 61355160562 (playwright-ci-postgresql 3/6):
Previously reported: The Root CauseThe test failures appear to be unrelated to the PR changes. Analysis shows:
DetailsFailed Test Examples:ColumnBulkOperations.spec.ts failures: ServiceCreationPermissions.spec.ts failure: These errors suggest timing issues and race conditions in test infrastructure rather than functional bugs introduced by the PR. AssessmentThe PR changes focus on adding Charts as a visible entity type in the explore tree UI. The failing tests cover completely different functionality (bulk operations, service permissions, landing page widgets, pagination, etc.) that should not be impacted by simply adding Charts to the explore tree hierarchy. The high number of flaky tests (22 total) across both jobs further supports the conclusion that these are infrastructure/timing-related test issues rather than code defects. Code Review 👍 Approved with suggestions 2 resolved / 3 findingsBug fix PR correctly enables Charts in explore tree with appropriate test coverage. The previous finding about using getByTestId instead of chained locators remains unresolved in the new test code. 💡 Quality: Use `getByTestId` instead of chained locator filters📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/ExploreTree.spec.ts:456-461 🔗 Playwright Developer Handbook - Locator Priority Order The test uses a complex locator pattern that could be simplified and made more resilient: await page
.locator('div')
.filter({ hasText: /^Dashboards$/ })
.locator('svg')
.first()
.click();According to the Playwright Developer Handbook's Locator Priority Order, Suggested fix: await page.getByTestId('explore-tree-expand-Dashboards').click();Or if the tree node has a proper test ID structure, use that consistently with await page.getByRole('button', { name: 'Expand Dashboards' }).click();✅ 2 resolved✅ Quality: Avoid using `networkidle` waitForLoadState in tests
✅ Bug: Test only validates API response, not UI rendering of Charts
Tip Comment OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
|



Describe your changes:
Charts with 487 assets were searchable but invisible in the left-side explore tree. Three issues prevented proper rendering:
notIncludeAggregationExploreTree()filtered charts from aggregation buckets, Dashboard'schildEntitiesarray lacked CHART type, andgetPluralizeEntityName()was missing the CHART plural label mapping.Changes:
EntityType.CHARTfromnotIncludeAggregationExploreTree()exclusion listEntityType.CHARTto Dashboard'schildEntitiesarrayEntityType.CHARTmapping togetPluralizeEntityName()so tree displays "Charts" (plural) instead of "Chart" (singular)Test Coverage:
The Playwright test validates the complete user interaction flow following the Playwright Developer Handbook guidelines:
toBeVisible()assertion (no manual timeouts)Bug Fixes:
Charts now render under Dashboards in explore tree with the correct plural label and appear in API aggregation responses.
Type of change:
Checklist:
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.