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.

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

[ML] Add functional tests for Data Drift view #167911

Merged
merged 17 commits into from
Oct 12, 2023

Conversation

qn895
Copy link
Member

@qn895 qn895 commented Oct 3, 2023

Summary

This PR adds functional test for Data drift view. It covers the following scenarios:

  • From index or saved search selection page, open data drift for KQL saved search
    • Check doc count reflects the query & filters
    • Check table exists
  • From data view creation view, open data drift view for time series dataset without creating new data view
  • From data view creation view, save new data view, and open data drift view for timeseries dataset
  • From data view creation view, save new data view, and open data drift view for non-timeseries dataset
  • From Trained models list' available actions, open data drift's data view creation view, open data drift view for timeseries

Flaky test suite runner... 49/50 passed ✅ - failed test is an unrelated flaky test

PR also fixes:

  • When ks test returns NaN value, this happens when the distributions are non overlapping. This means there is a drift and the p-value would be astronomically small. This PR enforces so that it shows Drift detected as 'Yes' and value < 0.000001.

Checklist

@qn895 qn895 force-pushed the ml-data-drift-func-tests branch 3 times, most recently from 887457f to c6642fd Compare October 9, 2023 19:39
@qn895 qn895 requested review from alvarezmelissa87 and removed request for alvarezmelissa87 October 9, 2023 20:45
@qn895 qn895 marked this pull request as ready for review October 10, 2023 04:40
@qn895 qn895 requested a review from a team as a code owner October 10, 2023 04:40
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

LGTM, although looks like one of the flaky test runner failed.

@qn895
Copy link
Member Author

qn895 commented Oct 10, 2023

LGTM, although looks like one of the flaky test runner failed.

The failed test is actually not related to data drift 🤔 Looks like a flaky test, need some more investigation.

@qn895 qn895 added the release_note:skip Skip the PR/issue when compiling release notes label Oct 10, 2023
@@ -149,7 +152,9 @@ export const DataDriftOverviewTable = ({
'data-test-subj': 'mlDataDriftOverviewTableDriftDetected',
sortable: true,
textOnly: true,
render: (driftDetected: boolean) => {
render: (driftDetected: boolean, item) => {
// @ts-expect-error currently ES two_sided does return string NaN, will be fixed
Copy link
Contributor

Choose a reason for hiding this comment

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

is there an issue we can mention in the comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

Valeriy is investigating this and will create an issue on the Elasticsearch side. But for now, we don't have an issue.

);
await ml.dataDrift.assertNoWindowParametersEmptyPromptExists();
await ml.testExecution.logTestStep(
`${testData.suiteTitle} displays elements in the page correctly`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`${testData.suiteTitle} displays elements in the page correctly`
`${testData.suiteTitle} displays elements on the page correctly`

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated here 0ef6b56 (#167911)

Comment on lines 306 to 308
const btn = await testSubjects.find('analyzeDataDriftButton');
const isDisabled = await btn.getAttribute('disabled');
expect(isDisabled).to.equal(disabled ? 'true' : null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const btn = await testSubjects.find('analyzeDataDriftButton');
const isDisabled = await btn.getAttribute('disabled');
expect(isDisabled).to.equal(disabled ? 'true' : null);
const isDisabled = !(awaot testSubjects. isEnabled('analyzeDataDriftButton));
expect(isDisabled).to.equal(disabled);

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated here 0ef6b56 (#167911)

Comment on lines 298 to 300
const btn = await testSubjects.find('analyzeDataDriftWithoutSavingButton');
const isDisabled = await btn.getAttribute('disabled');
expect(isDisabled).to.equal(disabled ? 'true' : null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const btn = await testSubjects.find('analyzeDataDriftWithoutSavingButton');
const isDisabled = await btn.getAttribute('disabled');
expect(isDisabled).to.equal(disabled ? 'true' : null);
const isDisabled = !(await testSubjects.isEnabled('analyzeDataDriftWithoutSavingButton'));
expect(isDisabled).to.equal(disabled);

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated here 0ef6b56 (#167911)

});
}

public async clickAnalyzeDataDriftActionButton(modelId: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can utiilze the invokeAction method from this service instead

Copy link
Member Author

Choose a reason for hiding this comment

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

invokeAction doesn't seem to account for when button is directly on the row (e.g. when there are <= 2 actions), or is hidden by the context menu (> 2 actions available) 🤔

);
}

public async assertAnalyzeDataDriftActionButtonEnabled(
Copy link
Contributor

Choose a reason for hiding this comment

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

would be great to have a reusable assertRowActionEnabled method in generic table service

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that we should have a reusable assertRowActionEnabled, but the current generic table service doesn't have concept for a context menu for the actions. That would require a lot of refactoring for the current trained models table & the generic service. I think that's out of scope of this PR.

@qn895 qn895 requested a review from darnautov October 11, 2023 21:26
@qn895
Copy link
Member Author

qn895 commented Oct 12, 2023

@elasticmachine merge upstream

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested with fix for item 1 in #168090, and LGTM

@qn895
Copy link
Member Author

qn895 commented Oct 12, 2023

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

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
dataVisualizer 613.6KB 613.8KB +205.0B
ml 3.5MB 3.5MB +301.0B
total +506.0B

History

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

cc @qn895

@qn895 qn895 merged commit f28349f into elastic:main Oct 12, 2023
34 checks passed
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 12, 2023
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit f28349f)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.11

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Oct 13, 2023
# Backport

This will backport the following commits from `main` to `8.11`:
- [[ML] Add functional tests for Data Drift view
(#167911)](#167911)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Quynh Nguyen
(Quinn)","email":"43350163+qn895@users.noreply.github.com"},"sourceCommit":{"committedDate":"2023-10-12T22:52:25Z","message":"[ML]
Add functional tests for Data Drift view (#167911)\n\nCo-authored-by:
Kibana Machine
<42973632+kibanamachine@users.noreply.github.com>","sha":"f28349faf19d78bb679918297742a0bf7cb064f5","branchLabelMapping":{"^v8.12.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":[":ml","test_ui_functional","release_note:skip","v8.11.0","v8.12.0","v8.11.1"],"number":167911,"url":"#167911
Add functional tests for Data Drift view (#167911)\n\nCo-authored-by:
Kibana Machine
<42973632+kibanamachine@users.noreply.github.com>","sha":"f28349faf19d78bb679918297742a0bf7cb064f5"}},"sourceBranch":"main","suggestedTargetBranches":["8.11"],"targetPullRequestStates":[{"branch":"8.11","label":"v8.11.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.12.0","labelRegex":"^v8.12.0$","isSourceBranch":true,"state":"MERGED","url":"#167911
Add functional tests for Data Drift view (#167911)\n\nCo-authored-by:
Kibana Machine
<42973632+kibanamachine@users.noreply.github.com>","sha":"f28349faf19d78bb679918297742a0bf7cb064f5"}}]}]
BACKPORT-->

Co-authored-by: Quynh Nguyen (Quinn) <43350163+qn895@users.noreply.github.com>
dej611 pushed a commit to dej611/kibana that referenced this pull request Oct 17, 2023
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
benakansara pushed a commit to benakansara/kibana that referenced this pull request Oct 22, 2023
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
benakansara pushed a commit to benakansara/kibana that referenced this pull request Oct 22, 2023
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants