Skip to content

Conversation

@shah-harshit
Copy link
Contributor

@shah-harshit shah-harshit commented Jan 8, 2026

Describe your changes:

Fixes #23398

Screenshot 2026-01-08 at 3 37 04 PM

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

  • UI layout restructure:
    • Moved description component from page level into both Test Cases and Pipeline tabs with wrapInCard styling for consistent card presentation
    • Extracted renderDescription() function to eliminate duplication across tabs
  • Metadata header enhancement:
    • Created bordered container test-suite-header-metadata with DomainLabel using headerLayout prop and OwnerLabel with expanded display
  • Styling improvements:
    • Added test-suite-content-container class with borders and padding for tab content areas
    • Implemented responsive Grid layout with Row/Col components and 20px vertical gutter spacing
  • Code quality improvements:
    • Wrapped onDescriptionUpdate in useCallback with proper dependencies to prevent stale closures
    • Corrected React hook dependency arrays in callbacks and tabs useMemo

This will update automatically on new commits.


@gitar-bot
Copy link

gitar-bot bot commented Jan 8, 2026

🔍 CI failure analysis for 12846cc: Two playwright CI jobs failed - one is infrastructure-related, but one is directly related to this PR's changes to the TestSuite page.

Issue

Two separate playwright jobs have failed:

  1. Job 59783740856 (playwright-ci-postgresql (3, 6)): Infrastructure-related failure unrelated to PR changes
  2. Job 59783740913 (playwright-ci-postgresql (6, 6)): Test failure directly related to PR changes

Root Cause for Job 59783740913 (Related to PR)

The test TestSuite.spec.ts:52 - "Logical TestSuite › Domain Add, Update and Remove" is failing because the PR changed how the domain placeholder is displayed on the TestSuite details page.

Error:

Error: expect(locator).toContainText(expected) failed

Locator: getByTestId('no-domain-text')
Expected substring: "No Domains"
Received string:    "--"

What changed:
This PR added showDashPlaceholder prop to the DomainLabel component in TestSuiteDetailsPage.component.tsx (line 562):

<DomainLabel
  headerLayout
  showDashPlaceholder  // <-- This was added
  domains={testSuite?.domains}
  ...
/>

When showDashPlaceholder={true}, the component displays "--" instead of "No Domains" when no domain is set.

Details

Test location: playwright/e2e/Pages/TestSuite.spec.ts:109

Test helper: playwright/utils/common.ts:386-388

await expect(page.getByTestId('no-domain-text')).toContainText(
  showDashPlaceholder ? '--' : 'No Domains'
);

The test utility removeSingleSelectDomain expects the placeholder text to be conditional based on the showDashPlaceholder parameter, but the test is currently hardcoded to expect "No Domains".

Fix

The solution is to update the test in TestSuite.spec.ts to pass showDashPlaceholder: true when calling the removeSingleSelectDomain helper function, since the TestSuite page now uses this prop.

Alternatively, the test helper at common.ts:386-388 already has conditional logic - the test just needs to pass the correct parameter value.


Root Cause for Job 59783740856 (Unrelated)

The ServiceForm.spec.ts test failed with an ENOENT error attempting to access a certificate file with an invalid filename. This is an infrastructure issue with:

  • Page timeout errors
  • Browser context closures
  • "Failed to get access token" during setup
  • "ModuleNotFoundError: No module named 'cachetools'"

This job's failure is unrelated to the PR's UI changes to TestSuiteDetailsPage.

Code Review ✅ Approved

Clean UI refactor that properly addresses previous findings. The description component extraction and dependency array corrections are implemented correctly.

Resolved ✅ 2 resolved
Code Quality: Duplicated DescriptionV1 component block across tabs

📄 openmetadata-ui/src/main/resources/ui/src/pages/TestSuiteDetailsPage/TestSuiteDetailsPage.component.tsx:419-438
The DescriptionV1 component with identical props is duplicated in both the "Test Cases" tab (lines 419-427) and the "Pipeline" tab (lines 452-460). This creates maintenance overhead where any change to the description component must be made in two places.

Consider extracting a shared component or using a render function:

const renderDescription = () => (
  <Col span={24}>
    <DescriptionV1
      wrapInCard
      description={testSuiteDescription}
      entityType={EntityType.TEST_SUITE}
      hasEditAccess={permissions.hasEditDescriptionPermission}
      showCommentsIcon={false}
      onDescriptionUpdate={onDescriptionUpdate}
    />
  </Col>
);

Then use {renderDescription()} in both tabs.

Bug: Removed `testOwners` from dependency array - verify correctness

📄 openmetadata-ui/src/main/resources/ui/src/pages/TestSuiteDetailsPage/TestSuiteDetailsPage.component.tsx:297-298
The testOwners variable was removed from the onDescriptionEdit callback's dependency array. If testOwners is used within the callback and can change independently, this could cause stale closure issues where the callback uses an outdated value.

Please verify that:

  1. testOwners is not actually used in the callback body, OR
  2. testOwners is derived from testSuite and therefore already covered by that dependency

If the PR summary mentions this as a "Code quality fix" for hook dependencies, it would be helpful to confirm the callback doesn't reference testOwners directly.

What Works Well

Good use of renderDescription() helper function to avoid JSX duplication. Proper useCallback wrapping of onDescriptionUpdate with correct dependencies. Clean separation of styling concerns with new LESS classes.

Tip

Comment Gitar fix CI or enable auto-apply: gitar auto-apply:on

Options

Auto-apply is off Gitar will not commit updates to this branch.
Display: compact Hiding non-applicable rules.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | This comment will update automatically (Docs)

@github-actions
Copy link
Contributor

github-actions bot commented Jan 8, 2026

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 64%
64.66% (51611/79813) 42.27% (25244/59724) 45.85% (7957/17354)

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 8, 2026

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR revamps the test suite details page UI by restructuring the layout to improve consistency and visual organization. The description component is moved from page level into both the "Test Cases" and "Pipeline" tabs, metadata display is enhanced with a bordered container, and React hook dependencies are corrected.

Key Changes:

  • Moved description component into tab content with card-style wrapping for consistent presentation
  • Enhanced metadata header with bordered container styling and improved owner/domain labels
  • Fixed React hook dependency arrays by removing stale testOwners reference and wrapping onDescriptionUpdate in useCallback

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
TestSuiteDetailsPage.component.tsx Restructured component layout to move description into tabs, wrapped onDescriptionUpdate in useCallback, fixed dependency arrays for onUpdateOwner and handleDomainUpdate, and added responsive Grid layout with gutter spacing
test-suite-details-page.styles.less Removed unused .test-suite-description styles, added .test-suite-header-metadata for bordered metadata container, added .test-suite-details-tabs for tab overflow handling, and added .test-suite-content-container for tab content styling with borders and padding

}
};
},
[testSuite]
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The useCallback dependency array is missing the t function from useTranslation. The t function is used inside the callback (line 314) and should be included in the dependency array to comply with the exhaustive-deps ESLint rule. While the t function from react-i18next is typically stable across renders, it's a best practice to include all dependencies for consistency and to avoid potential stale closure issues.

Suggested change
[testSuite]
[testSuite, t]

Copilot uses AI. Check for mistakes.
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 UI UI specific issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Revamp Test Suite details page

2 participants