Skip to content

[React18] Migrate test suites to account for testing library upgrades security-generative-ai#201160

Merged
eokoneyo merged 7 commits intomainfrom
chore/testing-library-migration-for-security-generative-ai
Dec 12, 2024
Merged

[React18] Migrate test suites to account for testing library upgrades security-generative-ai#201160
eokoneyo merged 7 commits intomainfrom
chore/testing-library-migration-for-security-generative-ai

Conversation

@eokoneyo
Copy link
Copy Markdown
Contributor

@eokoneyo eokoneyo commented Nov 21, 2024 •
edited by kibanamachine
Loading

This PR migrates test suites that use renderHook from the library @testing-library/react-hooks to adopt the equivalent and replacement of renderHook from the export that is now available from
@testing-library/react. This work is required for the planned migration to react18.

Context

In this PR, usages of waitForNextUpdate that previously could have been destructured from renderHook are now been replaced with waitFor exported from @testing-library/react, furthermore waitFor
that would also have been destructured from the same renderHook result is now been replaced with waitFor from the export of @testing-library/react.

Why is waitFor a sufficient enough replacement for waitForNextUpdate, and better for testing values subject to async computations?

WaitFor will retry the provided callback if an error is returned, till the configured timeout elapses. By default the retry interval is 50ms with a timeout value of 1000ms that
effectively translates to at least 20 retries for assertions placed within waitFor. See https://testing-library.com/docs/dom-testing-library/api-async/#waitfor for more information.
This however means that for person's writing tests, said person has to be explicit about expectations that describe the internal state of the hook being tested.
This implies checking for instance when a react query hook is being rendered, there's an assertion that said hook isn't loading anymore.

In this PR you'd notice that this pattern has been adopted, with most existing assertions following an invocation of waitForNextUpdate being placed within a waitFor
invocation. In some cases the replacement is simply a waitFor(() => new Promise((resolve) => resolve(null))) (many thanks to @kapral18, for point out exactly why this works),
where this suffices the assertions that follow aren't placed within a waitFor so this PR doesn't get larger than it needs to be.

It's also worth pointing out this PR might also contain changes to test and application code to improve said existing test.

What to do next?

  1. Review the changes in this PR.
  2. If you think the changes are correct, approve the PR.

Any questions?

If you have any questions or need help with this PR, please leave comments in this PR.

@eokoneyo eokoneyo added release_note:skip Skip the PR/issue when compiling release notes backport:prev-minor Team:Security Generative AI Security Generative AI React@18 labels Nov 21, 2024
@eokoneyo eokoneyo self-assigned this Nov 21, 2024
@eokoneyo
Copy link
Copy Markdown
Contributor Author

eokoneyo commented Nov 21, 2024

/ci

@eokoneyo eokoneyo marked this pull request as ready for review November 21, 2024 17:20
@eokoneyo eokoneyo requested a review from a team as a code owner November 21, 2024 17:20
@eokoneyo
Copy link
Copy Markdown
Contributor Author

eokoneyo commented Nov 25, 2024

@elasticmachine merge upstream

@eokoneyo
Copy link
Copy Markdown
Contributor Author

eokoneyo commented Nov 26, 2024

@elasticmachine merge upstream

@eokoneyo
Copy link
Copy Markdown
Contributor Author

eokoneyo commented Dec 2, 2024

@elasticmachine merge upstream

@eokoneyo
Copy link
Copy Markdown
Contributor Author

eokoneyo commented Dec 4, 2024

@elasticmachine merge upstream

@eokoneyo
Copy link
Copy Markdown
Contributor Author

eokoneyo commented Dec 9, 2024

@elasticmachine merge upstream

@eokoneyo
Copy link
Copy Markdown
Contributor Author

eokoneyo commented Dec 11, 2024

@elasticmachine merge upstream

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Dec 11, 2024

💚 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
securitySolution 14.7MB 14.7MB +7.0B

History

cc @eokoneyo

andrew-goldstein
andrew-goldstein reviewed Dec 11, 2024
View reviewed changes
x-pack/packages/kbn-elastic-assistant/impl/assistant/use_current_conversation/index.tsx
getDefaultConversation({ cTitle: WELCOME_CONVERSATION_TITLE });

// on the off chance that the conversation is not found, return
if (!nextConversation) {
Copy link
Copy Markdown
Contributor

@andrew-goldstein andrew-goldstein Dec 11, 2024

Choose a reason for hiding this comment

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

Thanks @eokoneyo for all the work in this PR! It appears the following (existing) tests would fail if this newly-added early return (in the hook itself) is commented out:

❌ should create a new conversation using the connector portion of the apiConfig of the current conversation (52 ms)
❌ should create a new conversation with correct isNewConversationDefault: true system prompt (9 ms)

I'm wondering:

  1. Was it (alternatively) possible to make those failing tests pass without changing the implementation of this hook?

  2. If the answer is "no", it's not possible to make those tests pass without this change, is it fair to say this change represents a fix for a bug that wasn't apparent in the original versions of the tests?

Copy link
Copy Markdown
Contributor Author

@eokoneyo eokoneyo Dec 12, 2024

Choose a reason for hiding this comment

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

Hi @andrew-goldstein yes, this new addition with the return could be viewed as a fix to a bug, it possible that whilst the application is running there will always be a conversation, but I found that with the test setup this is not the case hence why this was included. it's worth checking existing telemetry on your end to figure out if this has been happening in the wild.

andrew-goldstein
andrew-goldstein approved these changes Dec 11, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@andrew-goldstein andrew-goldstein left a comment

Choose a reason for hiding this comment

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

Thanks @eokoneyo for updating these tests and your effort to update Kibana's core dependencies 🙏
✅ Desk tested locally
LGTM 🚀

@eokoneyo eokoneyo merged commit 6ece92e into main Dec 12, 2024
@eokoneyo eokoneyo deleted the chore/testing-library-migration-for-security-generative-ai branch December 12, 2024 05:21
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Dec 12, 2024

Starting backport for target branches: 8.x

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

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Dec 12, 2024
@eokoneyo @elasticmachine
[React18] Migrate test suites to account for testing library upgrades…
45d737d 
… security-generative-ai (elastic#201160)

This PR migrates test suites that use `renderHook` from the library
`@testing-library/react-hooks` to adopt the equivalent and replacement
of `renderHook` from the export that is now available from
`@testing-library/react`. This work is required for the planned
migration to react18.

##  Context

In this PR, usages of `waitForNextUpdate` that previously could have
been destructured from `renderHook` are now been replaced with `waitFor`
exported from `@testing-library/react`, furthermore `waitFor`
that would also have been destructured from the same renderHook result
is now been replaced with `waitFor` from the export of
`@testing-library/react`.

***Why is `waitFor` a sufficient enough replacement for
`waitForNextUpdate`, and better for testing values subject to async
computations?***

WaitFor will retry the provided callback if an error is returned, till
the configured timeout elapses. By default the retry interval is `50ms`
with a timeout value of `1000ms` that
effectively translates to at least 20 retries for assertions placed
within waitFor. See
https://testing-library.com/docs/dom-testing-library/api-async/#waitfor
for more information.
This however means that for person's writing tests, said person has to
be explicit about expectations that describe the internal state of the
hook being tested.
This implies checking for instance when a react query hook is being
rendered, there's an assertion that said hook isn't loading anymore.

In this PR you'd notice that this pattern has been adopted, with most
existing assertions following an invocation of `waitForNextUpdate` being
placed within a `waitFor`
invocation. In some cases the replacement is simply a `waitFor(() => new
Promise((resolve) => resolve(null)))` (many thanks to @kapral18, for
point out exactly why this works),
where this suffices the assertions that follow aren't placed within a
waitFor so this PR doesn't get larger than it needs to be.

It's also worth pointing out this PR might also contain changes to test
and application code to improve said existing test.

### What to do next?
1. Review the changes in this PR.
2. If you think the changes are correct, approve the PR.

## Any questions?
If you have any questions or need help with this PR, please leave
comments in this PR.

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
(cherry picked from commit 6ece92e)
@kibanamachine kibanamachine mentioned this pull request Dec 12, 2024
Merged
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Dec 12, 2024

💚 All backports created successfully

Status Branch Result
✅ 8.x

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

Questions ?

Please refer to the Backport tool documentation

@kibanamachine kibanamachine mentioned this pull request Dec 12, 2024
Merged
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Dec 12, 2024
@eokoneyo @elasticmachine @CAWilson94
[React18] Migrate test suites to account for testing library upgrades…
a6ecfab 
… security-generative-ai (elastic#201160)

This PR migrates test suites that use `renderHook` from the library
`@testing-library/react-hooks` to adopt the equivalent and replacement
of `renderHook` from the export that is now available from
`@testing-library/react`. This work is required for the planned
migration to react18.

##  Context

In this PR, usages of `waitForNextUpdate` that previously could have
been destructured from `renderHook` are now been replaced with `waitFor`
exported from `@testing-library/react`, furthermore `waitFor`
that would also have been destructured from the same renderHook result
is now been replaced with `waitFor` from the export of
`@testing-library/react`.

***Why is `waitFor` a sufficient enough replacement for
`waitForNextUpdate`, and better for testing values subject to async
computations?***

WaitFor will retry the provided callback if an error is returned, till
the configured timeout elapses. By default the retry interval is `50ms`
with a timeout value of `1000ms` that
effectively translates to at least 20 retries for assertions placed
within waitFor. See
https://testing-library.com/docs/dom-testing-library/api-async/#waitfor
for more information.
This however means that for person's writing tests, said person has to
be explicit about expectations that describe the internal state of the
hook being tested.
This implies checking for instance when a react query hook is being
rendered, there's an assertion that said hook isn't loading anymore.

In this PR you'd notice that this pattern has been adopted, with most
existing assertions following an invocation of `waitForNextUpdate` being
placed within a `waitFor`
invocation. In some cases the replacement is simply a `waitFor(() => new
Promise((resolve) => resolve(null)))` (many thanks to @kapral18, for
point out exactly why this works),
where this suffices the assertions that follow aren't placed within a
waitFor so this PR doesn't get larger than it needs to be.

It's also worth pointing out this PR might also contain changes to test
and application code to improve said existing test.

### What to do next?
1. Review the changes in this PR.
2. If you think the changes are correct, approve the PR.

## Any questions?
If you have any questions or need help with this PR, please leave
comments in this PR.

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Dec 13, 2024
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Dec 13, 2024

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

kibanamachine added a commit that referenced this pull request Dec 13, 2024
@kibanamachine @eokoneyo @elasticmachine
[8.x] [React18] Migrate test suites to account for testing library up…
4b0adec 
…grades security-generative-ai (#201160) (#203960)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[React18] Migrate test suites to account for testing library upgrades
security-generative-ai
(#201160)](#201160)

<!--- Backport version: 9.4.3 -->

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

<!--BACKPORT [{"author":{"name":"Eyo O.
Eyo","email":"7893459+eokoneyo@users.noreply.github.com"},"sourceCommit":{"committedDate":"2024-12-12T05:21:20Z","message":"[React18]
Migrate test suites to account for testing library upgrades
security-generative-ai (#201160)\n\nThis PR migrates test suites that
use `renderHook` from the library\r\n`@testing-library/react-hooks` to
adopt the equivalent and replacement\r\nof `renderHook` from the export
that is now available from\r\n`@testing-library/react`. This work is
required for the planned\r\nmigration to react18.\r\n\r\n##
Context\r\n\r\nIn this PR, usages of `waitForNextUpdate` that previously
could have\r\nbeen destructured from `renderHook` are now been replaced
with `waitFor`\r\nexported from `@testing-library/react`, furthermore
`waitFor`\r\nthat would also have been destructured from the same
renderHook result\r\nis now been replaced with `waitFor` from the export
of\r\n`@testing-library/react`.\r\n\r\n***Why is `waitFor` a sufficient
enough replacement for\r\n`waitForNextUpdate`, and better for testing
values subject to async\r\ncomputations?***\r\n\r\nWaitFor will retry
the provided callback if an error is returned, till\r\nthe configured
timeout elapses. By default the retry interval is `50ms`\r\nwith a
timeout value of `1000ms` that\r\neffectively translates to at least 20
retries for assertions placed\r\nwithin waitFor.
See\r\nhttps://testing-library.com/docs/dom-testing-library/api-async/#waitfor\r\nfor
more information.\r\nThis however means that for person's writing tests,
said person has to\r\nbe explicit about expectations that describe the
internal state of the\r\nhook being tested.\r\nThis implies checking for
instance when a react query hook is being\r\nrendered, there's an
assertion that said hook isn't loading anymore.\r\n\r\nIn this PR you'd
notice that this pattern has been adopted, with most\r\nexisting
assertions following an invocation of `waitForNextUpdate`
being\r\nplaced within a `waitFor`\r\ninvocation. In some cases the
replacement is simply a `waitFor(() => new\r\nPromise((resolve) =>
resolve(null)))` (many thanks to @kapral18, for\r\npoint out exactly why
this works),\r\nwhere this suffices the assertions that follow aren't
placed within a\r\nwaitFor so this PR doesn't get larger than it needs
to be.\r\n\r\nIt's also worth pointing out this PR might also contain
changes to test\r\nand application code to improve said existing
test.\r\n\r\n### What to do next?\r\n1. Review the changes in this
PR.\r\n2. If you think the changes are correct, approve the
PR.\r\n\r\n## Any questions?\r\nIf you have any questions or need help
with this PR, please leave\r\ncomments in this
PR.\r\n\r\nCo-authored-by: Elastic Machine
<elasticmachine@users.noreply.github.com>","sha":"6ece92e9e121d6c90950ff7d2f2f5195aafdbfb9","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","backport:prev-minor","Team:Security
Generative AI","React@18"],"title":"[React18] Migrate test suites to
account for testing library upgrades
security-generative-ai","number":201160,"url":"https://github.com/elastic/kibana/pull/201160","mergeCommit":{"message":"[React18]
Migrate test suites to account for testing library upgrades
security-generative-ai (#201160)\n\nThis PR migrates test suites that
use `renderHook` from the library\r\n`@testing-library/react-hooks` to
adopt the equivalent and replacement\r\nof `renderHook` from the export
that is now available from\r\n`@testing-library/react`. This work is
required for the planned\r\nmigration to react18.\r\n\r\n##
Context\r\n\r\nIn this PR, usages of `waitForNextUpdate` that previously
could have\r\nbeen destructured from `renderHook` are now been replaced
with `waitFor`\r\nexported from `@testing-library/react`, furthermore
`waitFor`\r\nthat would also have been destructured from the same
renderHook result\r\nis now been replaced with `waitFor` from the export
of\r\n`@testing-library/react`.\r\n\r\n***Why is `waitFor` a sufficient
enough replacement for\r\n`waitForNextUpdate`, and better for testing
values subject to async\r\ncomputations?***\r\n\r\nWaitFor will retry
the provided callback if an error is returned, till\r\nthe configured
timeout elapses. By default the retry interval is `50ms`\r\nwith a
timeout value of `1000ms` that\r\neffectively translates to at least 20
retries for assertions placed\r\nwithin waitFor.
See\r\nhttps://testing-library.com/docs/dom-testing-library/api-async/#waitfor\r\nfor
more information.\r\nThis however means that for person's writing tests,
said person has to\r\nbe explicit about expectations that describe the
internal state of the\r\nhook being tested.\r\nThis implies checking for
instance when a react query hook is being\r\nrendered, there's an
assertion that said hook isn't loading anymore.\r\n\r\nIn this PR you'd
notice that this pattern has been adopted, with most\r\nexisting
assertions following an invocation of `waitForNextUpdate`
being\r\nplaced within a `waitFor`\r\ninvocation. In some cases the
replacement is simply a `waitFor(() => new\r\nPromise((resolve) =>
resolve(null)))` (many thanks to @kapral18, for\r\npoint out exactly why
this works),\r\nwhere this suffices the assertions that follow aren't
placed within a\r\nwaitFor so this PR doesn't get larger than it needs
to be.\r\n\r\nIt's also worth pointing out this PR might also contain
changes to test\r\nand application code to improve said existing
test.\r\n\r\n### What to do next?\r\n1. Review the changes in this
PR.\r\n2. If you think the changes are correct, approve the
PR.\r\n\r\n## Any questions?\r\nIf you have any questions or need help
with this PR, please leave\r\ncomments in this
PR.\r\n\r\nCo-authored-by: Elastic Machine
<elasticmachine@users.noreply.github.com>","sha":"6ece92e9e121d6c90950ff7d2f2f5195aafdbfb9"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/201160","number":201160,"mergeCommit":{"message":"[React18]
Migrate test suites to account for testing library upgrades
security-generative-ai (#201160)\n\nThis PR migrates test suites that
use `renderHook` from the library\r\n`@testing-library/react-hooks` to
adopt the equivalent and replacement\r\nof `renderHook` from the export
that is now available from\r\n`@testing-library/react`. This work is
required for the planned\r\nmigration to react18.\r\n\r\n##
Context\r\n\r\nIn this PR, usages of `waitForNextUpdate` that previously
could have\r\nbeen destructured from `renderHook` are now been replaced
with `waitFor`\r\nexported from `@testing-library/react`, furthermore
`waitFor`\r\nthat would also have been destructured from the same
renderHook result\r\nis now been replaced with `waitFor` from the export
of\r\n`@testing-library/react`.\r\n\r\n***Why is `waitFor` a sufficient
enough replacement for\r\n`waitForNextUpdate`, and better for testing
values subject to async\r\ncomputations?***\r\n\r\nWaitFor will retry
the provided callback if an error is returned, till\r\nthe configured
timeout elapses. By default the retry interval is `50ms`\r\nwith a
timeout value of `1000ms` that\r\neffectively translates to at least 20
retries for assertions placed\r\nwithin waitFor.
See\r\nhttps://testing-library.com/docs/dom-testing-library/api-async/#waitfor\r\nfor
more information.\r\nThis however means that for person's writing tests,
said person has to\r\nbe explicit about expectations that describe the
internal state of the\r\nhook being tested.\r\nThis implies checking for
instance when a react query hook is being\r\nrendered, there's an
assertion that said hook isn't loading anymore.\r\n\r\nIn this PR you'd
notice that this pattern has been adopted, with most\r\nexisting
assertions following an invocation of `waitForNextUpdate`
being\r\nplaced within a `waitFor`\r\ninvocation. In some cases the
replacement is simply a `waitFor(() => new\r\nPromise((resolve) =>
resolve(null)))` (many thanks to @kapral18, for\r\npoint out exactly why
this works),\r\nwhere this suffices the assertions that follow aren't
placed within a\r\nwaitFor so this PR doesn't get larger than it needs
to be.\r\n\r\nIt's also worth pointing out this PR might also contain
changes to test\r\nand application code to improve said existing
test.\r\n\r\n### What to do next?\r\n1. Review the changes in this
PR.\r\n2. If you think the changes are correct, approve the
PR.\r\n\r\n## Any questions?\r\nIf you have any questions or need help
with this PR, please leave\r\ncomments in this
PR.\r\n\r\nCo-authored-by: Elastic Machine
<elasticmachine@users.noreply.github.com>","sha":"6ece92e9e121d6c90950ff7d2f2f5195aafdbfb9"}}]}]
BACKPORT-->

Co-authored-by: Eyo O. Eyo <7893459+eokoneyo@users.noreply.github.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@kibanamachine kibanamachine added v8.18.0 and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andrew-goldstein andrew-goldstein andrew-goldstein approved these changes

Assignees

@eokoneyo eokoneyo

Labels

React@18 release_note:skip Skip the PR/issue when compiling release notes Team:Security Generative AI Security Generative AI v8.18.0 v9.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@eokoneyo @elasticmachine @kibanamachine @andrew-goldstein