Skip to content

[Unzyme] src/core/packages/apps#217599

Merged
TinaHeiligers merged 4 commits intoelastic:mainfrom
TinaHeiligers:unzyme_core_packages_apps
Apr 9, 2025
Merged

[Unzyme] src/core/packages/apps#217599
TinaHeiligers merged 4 commits intoelastic:mainfrom
TinaHeiligers:unzyme_core_packages_apps

Conversation

@TinaHeiligers
Copy link
Contributor

@TinaHeiligers TinaHeiligers commented Apr 8, 2025

Summary

Fix #217589
Related to #217387

Migrates metric_tiles and status_table unit tests from enzyme snapshot tests to explicit assertions using RTL. Explicit assertions have the advantage of being more readable, targeted, reducing noise due to unrelated changes (e.g. EUI updates) and to make debugging easier.

Checklist

@TinaHeiligers TinaHeiligers added Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// release_note:skip Skip the PR/issue when compiling release notes backport:all-open Backport to all branches that could still receive a release labels Apr 8, 2025
@TinaHeiligers TinaHeiligers requested a review from a team as a code owner April 8, 2025 23:49
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@TinaHeiligers TinaHeiligers added backport:prev-minor and removed backport:all-open Backport to all branches that could still receive a release labels Apr 8, 2025
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged


import React from 'react';
import { shallow } from 'enzyme';
import { mountWithIntl } from '@kbn/test-jest-helpers';
Copy link
Member

@afharo afharo Apr 9, 2025

Choose a reason for hiding this comment

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

This library uses enzyme under the hood. @Dosant, are we planning to migrate @kbn/test-jest-helpers to RTL?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure how this will look like, but we likely won't be able to internally migrate this helper without touching the actual test case code.
We should probably create a separate package for most common react testing library cases (if doesn't exist yet) and deprecate these functions.

For now, I'd either use react testing library directly or this wrapper

export const renderReactTestingLibraryWithI18n = (...args: Parameters<typeof render>) => {
const [ui, ...remainingRenderArgs] = args;
// Avoid using { wrapper: I18nProvider } in case the caller adds a custom wrapper.
return render(<I18nProvider>{ui}</I18nProvider>, ...remainingRenderArgs);
};

Copy link
Contributor

@Dosant Dosant Apr 9, 2025

Choose a reason for hiding this comment

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

@Bamieh, @TinaHeiligers,

LGTM we should be using @kbn/test-jest-helpers anyways since it has the custom centralized code written by the core team for testing i18n

I don't think we should.
I don't think we will be able to migrate mountWithIntl internally to use testing library without rewriting the test cases.

imo, mountWithIntl should be marked as deprecated just as any other enzyme api and another common util needs to be created and used instead ( or maybe just

export const renderReactTestingLibraryWithI18n = (...args: Parameters<typeof render>) => {
const [ui, ...remainingRenderArgs] = args;
// Avoid using { wrapper: I18nProvider } in case the caller adds a custom wrapper.
return render(<I18nProvider>{ui}</I18nProvider>, ...remainingRenderArgs);
};
, but we should name it nicer since it will be a default)

Copy link
Contributor

@Bamieh Bamieh left a comment

Choose a reason for hiding this comment

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

LGTM we should be using @kbn/test-jest-helpers anyways since it has the custom centralized code written by the core team for testing i18n

@TinaHeiligers TinaHeiligers merged commit 50ddab4 into elastic:main Apr 9, 2025
19 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 9.0

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

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Apr 9, 2025
## Summary

Fix elastic#217589
Related to elastic#217387

Migrates `metric_tiles` and `status_table` unit tests from `enzyme`
snapshot tests to explicit assertions using `RTL`. Explicit assertions
have the advantage of being more readable, targeted, reducing noise due
to unrelated changes (e.g. EUI updates) and to make debugging easier.

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

(cherry picked from commit 50ddab4)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
9.0

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

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Apr 9, 2025
# Backport

This will backport the following commits from `main` to `9.0`:
- [[Unzyme] src/core/packages/apps
(#217599)](#217599)

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

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

<!--BACKPORT [{"author":{"name":"Christiane (Tina)
Heiligers","email":"christiane.heiligers@elastic.co"},"sourceCommit":{"committedDate":"2025-04-09T14:57:29Z","message":"[Unzyme]
src/core/packages/apps (#217599)\n\n## Summary\n\nFix
https://github.com/elastic/kibana/issues/217589\nRelated to
#217387\n\nMigrates `metric_tiles` and `status_table` unit tests from
`enzyme`\nsnapshot tests to explicit assertions using `RTL`. Explicit
assertions\nhave the advantage of being more readable, targeted,
reducing noise due\nto unrelated changes (e.g. EUI updates) and to make
debugging easier.\n\n### Checklist\n\n- [x] [Unit or
functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere
updated or added to match the most common
scenarios","sha":"50ddab4418c7f53cdb5c3690e556dac79ab3c4eb","branchLabelMapping":{"^v9.1.0$":"main","^v8.19.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Core","release_note:skip","backport:prev-minor","v9.1.0"],"title":"[Unzyme]
src/core/packages/apps","number":217599,"url":"https://github.com/elastic/kibana/pull/217599","mergeCommit":{"message":"[Unzyme]
src/core/packages/apps (#217599)\n\n## Summary\n\nFix
https://github.com/elastic/kibana/issues/217589\nRelated to
#217387\n\nMigrates `metric_tiles` and `status_table` unit tests from
`enzyme`\nsnapshot tests to explicit assertions using `RTL`. Explicit
assertions\nhave the advantage of being more readable, targeted,
reducing noise due\nto unrelated changes (e.g. EUI updates) and to make
debugging easier.\n\n### Checklist\n\n- [x] [Unit or
functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere
updated or added to match the most common
scenarios","sha":"50ddab4418c7f53cdb5c3690e556dac79ab3c4eb"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/217599","number":217599,"mergeCommit":{"message":"[Unzyme]
src/core/packages/apps (#217599)\n\n## Summary\n\nFix
https://github.com/elastic/kibana/issues/217589\nRelated to
#217387\n\nMigrates `metric_tiles` and `status_table` unit tests from
`enzyme`\nsnapshot tests to explicit assertions using `RTL`. Explicit
assertions\nhave the advantage of being more readable, targeted,
reducing noise due\nto unrelated changes (e.g. EUI updates) and to make
debugging easier.\n\n### Checklist\n\n- [x] [Unit or
functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere
updated or added to match the most common
scenarios","sha":"50ddab4418c7f53cdb5c3690e556dac79ab3c4eb"}}]}]
BACKPORT-->

Co-authored-by: Christiane (Tina) Heiligers <christiane.heiligers@elastic.co>
@afharo afharo mentioned this pull request Apr 11, 2025
TinaHeiligers added a commit that referenced this pull request Apr 16, 2025
## Summary
fix #217589 again
The implementation in #217599 for
the status table still relied on enzyme indirectly by using
`mountWithI18n`.

This PR refactors the test from implementing the enzyme-reliant helper
to using `renderReactTestingLibraryWithI18n` that doesn't.

## Note to reviewers:
If `renderReactTestingLibraryWithI18n` becomes the standard, renaming it
to something shorter (e.g. `renderWithI18n`) would be nicer. ATM, the
helper's only used in 13 files and it would be better to do so now that
when adoption becomes wide-spread.

The package is owned by `shared-ux` and renaming the function would
require code-reviews from too many teams to justify doing so in this PR.

### Checklist

Check the PR satisfies following conditions. 

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Apr 16, 2025
## Summary
fix elastic#217589 again
The implementation in elastic#217599 for
the status table still relied on enzyme indirectly by using
`mountWithI18n`.

This PR refactors the test from implementing the enzyme-reliant helper
to using `renderReactTestingLibraryWithI18n` that doesn't.

## Note to reviewers:
If `renderReactTestingLibraryWithI18n` becomes the standard, renaming it
to something shorter (e.g. `renderWithI18n`) would be nicer. ATM, the
helper's only used in 13 files and it would be better to do so now that
when adoption becomes wide-spread.

The package is owned by `shared-ux` and renaming the function would
require code-reviews from too many teams to justify doing so in this PR.

### Checklist

Check the PR satisfies following conditions.

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
(cherry picked from commit c45f791)
akowalska622 pushed a commit to akowalska622/kibana that referenced this pull request Apr 17, 2025
## Summary
fix elastic#217589 again
The implementation in elastic#217599 for
the status table still relied on enzyme indirectly by using
`mountWithI18n`.

This PR refactors the test from implementing the enzyme-reliant helper
to using `renderReactTestingLibraryWithI18n` that doesn't.

## Note to reviewers:
If `renderReactTestingLibraryWithI18n` becomes the standard, renaming it
to something shorter (e.g. `renderWithI18n`) would be nicer. ATM, the
helper's only used in 13 files and it would be better to do so now that
when adoption becomes wide-spread.

The package is owned by `shared-ux` and renaming the function would
require code-reviews from too many teams to justify doing so in this PR.

### Checklist

Check the PR satisfies following conditions. 

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
jbudz pushed a commit that referenced this pull request Apr 17, 2025
…218491)

# Backport

This will backport the following commits from `main` to `9.0`:
- [[Unzyme] Migrates status_table of enzyme completely
(#218361)](#218361)

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

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

<!--BACKPORT [{"author":{"name":"Christiane (Tina)
Heiligers","email":"christiane.heiligers@elastic.co"},"sourceCommit":{"committedDate":"2025-04-16T20:23:28Z","message":"[Unzyme]
Migrates status_table of enzyme completely (#218361)\n\n## Summary\nfix
#217589 again\nThe
implementation in #217599 for\nthe
status table still relied on enzyme indirectly by
using\n`mountWithI18n`.\n\nThis PR refactors the test from implementing
the enzyme-reliant helper\nto using `renderReactTestingLibraryWithI18n`
that doesn't.\n\n## Note to reviewers:\nIf
`renderReactTestingLibraryWithI18n` becomes the standard, renaming
it\nto something shorter (e.g. `renderWithI18n`) would be nicer. ATM,
the\nhelper's only used in 13 files and it would be better to do so now
that\nwhen adoption becomes wide-spread.\n\nThe package is owned by
`shared-ux` and renaming the function would\nrequire code-reviews from
too many teams to justify doing so in this PR.\n\n### Checklist\n\nCheck
the PR satisfies following conditions. \n\n- [x] [Unit or
functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere
updated or added to match the most common
scenarios\n\n---------\n\nCo-authored-by: Elastic Machine
<elasticmachine@users.noreply.github.com>","sha":"c45f791ddb3917fb13d978fc53a3a440bf9505b5","branchLabelMapping":{"^v9.1.0$":"main","^v8.19.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","backport:prev-minor","v9.1.0"],"title":"[Unzyme]
Migrates status_table of enzyme
completely","number":218361,"url":"https://github.com/elastic/kibana/pull/218361","mergeCommit":{"message":"[Unzyme]
Migrates status_table of enzyme completely (#218361)\n\n## Summary\nfix
#217589 again\nThe
implementation in #217599 for\nthe
status table still relied on enzyme indirectly by
using\n`mountWithI18n`.\n\nThis PR refactors the test from implementing
the enzyme-reliant helper\nto using `renderReactTestingLibraryWithI18n`
that doesn't.\n\n## Note to reviewers:\nIf
`renderReactTestingLibraryWithI18n` becomes the standard, renaming
it\nto something shorter (e.g. `renderWithI18n`) would be nicer. ATM,
the\nhelper's only used in 13 files and it would be better to do so now
that\nwhen adoption becomes wide-spread.\n\nThe package is owned by
`shared-ux` and renaming the function would\nrequire code-reviews from
too many teams to justify doing so in this PR.\n\n### Checklist\n\nCheck
the PR satisfies following conditions. \n\n- [x] [Unit or
functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere
updated or added to match the most common
scenarios\n\n---------\n\nCo-authored-by: Elastic Machine
<elasticmachine@users.noreply.github.com>","sha":"c45f791ddb3917fb13d978fc53a3a440bf9505b5"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/218361","number":218361,"mergeCommit":{"message":"[Unzyme]
Migrates status_table of enzyme completely (#218361)\n\n## Summary\nfix
#217589 again\nThe
implementation in #217599 for\nthe
status table still relied on enzyme indirectly by
using\n`mountWithI18n`.\n\nThis PR refactors the test from implementing
the enzyme-reliant helper\nto using `renderReactTestingLibraryWithI18n`
that doesn't.\n\n## Note to reviewers:\nIf
`renderReactTestingLibraryWithI18n` becomes the standard, renaming
it\nto something shorter (e.g. `renderWithI18n`) would be nicer. ATM,
the\nhelper's only used in 13 files and it would be better to do so now
that\nwhen adoption becomes wide-spread.\n\nThe package is owned by
`shared-ux` and renaming the function would\nrequire code-reviews from
too many teams to justify doing so in this PR.\n\n### Checklist\n\nCheck
the PR satisfies following conditions. \n\n- [x] [Unit or
functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere
updated or added to match the most common
scenarios\n\n---------\n\nCo-authored-by: Elastic Machine
<elasticmachine@users.noreply.github.com>","sha":"c45f791ddb3917fb13d978fc53a3a440bf9505b5"}}]}]
BACKPORT-->

Co-authored-by: Christiane (Tina) Heiligers <christiane.heiligers@elastic.co>
akowalska622 pushed a commit to akowalska622/kibana that referenced this pull request May 29, 2025
## Summary
fix elastic#217589 again
The implementation in elastic#217599 for
the status table still relied on enzyme indirectly by using
`mountWithI18n`.

This PR refactors the test from implementing the enzyme-reliant helper
to using `renderReactTestingLibraryWithI18n` that doesn't.

## Note to reviewers:
If `renderReactTestingLibraryWithI18n` becomes the standard, renaming it
to something shorter (e.g. `renderWithI18n`) would be nicer. ATM, the
helper's only used in 13 files and it would be better to do so now that
when adoption becomes wide-spread.

The package is owned by `shared-ux` and renaming the function would
require code-reviews from too many teams to justify doing so in this PR.

### Checklist

Check the PR satisfies following conditions. 

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release_note:skip Skip the PR/issue when compiling release notes Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// v9.0.0 v9.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

src/core/packages/apps

7 participants