-
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
[Archive Migration] x-pack..dashboard_view_mode #108503
[Archive Migration] x-pack..dashboard_view_mode #108503
Conversation
c170ce0
to
574f214
Compare
@elasticmachine merge upstream |
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 we need a bit more cleanup in the after method.
Also, it seems there's a mix of user/role functionality here. I didn't run the test but it looks like it might be going through the roles UI to create roles? If so, we should clean that up and use the test_user service.
after(async () => { | ||
await kibanaServer.importExport.unload( | ||
'x-pack/test/functional/fixtures/kbn_archiver/dashboard_view_mode' | ||
); |
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 don't think this unload
is actually enough in this test. This test file saves a new saved search and a new dashboard. I think we would want to clean those with something like .clean(['search', 'dashboard'[);
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.
@LeeDr Looks like the test user svc is already restoring defaults. Is there more to be done regarding test users?
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.
It's not about the test user, it's about the saved objects. Currently the test does this;
- load some saved objects from a kbnArchive
- save some new SOs as a part of the test
- unload the SOs from step 1. This leaves the SOs that were created in step 2. The test should clean those up (delete the SOs).
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 the latest push addresses this boss.
d47be1f
to
f4b3253
Compare
Delete `x-pack/test/functional/es_archives/dashboard_view_mode`. Add `x-pack/test/functional/fixtures/kbn_archiver/dashboard_view_mode.json`. Update test to use the newly added archive.
Signed-off-by: Tre' Seymour <wayne.seymour@elastic.co>
f4b3253
to
b8f43ed
Compare
Signed-off-by: Tre' Seymour <wayne.seymour@elastic.co>
@mistic my thanks mate :) |
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
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.
LGTM - only code reviews
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
Comes from #102827 Helps with #108503 Piggy back off of the work from @tylersmalley Only include the changes under the test/functional/apps/discover folder.
Comes from #102827 Helps with #108503 Piggy back off of the work from @tylersmalley Only include the changes under the test/plugin_functional/test_suites/doc_views & test/visual_regression/tests/discover folders.
Comes from #102827 Helps with #108503 Piggy back off of the work from @tylersmalley Only include the changes under the test/functional/apps/home folder.
Comes from #102827 Helps with #108503 Piggy back off of the work from @tylersmalley Only include the changes under the test/functional/apps/management folder.
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. |
1 similar comment
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. |
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. |
2 similar comments
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. |
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. |
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. |
* [Archive Migration][Partial] discover apps-management Comes from #102827 Helps with #108503 Only include the changes under the test/functional/apps/management folder. * Remove the index pattern, that the test creates. * Drop beforeEach(), in favour of before(), since there's only one test. * Drop outdated comment, drop these three cleanup lines as the unload should handle it. * Just keep one cleanup. Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
* [Archive Migration][Partial] discover apps-management Comes from elastic#102827 Helps with elastic#108503 Only include the changes under the test/functional/apps/management folder. * Remove the index pattern, that the test creates. * Drop beforeEach(), in favour of before(), since there's only one test. * Drop outdated comment, drop these three cleanup lines as the unload should handle it. * Just keep one cleanup. Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
) * [Archive Migration][Partial] discover apps-management Comes from #102827 Helps with #108503 Only include the changes under the test/functional/apps/management folder. * Remove the index pattern, that the test creates. * Drop beforeEach(), in favour of before(), since there's only one test. * Drop outdated comment, drop these three cleanup lines as the unload should handle it. * Just keep one cleanup. Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Tre <wayne.seymour@elastic.co>
Comes from #102827 Helps with #108503 Piggy back off of the work from @tylersmalley Only include the changes under the test/plugin_functional/test_suites/doc_views & test/visual_regression/tests/discover folders.
Delete
x-pack/test/functional/es_archives/dashboard_view_mode
.Add
x-pack/test/functional/fixtures/kbn_archiver/dashboard_view_mode.json
.Update test to use the newly added archive.
Helps #102552