Skip to content

Conversation

@Hubert-Szczepanski-SAP
Copy link
Contributor

What this PR does / why we need it:

Why Edit functionality is not fully tested
Edit Resource tests are intentionally limited to basic initialization checks.

Multiple layers - The edit flow involves: openInAside, YamlSidePanel, YamlViewer, Monaco Editor, and onApply callback. Each layer requires complex setup and mocking.

Splitter Context complexity - Edit opens resources in a side panel managed by SplitterContext. This context cannot be easily mocked in Cypress component.

YAML editor (Monaco Editor) is a also complex to be mocked.

We verify the basics work, we should cover it in E2E

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Copy link
Contributor

@andreaskienle andreaskienle left a comment

Choose a reason for hiding this comment

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

A few remarks down below.

About the splitter: Have you tried wrapping the component in <SplitterLayout> in the mount function?

    cy.mount(
      <MemoryRouter>
        <SplitterProvider>
          <SplitterLayout>
            …
          </SplitterLayout>
        </SplitterProvider>
      </MemoryRouter>,

This way you can maybe open the YAML file in the aside panel and test the flow there.

If this is not possible, can we at least test the useHandleResourcePatch hook?

@andreaskienle andreaskienle requested a review from Copilot November 6, 2025 16:22
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 adds test coverage for the ManagedResources component by introducing dependency injection for hooks and creating comprehensive Cypress component tests. The changes enable easier testing of delete and edit operations by allowing mock implementations of hooks to be passed as props.

  • Refactored ManagedResources component to accept optional hook overrides via props
  • Added comprehensive Cypress component tests for delete and force delete operations
  • Enhanced ActionsMenu component with test identifiers and removed unused parameter

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
src/components/ControlPlane/ManagedResources.tsx Added dependency injection pattern for useApiResourceMutation and useHandleResourcePatch hooks to enable testing
src/components/ControlPlane/ManagedResources.cy.tsx Added comprehensive Cypress component tests covering delete, force delete, validation, and edit functionality
src/components/ControlPlane/ActionsMenu.tsx Added data-testid attribute and data-component-name for test identification, removed unused buttonIcon parameter from function signature

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

@andreaskienle andreaskienle left a comment

Choose a reason for hiding this comment

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

Much better without the cy.wait() calls! 👍🏻

@andreaskienle
Copy link
Contributor

Hey @lucasgoral, mind giving this a quick approve? 🙏
I committed a change suggested by Copilot, so my own approval doesn’t count 😭🤦‍♂️

@Hubert-Szczepanski-SAP Hubert-Szczepanski-SAP merged commit 7182a8c into main Nov 14, 2025
5 checks passed
@Hubert-Szczepanski-SAP Hubert-Szczepanski-SAP deleted the test/resources-delete-edit branch November 14, 2025 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants