Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[ML] Update jest test for FieldTypeIcon #152019

Merged
merged 8 commits into from
Mar 2, 2023
Merged

[ML] Update jest test for FieldTypeIcon #152019

merged 8 commits into from
Mar 2, 2023

Conversation

qn895
Copy link
Member

@qn895 qn895 commented Feb 23, 2023 •
edited by kibanamachine
Loading

Summary

Part of #147170. This PR updates the failing test for FieldTypeIcon, which was previously broken because it tested for number of children instead of the content. This PR fixes so that it:

  • Checks that tooltip is indeed not added if tooltipEnabled == false.
  • Checks that tooltip is visible and shows the correct label if tooltipEnabled == true.

Checklist

@qn895 qn895 added :ml release_note:skip Skip the PR/issue when compiling release notes Feature:File and Index Data Viz ML file and index data visualizer v8.7.0 v8.8.0 labels Feb 23, 2023
@qn895 qn895 requested a review from a team as a code owner February 23, 2023 18:33
@qn895 qn895 self-assigned this Feb 23, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@qn895 qn895 mentioned this pull request Feb 23, 2023
Closed
14 tasks
peteharverson
peteharverson approved these changes Feb 24, 2023
View reviewed changes
Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

LGTM

walterra
walterra reviewed Feb 24, 2023
View reviewed changes
Copy link
Contributor

@walterra walterra left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Great to see the migration from enzyme to testing-lib. Maybe we can also use this opportunity and get rid of the whole snapshot and instead just make an assertion about some content we'd expect to show up like the tooltip content.

Nit: The test function names are now not consistent, the new ones use it, the existing one with the snapshot uses test.

@qn895
Copy link
Member Author

qn895 commented Feb 24, 2023

@walterra Thanks for the suggestion. Made the updates here 3ac062f (#152019).

@walterra
Copy link
Contributor

walterra commented Mar 1, 2023

The tests now query the EUI classes .euiToken and .euiToolTipPopover. They are implementation details of the components we are using from other code and react-testing-lib best practice recommends avoiding querying those inner classes. Please try if it's possible to replace that with data-test-subj attributes in our own code. Ideally the testing code should only rely on our code so if EUI decides to change inner workings of a component without changing the API surface the tests shouldn't break.

@qn895 qn895 requested a review from a team as a code owner March 1, 2023 17:03
davismcphee
davismcphee reviewed Mar 1, 2023
View reviewed changes
packages/kbn-react-field/src/field_icon/field_icon.tsx Outdated
@@ -94,11 +94,11 @@ export function FieldIcon({
return (
<EuiToken
{...token}
className={classNames('kbnFieldIcon', className)}
Copy link
Contributor

Choose a reason for hiding this comment

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

The Data Discovery team was notified due to changes to this file, but it doesn't look like anything changed here other than the order of props. Is this intentional or just leftover from temporary changes?

Copy link
Member Author

@qn895 qn895 Mar 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@davismcphee You're right, the original change that triggered ownership review was reverted. Apologies for the ping.

walterra
walterra approved these changes Mar 2, 2023
View reviewed changes
Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

This looks great now, thanks for updating it, LGTM!

@qn895
Copy link
Member Author

qn895 commented Mar 2, 2023

@elasticmachine merge upstream

@qn895 qn895 enabled auto-merge (squash) March 2, 2023 15:12
@kibana-ci
Copy link
Collaborator

💚 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
dataVisualizer 366.0KB 366.1KB +78.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 428 430 +2

Total ESLint disabled count

id before after diff
securitySolution 506 508 +2

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @qn895

@qn895 qn895 merged commit c2161fe into elastic:main Mar 2, 2023
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Mar 2, 2023
@qn895
[ML] Update jest test for FieldTypeIcon (elastic#152019)
6208b27 
(cherry picked from commit c2161fe)
@kibanamachine kibanamachine mentioned this pull request Mar 2, 2023
Merged
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.7

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 Mar 2, 2023
@kibanamachine @qn895
[8.7] [ML] Update jest test for FieldTypeIcon (#152019) (#152594)
c55a8cd 
# Backport

This will backport the following commits from `main` to `8.7`:
- [[ML] Update jest test for FieldTypeIcon
(#152019)](#152019)

<!--- Backport version: 8.9.7 -->

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

<!--BACKPORT [{"author":{"name":"Quynh Nguyen
(Quinn)","email":"43350163+qn895@users.noreply.github.com"},"sourceCommit":{"committedDate":"2023-03-02T16:16:48Z","message":"[ML]
Update jest test for FieldTypeIcon
(#152019)","sha":"c2161fe3b16ba4163619a77c38dbfae0b98d9f4b","branchLabelMapping":{"^v8.8.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":[":ml","release_note:skip","Feature:File
and Index Data
Viz","v8.7.0","v8.8.0"],"number":152019,"url":"#152019
Update jest test for FieldTypeIcon
(#152019)","sha":"c2161fe3b16ba4163619a77c38dbfae0b98d9f4b"}},"sourceBranch":"main","suggestedTargetBranches":["8.7"],"targetPullRequestStates":[{"branch":"8.7","label":"v8.7.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.8.0","labelRegex":"^v8.8.0$","isSourceBranch":true,"state":"MERGED","url":"#152019
Update jest test for FieldTypeIcon
(#152019)","sha":"c2161fe3b16ba4163619a77c38dbfae0b98d9f4b"}}]}]
BACKPORT-->

Co-authored-by: Quynh Nguyen (Quinn) <43350163+qn895@users.noreply.github.com>
sloanelybutsurely pushed a commit to sloanelybutsurely/kibana that referenced this pull request Mar 8, 2023
@qn895
[ML] Update jest test for FieldTypeIcon (elastic#152019)
f9e1fb0
bmorelli25 pushed a commit to bmorelli25/kibana that referenced this pull request Mar 10, 2023
@qn895 @bmorelli25
[ML] Update jest test for FieldTypeIcon (elastic#152019)
016a7ad
nkhristinin pushed a commit that referenced this pull request Mar 22, 2023
@qn895 @nkhristinin
[ML] Update jest test for FieldTypeIcon (#152019)
4d053be
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davismcphee davismcphee davismcphee left review comments

@walterra walterra walterra approved these changes

@peteharverson peteharverson peteharverson approved these changes

Assignees

@qn895 qn895

Labels
Feature:File and Index Data Viz ML file and index data visualizer :ml release_note:skip Skip the PR/issue when compiling release notes v8.7.0 v8.8.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@qn895 @elasticmachine @walterra @kibana-ci @kibanamachine @peteharverson @davismcphee