Skip to content

[ML] Migrate Enzyme tests to RTL part 2 #226205

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 46 commits into from
Jul 9, 2025

Conversation

rbrtj
Copy link
Contributor

@rbrtj rbrtj commented Jul 2, 2025

The previous PR - #225423, targeted only direct imports from enzyme. This PR targets all occurrences of helper functions that use Enzyme under the hood.

@peteharverson peteharverson requested review from darnautov and removed request for jgowdyelastic July 4, 2025 09:19
const wrapper = shallowWithIntl(<AnnotationDescriptionList annotation={mockAnnotations[0]} />);
expect(wrapper).toMatchSnapshot();
const { container } = renderWithI18n(
<AnnotationDescriptionList annotation={mockAnnotations[0] as unknown as Annotation} />
Copy link
Contributor

Choose a reason for hiding this comment

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

did you try satisfies Annotation for the mock?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can get rid of as unknown but satisfies is not working properly.
95b9635

};

// Mock withKibana HOC
jest.mock('@kbn/kibana-react-plugin/public', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I found several createWithKibanaMock in the codebase. I reckon it'd be convenient to create a reusable helper in kibana_react

instance.closeFlyout();
test6.wrapper.update();
expect(test6.wrapper).toMatchSnapshot();
test(`don't render when not opened`, () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to keep tests when it's rendered?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To test it we'd need to access component instance through ref. It uses function registration pattern rather than accepting props (isOpen)..
I believe it isn't a good way to test it.
example:

 const componentRef = React.createRef();
  
  const { container } = render(
    <RuleEditorFlyout 
      {...getRequiredProps()} 
      ref={componentRef}
    />
  );
  
  act(() => {
    componentRef.current.showFlyout({
      jobId: 'test-job',
      detectorIndex: 0,
      source: { function: 'mean' }
    });
  });

wrapper.find('EuiLink').simulate('click');
wrapper.update();
expect(addItemToFilterList).toHaveBeenCalled();
fireEvent.click(getByRole('button', { name: 'Add elastic.co to safe_domains' }));
Copy link
Contributor

Choose a reason for hiding this comment

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

shall we rely on the test id instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in d77271e

const { container } = renderWithI18n(<DeleteRuleModal {...requiredProps} />);

// Find and click the delete link
const deleteLink = screen.getByRole('button', { name: 'Delete rule' });
Copy link
Contributor

Choose a reason for hiding this comment

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

did you consider using a data-test-subj for the buttons?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in d77271e

Comment on lines 13 to 15
jest.mock('../../../../capabilities/check_capabilities', () => ({
usePermissionCheck: () => [true, true],
}));
Copy link
Contributor

Choose a reason for hiding this comment

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

consider using reusable mock from

export const usePermissionCheck = jest.fn((arg: string | string[]) => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 82c857c

usePermissionCheck: () => [true, true],
}));

jest.mock('../../../../contexts/kibana/kibana_context', () => ({
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The mock wasn't actually needed, removed in 82c857c

const addButton = wrapper.find('EuiButton').first();
expect(addButton.prop('disabled')).toBe(true);
// Find the Add button by its role
const addButton = getByRole('button', { name: 'Add' });
Copy link
Contributor

Choose a reason for hiding this comment

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

it's be safer to use test ids

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in d77271e

Copy link
Contributor

@darnautov darnautov left a comment

Choose a reason for hiding this comment

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

LGTM, left some suggestions

@rbrtj rbrtj requested review from a team as code owners July 8, 2025 08:46
@csr csr self-requested a review July 8, 2025 15:30
Copy link
Contributor

@csr csr left a comment

Choose a reason for hiding this comment

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

AppEx QA owned changes LGTM

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
ml 5.4MB 5.4MB +163.0B

History

cc @rbrtj

@rbrtj rbrtj merged commit 293bb28 into elastic:main Jul 9, 2025
12 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.19, 9.1

https://github.com/elastic/kibana/actions/runs/16163097447

kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Jul 9, 2025
The previous PR - elastic#225423,
targeted only direct imports from `enzyme`. This PR targets all
occurrences of helper functions that use Enzyme under the hood.

---------

Co-authored-by: kibanamachine <[email protected]>
(cherry picked from commit 293bb28)
@kibanamachine
Copy link
Contributor

💔 Some backports could not be created

Status Branch Result
8.19 Backport failed because of merge conflicts
9.1

Note: Successful backport PRs will be merged automatically after passing CI.

Manual backport

To create the backport manually run:

node scripts/backport --pr 226205

Questions ?

Please refer to the Backport tool documentation

@rbrtj
Copy link
Contributor Author

rbrtj commented Jul 9, 2025

💚 All backports created successfully

Status Branch Result
8.19

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

rbrtj added a commit to rbrtj/kibana that referenced this pull request Jul 9, 2025
The previous PR - elastic#225423,
targeted only direct imports from `enzyme`. This PR targets all
occurrences of helper functions that use Enzyme under the hood.

---------

Co-authored-by: kibanamachine <[email protected]>
(cherry picked from commit 293bb28)

# Conflicts:
#	x-pack/platform/plugins/shared/ml/public/application/settings/settings.test.tsx
kibanamachine added a commit that referenced this pull request Jul 9, 2025
# Backport

This will backport the following commits from `main` to `9.1`:
- [[ML] Migrate `Enzyme` tests to `RTL` part 2
(#226205)](#226205)

<!--- Backport version: 9.6.6 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sorenlouv/backport)

<!--BACKPORT [{"author":{"name":"Robert
Jaszczurek","email":"[email protected]"},"sourceCommit":{"committedDate":"2025-07-09T07:28:22Z","message":"[ML]
Migrate `Enzyme` tests to `RTL` part 2 (#226205)\n\nThe previous PR -
https://github.com/elastic/kibana/pull/225423,\ntargeted only direct
imports from `enzyme`. This PR targets all\noccurrences of helper
functions that use Enzyme under the
hood.\n\n---------\n\nCo-authored-by: kibanamachine
<[email protected]>","sha":"293bb28818f87bc9793e4ecbf9ecc56cfb00990c","branchLabelMapping":{"^v9.2.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":[":ml","release_note:skip","Team:ML","backport:version","v9.1.0","v8.19.0","v9.2.0"],"title":"[ML]
Migrate `Enzyme` tests to `RTL` part
2","number":226205,"url":"https://github.com/elastic/kibana/pull/226205","mergeCommit":{"message":"[ML]
Migrate `Enzyme` tests to `RTL` part 2 (#226205)\n\nThe previous PR -
https://github.com/elastic/kibana/pull/225423,\ntargeted only direct
imports from `enzyme`. This PR targets all\noccurrences of helper
functions that use Enzyme under the
hood.\n\n---------\n\nCo-authored-by: kibanamachine
<[email protected]>","sha":"293bb28818f87bc9793e4ecbf9ecc56cfb00990c"}},"sourceBranch":"main","suggestedTargetBranches":["9.1","8.19"],"targetPullRequestStates":[{"branch":"9.1","label":"v9.1.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.19","label":"v8.19.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v9.2.0","branchLabelMappingKey":"^v9.2.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/226205","number":226205,"mergeCommit":{"message":"[ML]
Migrate `Enzyme` tests to `RTL` part 2 (#226205)\n\nThe previous PR -
https://github.com/elastic/kibana/pull/225423,\ntargeted only direct
imports from `enzyme`. This PR targets all\noccurrences of helper
functions that use Enzyme under the
hood.\n\n---------\n\nCo-authored-by: kibanamachine
<[email protected]>","sha":"293bb28818f87bc9793e4ecbf9ecc56cfb00990c"}}]}]
BACKPORT-->

Co-authored-by: Robert Jaszczurek <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:version Backport to applied version labels :ml release_note:skip Skip the PR/issue when compiling release notes Team:ML Team label for ML (also use :ml) v8.19.0 v9.1.0 v9.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants