-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Add CITs for index management to show hidden and rollup indices and update existing tabs test to reflect all tabs #112010
Conversation
Pinging @elastic/kibana-stack-management (Team:Stack Management) |
x-pack/plugins/index_management/__jest__/client_integration/helpers/test_subjects.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/index_management/__jest__/client_integration/home/home.helpers.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/index_management/__jest__/client_integration/home/home.test.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/index_management/__jest__/client_integration/home/home.test.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/index_management/__jest__/client_integration/home/home.test.ts
Outdated
Show resolved
Hide resolved
…Management_CITs_To_Use_Testbed_And_Add_Coverage
…the context menu updating after closing the index.
…Management_CITs_To_Use_Testbed_And_Add_Coverage
💚 Build SucceededMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
|
||
await act(async () => { | ||
await nextTick(); | ||
component.update(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think our current understanding of flaky tests is that we don't need to use nextTick()
instead I think we put the setup()
in an act()
wrapper, similar to this:
await act(async () => {
testBed = await setup();
});
const { component } = testBed;
component.update();
The code in Index Management is a bit older than in ILM, that is why there are still nextTcik()
calls. But could you please update your new tests with this snippet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please please never use nextTick()
anymore 👍
.childAt(1) | ||
.childAt(0) | ||
.find('button[data-test-subj="indexTableContextMenuButton"]') | ||
.at(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code looks a bit confusing, I think that's because the context buttons don't have specific data-test-subj
values. I suggest add a value in this file to use in the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for working on these tests, @cuff-links!
I left 2 comments, I think the one about nextTick
could be great to fix before merging and the 2nd one is non-blocking for me.
Summary
This PR is to add the test coverage for showing system Indices and also for showing rollup Indices. It's a follow-up to https://github.com/elastic/kibana-team/issues/342.
Test cases being covered
Shows Rollup Indices (After adding a mock rollup index)(Removed after finding out that we can't load the rollups plugin to test this. Covered by Functional Test.)