Skip to content

[Snapshot & Restore] Add component tests for snapshot list search#121537

Merged
yuliacech merged 5 commits intoelastic:mainfrom
yuliacech:snapshots_search_tests
Dec 20, 2021
Merged

[Snapshot & Restore] Add component tests for snapshot list search#121537
yuliacech merged 5 commits intoelastic:mainfrom
yuliacech:snapshots_search_tests

Conversation

@yuliacech
Copy link
Contributor

Related to Snapshot & Restore tests issue
Follow up work for Snapshots pagination PR.

Summary

This PR adds component integration tests for search functionality in snapshots list.
Note: Instead of asserting on sinon fake server requests, I had to mock useLoadSnapshots because the sinon server doesn't keep track of query params used in the requests (sinon issue I opened for that).

@yuliacech yuliacech added technical debt Improvement of the software architecture and operational architecture Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// release_note:skip Skip the PR/issue when compiling release notes Feature:Snapshot and Restore Elasticsearch snapshots and repositories UI labels Dec 17, 2021
SNAPSHOT_LIST_MAX_SIZE: 2,
};
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This old mock was used for testing a warning callout about only showing a limited number of snapshots. The warning was removed in #110266

@yuliacech yuliacech marked this pull request as ready for review December 17, 2021 16:52
@yuliacech yuliacech requested a review from a team as a code owner December 17, 2021 16:52
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-stack-management (Team:Stack Management)

Copy link
Member

@sabarasaba sabarasaba left a comment

Choose a reason for hiding this comment

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

Nice work adding all these tests @yuliacech! Only left two small comments 🚀


describe('debounce', () => {
test('waits after input to update list params for search', async () => {
await setSearchText('snapshot=test_snapshot', false);
Copy link
Member

Choose a reason for hiding this comment

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

To improve readability, wdyt of creating a variable for this?

  const ADVANCE_TIME = false;
  await setSearchText('snapshot=test_snapshot', ADVANCE_TIME);

jest.useFakeTimers();
const snapshot = fixtures.getSnapshot({
repository: REPOSITORY_NAME,
snapshot: `a${getRandomString()}`,
Copy link
Member

Choose a reason for hiding this comment

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

is the a prefix on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I copy-pasted this from a different test, not needed here.

@yuliacech
Copy link
Contributor Author

@elasticmachine merge upstream

@yuliacech
Copy link
Contributor Author

Thank you for the review, @sabarasaba! I addressed your comments, could you please have another look?

@yuliacech yuliacech requested a review from sabarasaba December 20, 2021 11:08
Copy link
Member

@sabarasaba sabarasaba left a comment

Choose a reason for hiding this comment

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

Latest changes lgtm 🚀

* &searchField=repository&searchValue=test&searchMatch=must&searchOperator=exact
* would be shown as url=/api/snapshot_restore/snapshots is sinon server
*/
jest.mock('../../public/application/services/http', () => ({
Copy link
Contributor

@sebelga sebelga Dec 20, 2021

Choose a reason for hiding this comment

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

Just a note. I haven't made an "official" recommendation for it but personally I like that pattern to declare in a single place all the jest mocks for the component integration tests. It has 2 benefits: 1. reduce the size of the test file and 2. avoid declaring multiple times the same jest.mock(...) declaration.

In my last PR you can see

@kibana-ci
Copy link

💚 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
snapshotRestore 258.5KB 258.6KB +81.0B

History

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

@yuliacech yuliacech merged commit c4585b3 into elastic:main Dec 20, 2021
@kibanamachine kibanamachine added the backport:skip This PR does not require backporting label Dec 20, 2021
mibragimov pushed a commit to mibragimov/kibana that referenced this pull request Dec 22, 2021
…astic#121537)

* [Snapshot & Restore] Add component tests for snapshot list search

* [Snapshot & Restore] Fix term search

* [Snapshot & Restore] Add error handling tests

* [Snapshot & Restore] Add code review suggestions

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
yuliacech added a commit to yuliacech/kibana that referenced this pull request Feb 15, 2022
…astic#121537)

* [Snapshot & Restore] Add component tests for snapshot list search

* [Snapshot & Restore] Fix term search

* [Snapshot & Restore] Add error handling tests

* [Snapshot & Restore] Add code review suggestions

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit c4585b3)
yuliacech added a commit to yuliacech/kibana that referenced this pull request Feb 15, 2022
…astic#121537)

* [Snapshot & Restore] Add component tests for snapshot list search

* [Snapshot & Restore] Fix term search

* [Snapshot & Restore] Add error handling tests

* [Snapshot & Restore] Add code review suggestions

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit c4585b3)
@yuliacech
Copy link
Contributor Author

Backporting this PR to 8.0 and 7.17 to avoid merge conflicts for backports of #125234 and any further bug fixes.

yuliacech added a commit that referenced this pull request Feb 15, 2022
…21537) (#125617)

* [Snapshot & Restore] Add component tests for snapshot list search

* [Snapshot & Restore] Fix term search

* [Snapshot & Restore] Add error handling tests

* [Snapshot & Restore] Add code review suggestions

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit c4585b3)
yuliacech added a commit that referenced this pull request Feb 15, 2022
…21537) (#125618)

* [Snapshot & Restore] Add component tests for snapshot list search

* [Snapshot & Restore] Fix term search

* [Snapshot & Restore] Add error handling tests

* [Snapshot & Restore] Add code review suggestions

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit c4585b3)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@yuliacech yuliacech deleted the snapshots_search_tests branch March 7, 2022 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting Feature:Snapshot and Restore Elasticsearch snapshots and repositories UI release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// technical debt Improvement of the software architecture and operational architecture v7.17.1 v8.0.1 v8.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants