Added test for resource details section in object explorer #2218
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive end-to-end tests for the resource details section in the Object Explorer feature. The tests validate various aspects of the details panel functionality including tab navigation, YAML/JSON editing, and logs viewing.
Key Changes:
- Added 10 test cases covering resource details panel, summary/edit/logs tabs, and YAML/JSON editing capabilities
- Implemented helper function
checkResourceDetailsAvailable()to verify feature availability - Applied MSW scenarios for mocking resource data in tests
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }) | ||
| ) { | ||
| await summaryTab.click(); | ||
| await page.waitForTimeout(500); |
There was a problem hiding this comment.
Multiple arbitrary timeouts throughout tests: Lines 60, 146, 182, 228, 286, 330, 357, 412, 426, 453, 512, and 525 all use page.waitForTimeout() with arbitrary durations. These introduce unnecessary delays and make tests slower. Consider replacing with event-driven waits using Playwright's built-in methods like waitForLoadState(), waitForSelector(), or element state checks.
| await page.waitForTimeout(500); | |
| // Wait for either resource name or namespace to be visible after clicking summary tab | |
| await Promise.race([ | |
| page.locator('text=/name|Name/i').first().waitFor({ state: 'visible', timeout: 2000 }), | |
| page.locator('text=/namespace|Namespace/i').first().waitFor({ state: 'visible', timeout: 2000 }) | |
| ]); |
| await page.waitForTimeout(1000); | ||
|
|
||
| const errorIndicator = page | ||
| .locator('.error, .invalid, [class*="error"], [class*="invalid"]') |
There was a problem hiding this comment.
Overly broad error indicator selectors: The selector .locator('.error, .invalid, [class*="error"], [class*="invalid"]') is too generic and could match many unrelated elements. This could lead to false positives. Consider using more specific selectors or test IDs for error indicators.
| .locator('.error, .invalid, [class*="error"], [class*="invalid"]') | |
| .locator('[data-testid="yaml-error-indicator"]') |
| const yamlContent = | ||
| (await yamlEditor.textContent().catch(error => { | ||
| console.error('Error getting YAML content:', error); | ||
| return ''; | ||
| })) || | ||
| (await yamlEditor.inputValue().catch(error => { | ||
| console.error('Error getting YAML input value:', error); | ||
| return ''; | ||
| })) || | ||
| ''; |
There was a problem hiding this comment.
Complex nested logic for retrieving YAML content: This pattern of trying multiple methods to get content (textContent(), inputValue()) with fallbacks is repeated on lines 237-246, 339-348, 360-368, 435-444, 456-464. Consider extracting this into a helper function like getEditorContent(editor) to reduce duplication and improve maintainability.
| })) || | ||
| ''; | ||
|
|
||
| if (yamlContent && yamlContent.length > 10) { |
There was a problem hiding this comment.
Magic number without explanation: The value 10 is used to check if content length is sufficient, but there's no explanation why 10 characters is the threshold. Consider extracting this as a named constant like MIN_YAML_CONTENT_LENGTH with a comment explaining the rationale.
| if (originalContent && originalContent.length > 10) { | ||
| try { | ||
| await yamlEditor.click(); | ||
| await page.keyboard.press('Control+A'); |
There was a problem hiding this comment.
Keyboard shortcut may not work cross-platform: Using Control+A (line 353, 449, 523) won't work on macOS where Meta+A (Command+A) is the standard. Consider using Playwright's cross-platform keyboard API or detecting the platform to use the appropriate modifier key.
| await page.keyboard.press('Control+A'); | |
| // Cross-platform "Select All" shortcut | |
| const isMac = await page.evaluate(() => navigator.platform.includes('Mac')); | |
| await page.keyboard.press(isMac ? 'Meta+A' : 'Control+A'); |
| await page.keyboard.type( | ||
| 'apiVersion: v1\nkind: Pod\nmetadata:\n name: test-pod\n namespace: default' | ||
| ); |
There was a problem hiding this comment.
Hard-coded test data in multiple locations: YAML/JSON test content is hard-coded inline at lines 354-356, 450-452, and 524. Consider extracting these as constants at the top of the file or in a separate fixtures file for better maintainability and reusability.
|
|
||
| test('should open resource details panel when clicking on resource card', async ({ page }) => { | ||
| await objectExplorerPage.changeViewMode('grid'); | ||
| await page.waitForTimeout(500); |
There was a problem hiding this comment.
Hard-coded arbitrary timeout: Using page.waitForTimeout(500) introduces unnecessary delays and makes tests slower. Consider using Playwright's built-in waiting mechanisms like waitForLoadState() or waiting for specific elements to be ready instead of arbitrary waits.
| await page.waitForTimeout(500); | |
| // Wait for the first resource card to be visible after changing view mode | |
| await page.locator('.MuiCard-root').first().waitFor({ state: 'visible' }); |
|
@alokdangre Good work mate |
|
@alokdangre reduce time .. same |
|
This pull request has been automatically marked as stale because it has not had recent activity. |
3 participants
Description
Added e2e test for resource details section in object explorer
Related Issue
Fixes #<issue_number>
Changes Made
Checklist
Please ensure the following before submitting your PR:
Screenshots or Logs (if applicable)
Additional Notes