Skip to content

Conversation

@sanketpathak
Copy link
Contributor

Description

https://issues.redhat.com/browse/RHIDP-9375

Please explain the changes you made here.

Which issue(s) does this PR fix

  • Fixes #?

PR acceptance criteria

Please make sure that the following steps are complete:

  • GitHub Actions are completed and successful
  • Unit Tests are updated and passing
  • E2E Tests are updated and passing
  • Documentation is updated if necessary (requirement for new features)
  • Add a screenshot if the change is UX/UI related

How to test changes / Special notes to the reviewer

@rhdh-qodo-merge
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

RHIDP-9375 - Partially compliant

Compliant requirements:

  • Update/add RHDH e2e tests related to Bulk Import to reflect the latest Bulk Import UI changes

Non-compliant requirements:

Requires further human verification:

  • Bulk import tests pass in RHDH for the latest UI changes in Bulk Import (verify by running/observing CI e2e execution)
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🔒 No security concerns identified
⚡ Recommended focus areas for review

Selector brittleness

The test relies on exact visible text and specific accessible names (e.g., the accordion label and tooltip name). Minor copy changes or i18n adjustments can break the test. Consider preferring stable selectors (dedicated data-testids) for critical UI elements like the accordion toggle, the "Source control tool" control, and the tooltip trigger.

test('should render bulk import page with source control options, search, and repositories table', async () => {
  await uiHelper.openSidebar("Bulk import");
  await uiHelper.verifyHeading('Bulk import');
  await expect(page.getByText('Import to Red Hat Developer')).toHaveAttribute('aria-expanded', 'true');
  await page.getByRole('button', { name: 'Import to Red Hat Developer' }).click();
  await expect(page.getByText('Import to Red Hat Developer')).toHaveAttribute('aria-expanded', 'false');
  await page.getByRole('button', { name: 'Import to Red Hat Developer' }).click();
  await page.getByText('Source control tool', {exact: true}).click();
  await page.getByTestId('HelpOutlineIcon').hover();
  await expect(page.getByRole('tooltip', {name: 'Importing requires approval.'})).toBeVisible();
  await expect(page.getByRole('radio', { name: 'GitHub' })).toBeChecked();
  await page.getByRole('radio', { name: 'GitLab' }).check();
  await expect(page.getByRole('radio', { name: 'GitLab' })).toBeChecked();
  await page.getByRole('radio', { name: 'GitHub' }).check();
Tooltip assertion fragility

The tooltip verification assumes the tooltip has role tooltip and an accessible name exactly equal to the tooltip text. Depending on the tooltip implementation, it may not expose a name as expected or may render with timing/animation delays. Consider asserting visibility of the tooltip text via a more resilient locator and/or waiting for the tooltip container rather than relying on the role name matching.

await page.getByTestId('HelpOutlineIcon').hover();
await expect(page.getByRole('tooltip', {name: 'Importing requires approval.'})).toBeVisible();
await expect(page.getByRole('radio', { name: 'GitHub' })).toBeChecked();
Aria snapshot maintenance

The toMatchAriaSnapshot table assertion is likely to be high-maintenance because small accessibility tree changes (column header wording, additional columns, wrapping elements) will fail the test. Consider narrowing the assertion to key headers/roles (e.g., presence of table + specific column headers) instead of a broad snapshot, unless snapshot churn is expected and acceptable.

  await expect(page.getByRole('article')).toMatchAriaSnapshot(`
    - table:
      - rowgroup:
        - row "select all repositories Name URL Organization Status":
          - columnheader "select all repositories Name":
            - checkbox "select all repositories"
            - text: Name
          - columnheader "URL"
          - columnheader "Organization"
          - columnheader "Status"
  `);
});
📄 References
  1. No matching references available

@rhdh-qodo-merge
Copy link

PR Type

Tests


Description

  • Add comprehensive e2e test for bulk import UI page rendering

  • Verify source control options, expandable sections, and radio buttons

  • Validate repositories table structure with aria snapshot testing

  • Test tooltip visibility and user interactions with UI elements


File Walkthrough

Relevant files
Tests
bulk-import.spec.ts
Add bulk import UI rendering and interaction e2e test       

e2e-tests/playwright/e2e/plugins/bulk-import.spec.ts

  • Added new test case validating bulk import page rendering with source
    control options
  • Verifies expandable "Import to Red Hat Developer" section
    functionality
  • Tests source control tool selection (GitHub/GitLab radio buttons) and
    tooltip display
  • Validates repositories table structure using aria snapshot matching
+27/-0   

@rhdh-qodo-merge
Copy link

rhdh-qodo-merge bot commented Jan 21, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Use role-based locator

Replace page.getByText('Source control tool', {exact: true}) with the more
robust page.getByRole('button', { name: 'Source control tool' }).

e2e-tests/playwright/e2e/plugins/bulk-import.spec.ts [90]

-await page.getByText('Source control tool', {exact: true}).click();
+await page.getByRole('button', { name: 'Source control tool' }).click();
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: This suggestion correctly improves the test's robustness and maintainability by replacing a text-based selector with a more semantic and resilient role-based selector.

Medium
High-level
New test lacks end-to-end validation

The new test only checks the UI of the bulk import page. It should be expanded
to cover the full user workflow, including searching for, selecting, and
importing a repository to validate the core functionality.

Examples:

e2e-tests/playwright/e2e/plugins/bulk-import.spec.ts [83-108]
  test('should render bulk import page with source control options, search, and repositories table', async () => {
    await uiHelper.openSidebar("Bulk import");
    await uiHelper.verifyHeading('Bulk import');
    await expect(page.getByText('Import to Red Hat Developer')).toHaveAttribute('aria-expanded', 'true');
    await page.getByRole('button', { name: 'Import to Red Hat Developer' }).click();
    await expect(page.getByText('Import to Red Hat Developer')).toHaveAttribute('aria-expanded', 'false');
    await page.getByRole('button', { name: 'Import to Red Hat Developer' }).click();
    await page.getByText('Source control tool', {exact: true}).click();
    await page.getByTestId('HelpOutlineIcon').hover();
    await expect(page.getByRole('tooltip', {name: 'Importing requires approval.'})).toBeVisible();

 ... (clipped 16 lines)

Solution Walkthrough:

Before:

test('should render bulk import page...', async () => {
  // Navigate to bulk import page
  await uiHelper.openSidebar("Bulk import");
  await uiHelper.verifyHeading('Bulk import');

  // Check basic UI controls like radio buttons and accordions
  await page.getByRole('button', { name: 'Import to Red Hat Developer' }).click();
  await page.getByRole('radio', { name: 'GitLab' }).check();
  await page.getByRole('radio', { name: 'GitHub' }).check();

  // Verify table structure with a snapshot
  await expect(page.getByRole('article')).toMatchAriaSnapshot(...);
});

After:

test('should perform end-to-end bulk import...', async () => {
  // Navigate to bulk import page
  await uiHelper.openSidebar("Bulk import");
  await uiHelper.verifyHeading('Bulk import');

  // Search for a repository
  await page.getByPlaceholder('Search...').fill('my-repo');

  // Select a repository from the results
  await page.getByRole('row', { name: 'my-repo' }).getByRole('checkbox').check();

  // Initiate the import process
  await page.getByRole('button', { name: 'Import (1)' }).click();

  // Verify the import status or outcome
  await expect(page.getByText('Import successful')).toBeVisible();
});
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that the new test only validates UI rendering and not the core bulk import functionality, which is a significant limitation in an end-to-end test suite.

Low
Possible issue
Ensure table loaded

Add await page.waitForSelector('article table'); before taking the ARIA snapshot
to ensure the table is fully loaded and prevent flaky test failures.

e2e-tests/playwright/e2e/plugins/bulk-import.spec.ts [97-107]

+await page.waitForSelector('article table');
 await expect(page.getByRole('article')).toMatchAriaSnapshot(`
   - table:
     - rowgroup:
       - row "select all repositories Name URL Organization Status":
         - columnheader "select all repositories Name":
           - checkbox "select all repositories"
           - text: Name
         - columnheader "URL"
         - columnheader "Organization"
         - columnheader "Status"
 `);
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion adds an explicit wait for the table to be present before taking a snapshot, which is a good practice to prevent test flakiness, even if Playwright's auto-waiting might handle it.

Low
Wait after toggling expand

Add an explicit wait to confirm the aria-expanded attribute is true after
clicking the "Import to Red Hat Developer" button to prevent potential race
conditions.

e2e-tests/playwright/e2e/plugins/bulk-import.spec.ts [89]

 await page.getByRole('button', { name: 'Import to Red Hat Developer' }).click();
+await expect(page.getByRole('button', { name: 'Import to Red Hat Developer' })).toHaveAttribute('aria-expanded', 'true');
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies a potential race condition and adds an explicit wait, which improves the test's reliability, although Playwright's auto-waiting might already handle this.

Low
  • Update

@sanketpathak sanketpathak force-pushed the e2e-update-bulk-import-ui-rhdh-9375 branch from 12a0a90 to 915b5f0 Compare January 21, 2026 12:25
@sanketpathak sanketpathak force-pushed the e2e-update-bulk-import-ui-rhdh-9375 branch from 915b5f0 to d257be2 Compare January 21, 2026 12:49
@sanketpathak sanketpathak changed the title feat(bulk-import): Update e2e tests in RHDH for bulk import UI changes chore(e2e): Update e2e tests in RHDH for bulk import UI changes Jan 21, 2026
@sanketpathak
Copy link
Contributor Author

/retest

@github-actions
Copy link
Contributor

The image is available at:

/test e2e-ocp-helm

@sanketpathak sanketpathak force-pushed the e2e-update-bulk-import-ui-rhdh-9375 branch from d257be2 to 504b16f Compare January 21, 2026 15:29
@openshift-ci
Copy link

openshift-ci bot commented Jan 21, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign psrna for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@github-actions
Copy link
Contributor

🚫 Image Push Skipped.

The container image push was skipped because the build was skipped (either due to [skip-build] tag or no relevant changes with existing image)

@sonarqubecloud
Copy link

@sanketpathak sanketpathak changed the title chore(e2e): Update e2e tests in RHDH for bulk import UI changes chore(bulk-import): update e2e tests in RHDH for bulk import UI changes Jan 21, 2026
@sanketpathak
Copy link
Contributor Author

/retest

@openshift-ci
Copy link

openshift-ci bot commented Jan 21, 2026

@sanketpathak: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-ocp-helm 504b16f link true /test e2e-ocp-helm

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant