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.

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

[React@18] Run jest tests #206411

Draft
wants to merge 17 commits into
base: main
Choose a base branch
from
Draft

Conversation

Dosant
Copy link
Contributor

@Dosant Dosant commented Jan 13, 2025

Summary

Summarize your PR. If it involves visual changes include a screenshot or gif.

  • Issue with import { render } enzyme and emotion classname snapshot serializer
  • [job] [logs] Filter Group Component Filter Changed Banner should ignore url params if there is an error in using them. Additional ReactDOM.render warning in console breaks spy on console logic
  • [job] [logs] should call onUsersChange on closing the popover - Change el.click() to fireEvent.click(el)
  • [job] [logs] should call assignees update functionality with the right arguments. - Change el.click() to fireEvent.click(el)
  • [job] [logs] Jest Tests / Custom Dashboards Actions should render the unlink dashboard action when the user cannot unlink a dashboard / Custom Dashboards Actions should render the unlink dashboard action when the user cannot unlink a dashboard - issue with mock clean up, adjust the mock
  • [job] [logs] Jest Tests / Netflow renders correctly against snapshot / etflowRowRenderer renders correctly against snapshot relates to new id values react generates with colon prefix and suffix
  • [job] [logs] Jest Tests / EntryContent should render a nested value snapshot fix
  • [job] [logs] Jest Tests / TTYPlayer/hooks useXtermPlayer use waitFor for assertion
  • [job] [logs] Jest Tests / useTimelineTypes timelineFilters set timelineTypes correctly. Using fireEvent.click in place of invoking click directly on the element should resolve the issue
  • [ ]

@Dosant Dosant requested a review from eokoneyo January 13, 2025 11:17
@elasticmachine
Copy link
Contributor

elasticmachine commented Jan 13, 2025

🤖 Jobs for this PR can be triggered through checkboxes. 🚧

ℹ️ To trigger the CI, please tick the checkbox below 👇

  • Click to trigger kibana-pull-request for this PR!
  • Click to trigger kibana-deploy-project-from-pr for this PR!

@eokoneyo
Copy link
Contributor

/ci

@@ -589,8 +589,14 @@ describe(' Filter Group Component ', () => {
/>
);

expect(consoleErrorSpy.mock.calls.length).toBe(1);
expect(String(consoleErrorSpy.mock.calls[0][0])).toMatch(URL_PARAM_ARRAY_EXCEPTION_MSG);
const componentErrors = consoleErrorSpy.mock.calls.filter(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing [job] [logs] Jest Tests #3 / Filter Group Component Filter Changed Banner should ignore url params if there is an error in using them

The problem is that when running React@18 in legacy mode there is an additional log message that warns about running in legacy mode.

@@ -110,14 +110,14 @@ describe('<FilterByAssigneesPopover />', () => {
const onUsersChangeMock = jest.fn();
const { getByTestId, getByText } = renderFilterByAssigneesPopover([], onUsersChangeMock);

getByTestId(FILTER_BY_ASSIGNEES_BUTTON).click();
fireEvent.click(getByTestId(FILTER_BY_ASSIGNEES_BUTTON));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixes [job] [logs] Jest Tests #8 / should call onUsersChange on closing the popover

Looks like with an update to React@18 (even in legacy mode) these events can be skipped. By using fireEvent we wrap each event in act allowing tests to process pending events.

getByTestId(ASSIGNEES_ADD_BUTTON_TEST_ID).click();
getByText('User 1').click();
getByText('User 3').click();
fireEvent.click(getByTestId(ASSIGNEES_ADD_BUTTON_TEST_ID));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[job] [logs] Jest Tests #9 / should call assignees update functionality with the right arguments

Looks like with an update to React@18 (even in legacy mode) these events can be skipped. By using fireEvent we wrap each event in act allowing tests to process pending events.

@eokoneyo
Copy link
Contributor

/ci

@eokoneyo
Copy link
Contributor

/ci

Dosant added a commit that referenced this pull request Jan 16, 2025
## Summary

Extracted from #206411
[[job]](https://buildkite.com/elastic/kibana-pull-request/builds/267344#019469ff-7fb9-4c5d-8569-2e445aab27be)
[[logs]](https://buildkite.com/organizations/elastic/pipelines/kibana-pull-request/builds/267344/jobs/019469ff-7fb9-4c5d-8569-2e445aab27be/artifacts/01946a1c-62fa-4d30-8863-1b40f8c0b924)
Jest Tests #9 / Overview renders correctly when there is no user data
view
This simplifies overview.tsx by refactoring to rtl and removing the
whole snapshot. The snapshot was not useful and the test is still making
sure that the intended component is still rendered. By removing enzyme,
the test now works properly for both react 17 and 18.
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jan 16, 2025
## Summary

Extracted from elastic#206411
[[job]](https://buildkite.com/elastic/kibana-pull-request/builds/267344#019469ff-7fb9-4c5d-8569-2e445aab27be)
[[logs]](https://buildkite.com/organizations/elastic/pipelines/kibana-pull-request/builds/267344/jobs/019469ff-7fb9-4c5d-8569-2e445aab27be/artifacts/01946a1c-62fa-4d30-8863-1b40f8c0b924)
Jest Tests elastic#9 / Overview renders correctly when there is no user data
view
This simplifies overview.tsx by refactoring to rtl and removing the
whole snapshot. The snapshot was not useful and the test is still making
sure that the intended component is still rendered. By removing enzyme,
the test now works properly for both react 17 and 18.

(cherry picked from commit bdcad52)
@eokoneyo eokoneyo mentioned this pull request Jan 16, 2025
1 task
Dosant added a commit that referenced this pull request Jan 17, 2025
## Summary

Extracted remaining easy backward-compatible unit test fixes that fail
with React@18 from #206411

The idea is that the tests should pass for both React@17 and React@18
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jan 17, 2025
## Summary

Extracted remaining easy backward-compatible unit test fixes that fail
with React@18 from elastic#206411

The idea is that the tests should pass for both React@17 and React@18

(cherry picked from commit 3ca02b3)
cqliu1 pushed a commit to cqliu1/kibana that referenced this pull request Jan 17, 2025
## Summary

Extracted remaining easy backward-compatible unit test fixes that fail
with React@18 from elastic#206411

The idea is that the tests should pass for both React@17 and React@18
eokoneyo added a commit that referenced this pull request Jan 20, 2025
## Summary

Culled from #206411

This PR is informed from the work that's being done for the migration to
React 18, whilst trying out kibana with react 18 we had couple of test
fail relating to this particular component, details here
[[job]](https://buildkite.com/elastic/kibana-pull-request/builds/266993#0194655f-1466-4ee3-80ed-54e398b09492)
[[logs]](https://buildkite.com/organizations/elastic/pipelines/kibana-pull-request/builds/266993/jobs/0194655f-1466-4ee3-80ed-54e398b09492/artifacts/01946583-34ed-444a-bc55-10e684c325ef),
it's worth mentioning the way the component was written causes
unnecessary re-renders that doesn't actually make the interval for the
tty playspeed exactly constant. The approach taken here is such that
there's no need to depend on state change to cause the next line to be
written, now we setup a timer interval just once for the entire duration
that the tty is playing, and said timer only ever gets cleaned up on
pause.


P.S. This fix does not utilize any APIs from react 18 so it's backward
compatible with our current version of react.

### Checklist

<!-- Check the PR satisfies following conditions. 

Reviewers should verify this PR satisfies this list as well.

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials -->
- [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
<!--
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This was checked for breaking HTTP API changes, and any breaking
changes have been approved by the breaking-change committee. The
`release_note:breaking` label should be applied in these situations.
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] The PR description includes the appropriate Release Notes section,
and the correct `release_note:*` label is applied per the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

### Identify risks

Does this PR introduce any risks? For example, consider risks like hard
to test bugs, performance regression, potential of data loss.

Describe the risk, its severity, and mitigation for each identified
risk. Invite stakeholders and evaluate how to proceed before merging.

- [ ] [See some risk
examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx)
- [ ] ...

-->

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@eokoneyo
Copy link
Contributor

@elasticmachine merge upstream

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

Culled from elastic#206411

This PR is informed from the work that's being done for the migration to
React 18, whilst trying out kibana with react 18 we had couple of test
fail relating to this particular component, details here
[[job]](https://buildkite.com/elastic/kibana-pull-request/builds/266993#0194655f-1466-4ee3-80ed-54e398b09492)
[[logs]](https://buildkite.com/organizations/elastic/pipelines/kibana-pull-request/builds/266993/jobs/0194655f-1466-4ee3-80ed-54e398b09492/artifacts/01946583-34ed-444a-bc55-10e684c325ef),
it's worth mentioning the way the component was written causes
unnecessary re-renders that doesn't actually make the interval for the
tty playspeed exactly constant. The approach taken here is such that
there's no need to depend on state change to cause the next line to be
written, now we setup a timer interval just once for the entire duration
that the tty is playing, and said timer only ever gets cleaned up on
pause.

P.S. This fix does not utilize any APIs from react 18 so it's backward
compatible with our current version of react.

### Checklist

<!-- Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials -->
- [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
<!--
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This was checked for breaking HTTP API changes, and any breaking
changes have been approved by the breaking-change committee. The
`release_note:breaking` label should be applied in these situations.
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] The PR description includes the appropriate Release Notes section,
and the correct `release_note:*` label is applied per the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

### Identify risks

Does this PR introduce any risks? For example, consider risks like hard
to test bugs, performance regression, potential of data loss.

Describe the risk, its severity, and mitigation for each identified
risk. Invite stakeholders and evaluate how to proceed before merging.

- [ ] [See some risk
examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx)
- [ ] ...

-->

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
(cherry picked from commit e7b5f3d)
cqliu1 pushed a commit to cqliu1/kibana that referenced this pull request Jan 21, 2025
## Summary

Extracted remaining easy backward-compatible unit test fixes that fail
with React@18 from elastic#206411

The idea is that the tests should pass for both React@17 and React@18
cqliu1 pushed a commit to cqliu1/kibana that referenced this pull request Jan 21, 2025
## Summary

Culled from elastic#206411

This PR is informed from the work that's being done for the migration to
React 18, whilst trying out kibana with react 18 we had couple of test
fail relating to this particular component, details here
[[job]](https://buildkite.com/elastic/kibana-pull-request/builds/266993#0194655f-1466-4ee3-80ed-54e398b09492)
[[logs]](https://buildkite.com/organizations/elastic/pipelines/kibana-pull-request/builds/266993/jobs/0194655f-1466-4ee3-80ed-54e398b09492/artifacts/01946583-34ed-444a-bc55-10e684c325ef),
it's worth mentioning the way the component was written causes
unnecessary re-renders that doesn't actually make the interval for the
tty playspeed exactly constant. The approach taken here is such that
there's no need to depend on state change to cause the next line to be
written, now we setup a timer interval just once for the entire duration
that the tty is playing, and said timer only ever gets cleaned up on
pause.


P.S. This fix does not utilize any APIs from react 18 so it's backward
compatible with our current version of react.

### Checklist

<!-- Check the PR satisfies following conditions. 

Reviewers should verify this PR satisfies this list as well.

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials -->
- [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
<!--
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This was checked for breaking HTTP API changes, and any breaking
changes have been approved by the breaking-change committee. The
`release_note:breaking` label should be applied in these situations.
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] The PR description includes the appropriate Release Notes section,
and the correct `release_note:*` label is applied per the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

### Identify risks

Does this PR introduce any risks? For example, consider risks like hard
to test bugs, performance regression, potential of data loss.

Describe the risk, its severity, and mitigation for each identified
risk. Invite stakeholders and evaluate how to proceed before merging.

- [ ] [See some risk
examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx)
- [ ] ...

-->

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@Dosant
Copy link
Contributor Author

Dosant commented Jan 23, 2025

@elasticmachine merge upstream

viduni94 pushed a commit to viduni94/kibana that referenced this pull request Jan 23, 2025
## Summary

Extracted from elastic#206411
[[job]](https://buildkite.com/elastic/kibana-pull-request/builds/267344#019469ff-7fb9-4c5d-8569-2e445aab27be)
[[logs]](https://buildkite.com/organizations/elastic/pipelines/kibana-pull-request/builds/267344/jobs/019469ff-7fb9-4c5d-8569-2e445aab27be/artifacts/01946a1c-62fa-4d30-8863-1b40f8c0b924)
Jest Tests elastic#9 / Overview renders correctly when there is no user data
view
This simplifies overview.tsx by refactoring to rtl and removing the
whole snapshot. The snapshot was not useful and the test is still making
sure that the intended component is still rendered. By removing enzyme,
the test now works properly for both react 17 and 18.
viduni94 pushed a commit to viduni94/kibana that referenced this pull request Jan 23, 2025
## Summary

Extracted remaining easy backward-compatible unit test fixes that fail
with React@18 from elastic#206411

The idea is that the tests should pass for both React@17 and React@18
viduni94 pushed a commit to viduni94/kibana that referenced this pull request Jan 23, 2025
## Summary

Culled from elastic#206411

This PR is informed from the work that's being done for the migration to
React 18, whilst trying out kibana with react 18 we had couple of test
fail relating to this particular component, details here
[[job]](https://buildkite.com/elastic/kibana-pull-request/builds/266993#0194655f-1466-4ee3-80ed-54e398b09492)
[[logs]](https://buildkite.com/organizations/elastic/pipelines/kibana-pull-request/builds/266993/jobs/0194655f-1466-4ee3-80ed-54e398b09492/artifacts/01946583-34ed-444a-bc55-10e684c325ef),
it's worth mentioning the way the component was written causes
unnecessary re-renders that doesn't actually make the interval for the
tty playspeed exactly constant. The approach taken here is such that
there's no need to depend on state change to cause the next line to be
written, now we setup a timer interval just once for the entire duration
that the tty is playing, and said timer only ever gets cleaned up on
pause.


P.S. This fix does not utilize any APIs from react 18 so it's backward
compatible with our current version of react.

### Checklist

<!-- Check the PR satisfies following conditions. 

Reviewers should verify this PR satisfies this list as well.

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials -->
- [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
<!--
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This was checked for breaking HTTP API changes, and any breaking
changes have been approved by the breaking-change committee. The
`release_note:breaking` label should be applied in these situations.
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] The PR description includes the appropriate Release Notes section,
and the correct `release_note:*` label is applied per the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

### Identify risks

Does this PR introduce any risks? For example, consider risks like hard
to test bugs, performance regression, potential of data loss.

Describe the risk, its severity, and mitigation for each identified
risk. Invite stakeholders and evaluate how to proceed before merging.

- [ ] [See some risk
examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx)
- [ ] ...

-->

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@Dosant
Copy link
Contributor Author

Dosant commented Jan 24, 2025

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
files 12 13 +1
maps 1277 1278 +1
ml 2166 2167 +1
total +3

Async chunks

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

id before after diff
maps 3.1MB 3.1MB +949.0B
ml 4.7MB 4.7MB +949.0B
securitySolution 21.3MB 21.3MB -244.0B
total +1.6KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
files 10.2KB 11.1KB +950.0B
kbnUiSharedDeps-npmDll 5.9MB 5.9MB +60.2KB
total +61.2KB

Saved Objects .kibana field count

Every field in each saved object type adds overhead to Elasticsearch. Kibana needs to keep the total field count below Elasticsearch's default limit of 1000 fields. Only specify field mappings for the fields you wish to search on or query. See https://www.elastic.co/guide/en/kibana/master/saved-objects-service.html#_mappings

id before after diff
_data_stream_timestamp 1 - -1
_doc_count 1 - -1
_ignored_source 1 - -1
_index_mode 1 - -1
_tier 1 - -1
apm-custom-dashboards 5 - -5
apm-server-schema 2 - -2
apm-service-group 5 - -5
application_usage_daily 2 - -2
config 2 - -2
config-global 2 - -2
coreMigrationVersion 1 - -1
created_at 1 - -1
created_by 1 - -1
entity-definition 9 - -9
entity-discovery-api-key 2 - -2
event_loop_delays_daily 2 - -2
favorites 4 - -4
file 11 - -11
file-upload-usage-collection-telemetry 3 - -3
fileShare 5 - -5
guided-onboarding-guide-state 3 - -3
infra-custom-dashboards 4 - -4
infrastructure-monitoring-log-view 2 - -2
legacy-url-alias 7 - -7
managed 1 - -1
ml-job 6 - -6
ml-module 13 - -13
ml-trained-model 7 - -7
monitoring-telemetry 2 - -2
namespace 1 - -1
namespaces 1 - -1
observability-onboarding-state 2 - -2
originId 1 - -1
product-doc-install-status 6 - -6
references 4 - -4
sample-data-telemetry 3 - -3
slo 11 - -11
space 5 - -5
synthetics-monitor 33 - -33
tag 4 - -4
type 1 - -1
typeMigrationVersion 1 - -1
ui-metric 2 - -2
updated_at 1 - -1
updated_by 1 - -1
upgrade-assistant-ml-upgrade-operation 3 - -3
upgrade-assistant-reindex-operation 3 - -3
uptime-synthetics-api-key 2 - -2
url 5 - -5
usage-counters 2 - -2
total -199

History

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.

3 participants