Adding Classification and Tags page missing tests #25270
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! |
|
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! |
karanh37
added
UI
Jest test CoverageUI tests summary
|
| await page.waitForTimeout(500); | ||
|
|
||
| const tenPerPageOption = page.getByRole('menuitem', { | ||
| name: '10 / Page', | ||
| }); |
There was a problem hiding this comment.
Avoid using arbitrary timeouts. According to the Playwright Developer Handbook, prefer waiting for specific API responses, element visibility, or use waitForAllLoadersToDisappear. Arbitrary timeouts make tests slower and more flaky. Consider waiting for the menu to be visible or for a specific element state instead.
| await page.waitForTimeout(500); | |
| const tenPerPageOption = page.getByRole('menuitem', { | |
| name: '10 / Page', | |
| }); | |
| const tenPerPageOption = page.getByRole('menuitem', { | |
| name: '10 / Page', | |
| }); | |
| await expect(tenPerPageOption).toBeVisible(); |
|
|
||
| // Test changing to 25 per page | ||
| await pageSizeDropdown.click(); | ||
| await page.waitForTimeout(500); |
There was a problem hiding this comment.
Avoid using arbitrary timeouts. Replace with a wait for the menu option to be visible or the dropdown to be fully rendered.
|
|
||
| // Test changing to 50 per page (if available) | ||
| await pageSizeDropdown.click(); | ||
| await page.waitForTimeout(500); |
There was a problem hiding this comment.
Avoid using arbitrary timeouts. Replace with a wait for the dropdown or specific element to be ready.
Code Review 👍 Approved with suggestions 0 resolved / 5 findingsGood test coverage expansion with several commented-out code blocks that should be cleaned up. More details 💡 5 suggestions💡 Quality: Commented-out code left in production📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Classification.spec.ts:675 There's a commented-out wait for response that should either be uncommented and used, or removed entirely if it's not needed: // await domainOptionToRemove.waitFor({ state: 'visible', timeout: 50000 });Having commented-out code in tests can be confusing for future maintainers - they won't know if it was intentionally removed or accidentally left behind. If the wait is not needed, remove the comment. If it's needed, uncomment it. 💡 Edge Case: Commented-out response wait may cause flaky tests📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Classification.spec.ts:825-833 In the 'Usage Count Accuracy' test, there's a commented-out // const removeTagResponse = page.waitForResponse(
// (response) =>
// response.url().includes('/api/v1/columns/name/') &&
// response.request().method() === 'PATCH'
// );
...
// await removeTagResponse;Without waiting for the API response, the test may proceed before the removal is complete, leading to flaky test results. Either uncomment and use the response wait, or verify the implementation works reliably without it. 💡 Quality: Commented-out assertion should be fixed or removed📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Classification.spec.ts:559-563 In the 'Enable/Disable Tag via UI Toggle' test, there's a commented-out assertion: // Verify that the "Add Asset" button is disabled when the tag is disabled
// await expect(
// page.getByTestId('data-assets-add-button')
// ).toBeDisabled();This assertion appears to be important for validating that disabled tags cannot have assets added. If the assertion was failing due to an incorrect test ID (note: the test uses 💡 Quality: Commented-out response wait may cause flaky test📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Classification.spec.ts:923-925 In the 'Mutually Exclusive Behavior' test, there's a commented-out // await page.waitForResponse(
// `/api/v1/search/query?q=*${encodeURIComponent(tagA.data.name)}*`
// );Without waiting for the search response, the test may try to click on the tag option before search results are rendered, potentially causing flaky failures. Consider uncommenting this or using an alternative wait mechanism. 💡 Quality: Large commented-out test should be removed or fixed📄 openmetadata-ui/src/main/resources/ui/playwright/e2e/Pages/Tags.spec.ts:1307 There's a large commented-out test block (~200 lines) for 'Tag Page Activity Feed' starting at line 1307 with the comment "Commented this since the test is failing". This is problematic because:
Recommendation: Either fix the test, remove it entirely, or track the issue in a bug ticket and skip the test with test.skip('Tag Page Activity Feed', async ({ page }) => {
// TODO: Skipped - see issue #XXXX
...
});What Works WellComprehensive test coverage for Classification and Tags pages including pagination, CRUD operations, mutually exclusive behavior, and edge cases like duplicate handling. Tests follow good patterns with proper setup/teardown and API response waits. RecommendationsConsider cleaning up commented-out code before merging. Use OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
|
3 participants
Describe your changes:
Fixes
I worked on adding some missing classification and tags page tests
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>Summary by Gitar
Classification.spec.tswith 7 comprehensive test scenarios covering pagination, tag editing, enable/disable, domain management, usage counts, mutually exclusive behavior, and duplicate handlingClassificationClass,TagClass,TableClass, andDomainsupport classes with API-based setup and UI validationThis will update automatically on new commits.