Skip to content

[ML] Adds functional tests for the index data visualizer card contents#83174

Merged
peteharverson merged 4 commits intoelastic:masterfrom
peteharverson:ml-datavisualizer-contents-tests
Nov 13, 2020
Merged

[ML] Adds functional tests for the index data visualizer card contents#83174
peteharverson merged 4 commits intoelastic:masterfrom
peteharverson:ml-datavisualizer-contents-tests

Conversation

@peteharverson
Copy link
Copy Markdown
Contributor

@peteharverson peteharverson commented Nov 11, 2020

Summary

Adds functional tests for the contents of the cards in the index-based data visualizer, plus a test that a change to the sample size (per shard) control results in a change to the displayed document count for one of the cards.

Additional tests added:

  • That each of the expected content elements (doc count, cardinality, charts, examples etc) are displayed
  • For number fields, that the expected details mode (top values or distribution chart) is displayed, and that the button to toggle the details mode changes the contents
  • That the expected number of top values / examples are displayed for number, keyword and text types
  • For number fields, checks the expected document card, and that the min / median and max values are displayed to the expected precision (max 3 decimal places)

Also adds in formatting for the total document count field, so it is consistent with the display of document counts inside each of the cards.
Before:
image

After:
image

Checklist

@peteharverson peteharverson added review :ml v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.11.0 labels Nov 11, 2020
@peteharverson peteharverson self-assigned this Nov 11, 2020
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/ml-ui (:ml)

},

async assertNonMetricCardContents(cardType: string, fieldName: string, exampleCount?: number) {
await testSubjects.existOrFail(
Copy link
Copy Markdown
Member

@qn895 qn895 Nov 11, 2020

Choose a reason for hiding this comment

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

Since this assertion for mlFieldDataCardContent is used in multiple places I think it would be good to have its own function with fieldName and cardType as the arguments. Same for mlFieldDataCardDocCount and mlFieldDataCardCardinality.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in 563f2a3

@peteharverson peteharverson requested a review from a team as a code owner November 12, 2020 09:45
Copy link
Copy Markdown
Contributor

@darnautov darnautov left a comment

Choose a reason for hiding this comment

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

LGTM

defaultMessage="Sample size (per shard): {wrappedValue}"
values={{ wrappedValue: <b>{v}</b> }}
/>
<div data-test-subj={`mlDataVisualizerShardSizeOption ${v}`}>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: I presume it should be span instead of div, just to make sure the element stays inline like FormattedMessage result element

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Switched to <span> in 563f2a3

@pheyos
Copy link
Copy Markdown
Member

pheyos commented Nov 12, 2020

Checking test stability in a flaky test runner job ... no failure in 50 runs ✔️

Copy link
Copy Markdown
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.

Great to have those tests in! 🎉
LGTM

@kibanamachine
Copy link
Copy Markdown
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 6.7MB 6.7MB +2.1KB

History

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

@peteharverson peteharverson merged commit dde2d11 into elastic:master Nov 13, 2020
@peteharverson peteharverson deleted the ml-datavisualizer-contents-tests branch November 13, 2020 09:39
peteharverson added a commit to peteharverson/kibana that referenced this pull request Nov 13, 2020
elastic#83174)

* [ML] Adds functional tests for the index data visualizer card contents

* [ML] Fix translations

* [ML] Fix type errors in permissions tests

* [ML] Address comments from review
peteharverson added a commit that referenced this pull request Nov 13, 2020
#83174) (#83371)

* [ML] Adds functional tests for the index data visualizer card contents

* [ML] Fix translations

* [ML] Fix type errors in permissions tests

* [ML] Address comments from review
phillipb added a commit to phillipb/kibana that referenced this pull request Nov 13, 2020
…o-node-details

* 'master' of github.com:elastic/kibana:
  [ML] Adds functional tests for the index data visualizer card contents (elastic#83174)
gmmorris added a commit to gmmorris/kibana that referenced this pull request Nov 16, 2020
* master:
  Migrate `/translations` route to core (elastic#83280)
  [APM] Ensure APM jest script can run (elastic#83398)
  [Uptime] Monitor status alert use url as instance (elastic#81736)
  [ML] Add basic license test run details to ML+Transform READMEs (elastic#83259)
  TSVB doesn't communicate it's index-patterns to dashboard (elastic#82964)
  [Alerting UI] Added ability to assign alert actions to resolved action group in UI (elastic#83139)
  Skips Vega test
  skip flaky suite (elastic#79967)
  [bundle optimization] Update to semver 7.x to get tree-shaking (elastic#83020)
  Added ability to fire actions when an alert instance is resolved (elastic#82799)
  [ML] Adds functional tests for the index data visualizer card contents (elastic#83174)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:ml release_note:skip Skip the PR/issue when compiling release notes review v7.11.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants