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.

Sign up for GitHub

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

Jump to bottom

[ML] Fix functional tests for index based Data Visualizer #86071

Merged
merged 12 commits into from
Dec 17, 2020
Merged

[ML] Fix functional tests for index based Data Visualizer #86071

merged 12 commits into from
Dec 17, 2020

Conversation

qn895
Copy link
Member

@qn895 qn895 commented Dec 16, 2020 •
edited
Loading

Summary

This PR is a follow up of #85726 which adds coverage for the content, sampler shard size, and filters.

Checklist

Delete any items that are not applicable to this PR.

@qn895 qn895 added the :ml label Dec 16, 2020
@qn895 qn895 self-assigned this Dec 16, 2020
@qn895 qn895 added Feature:File and Index Data Viz ML file and index data visualizer test_ui_functional v7.11.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Dec 16, 2020
pheyos
pheyos reviewed Dec 16, 2020
View reviewed changes
Copy link
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

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

Gave it a first read and in addition to the items discussed on slack, I have a few more suggestions.
Also, I think we should add a test that types a search string into the query bar and validates that the table is filtered correctly.

x-pack/plugins/ml/public/application/components/multi_select_picker/multi_select_picker.tsx Outdated Show resolved Hide resolved
x-pack/plugins/ml/public/application/components/multi_select_picker/multi_select_picker.tsx Outdated Show resolved Hide resolved
x-pack/plugins/ml/public/application/components/multi_select_picker/multi_select_picker.tsx Outdated Show resolved Hide resolved
x-pack/plugins/ml/public/application/components/multi_select_picker/multi_select_picker.tsx Outdated Show resolved Hide resolved
x-pack/test/functional/services/ml/data_visualizer_table.ts Outdated Show resolved Hide resolved
x-pack/test/functional/services/ml/data_visualizer_table.ts Outdated Show resolved Hide resolved
x-pack/test/functional/apps/ml/data_visualizer/index_data_visualizer.ts Outdated Show resolved Hide resolved
x-pack/test/functional/apps/ml/data_visualizer/index_data_visualizer.ts Outdated Show resolved Hide resolved
x-pack/test/functional/apps/ml/data_visualizer/index_data_visualizer.ts Outdated Show resolved Hide resolved
@qn895
Copy link
Member Author

qn895 commented Dec 16, 2020 •
edited
Loading

Started flaky test runner suite... no failures in 50 runs ✅

pheyos
pheyos approved these changes Dec 17, 2020
View reviewed changes
Copy link
Member

@pheyos pheyos left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM 🎉

Just two ideas for a future PR.

Also, I think we should add a test that types a search string into the query bar and validates that the table is filtered correctly.

This is still not in, but indirectly covered by loading a saved search in one of the tests, so I'm ok with leaving it for a future PR.

x-pack/test/functional/apps/ml/data_visualizer/index_data_visualizer.ts

await ml.testExecution.logTestStep(
`${testData.suiteTitle} displays elements in the field count panel correctly`
);
await ml.dataVisualizerIndexBased.assertFieldCountPanelExist();
await ml.dataVisualizerIndexBased.assertMetricFieldsSummaryExist();
await ml.dataVisualizerIndexBased.assertFieldsSummaryExist();
await ml.dataVisualizerIndexBased.assertShowEmptyFieldsSwitchExists();
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest to move this to the displays the data visualizer table test block and also checking for the search box and the filter buttons there. But this can be done in a future PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Rearranged these under the data visualizer table block here 5581931 (#86071)

x-pack/test/functional/services/ml/data_visualizer_table.ts Outdated
});
}

public async setMultiSelectFilter(
Copy link
Member

Choose a reason for hiding this comment

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

This method and removeMultiSelectFilter could probably be moved to the common UI service for future re-use. But this can be done in a future PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved these two methods to mlCommonUI here 5581931 (#86071)

@qn895
Copy link
Member Author

qn895 commented Dec 17, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 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
ml 7.0MB 7.0MB +272.0B

Distributable file count

id before after diff
default 47286 48046 +760

History

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

@qn895 qn895 marked this pull request as ready for review December 17, 2020 20:38
@qn895 qn895 requested a review from a team as a code owner December 17, 2020 20:38
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

alvarezmelissa87
alvarezmelissa87 approved these changes Dec 17, 2020
View reviewed changes
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 left a comment

Choose a reason for hiding this comment

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

LGTM ⚡

@qn895 qn895 mentioned this pull request Dec 17, 2020
Closed
35 tasks
@qn895 qn895 merged commit 426f963 into elastic:master Dec 17, 2020
@qn895 qn895 deleted the ml-functional-test-data-viz branch December 17, 2020 21:31
@qn895 qn895 mentioned this pull request Dec 18, 2020
Merged
qn895 added a commit to qn895/kibana that referenced this pull request Dec 18, 2020
@qn895 @kibanamachine
[ML] Fix functional tests for index based Data Visualizer (elastic#86071
3aef00f 
)

Co-authored-by: Kibana Machine <[email protected]>
qn895 added a commit to qn895/kibana that referenced this pull request Dec 18, 2020
@qn895 @kibanamachine
[ML] Fix functional tests for index based Data Visualizer (elastic#86071
7e64bed 
)

Co-authored-by: Kibana Machine <[email protected]>
@qn895 qn895 mentioned this pull request Dec 18, 2020
Merged
qn895 added a commit that referenced this pull request Dec 18, 2020
@qn895 @kibanamachine
[7.x] [ML] Fix functional tests for index based Data Visualizer (#86071…
7fba080 
…) (#86408)

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
qn895 added a commit that referenced this pull request Dec 18, 2020
@qn895 @kibanamachine
[7.11] [ML] Fix functional tests for index based Data Visualizer (#86071
10722e2 
) (#86409)

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pheyos pheyos pheyos approved these changes

@alvarezmelissa87 alvarezmelissa87 alvarezmelissa87 approved these changes

Assignees

@qn895 qn895

Labels
Feature:File and Index Data Viz ML file and index data visualizer :ml release_note:skip Skip the PR/issue when compiling release notes test_ui_functional v7.11.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@qn895 @kibanamachine @elasticmachine @pheyos @alvarezmelissa87