Skip to content

[ML] Add clone tests for single metric, multi metric and population jobs#46894

Merged
pheyos merged 14 commits intoelastic:masterfrom
pheyos:clone_job_tests
Sep 30, 2019
Merged

[ML] Add clone tests for single metric, multi metric and population jobs#46894
pheyos merged 14 commits intoelastic:masterfrom
pheyos:clone_job_tests

Conversation

@pheyos
Copy link
Member

@pheyos pheyos commented Sep 30, 2019

Summary

This PR adds tests for ML single metric, multi metric and population job cloning.

High level test steps

  • The clone tests for single metric, multi metric and population jobs continue where the corresponding create job test left off.
  • Click clone button and verify that the correct job wizard is used
  • Verify that fields are pre-filled correctly
  • Create the clone job and verify job details

Other changes

  • Rename test files from create_single_metric_job to single_metric_job as the test file now also contains a cloning sub suite (similar for multi metric and population jobs).
  • Refactor a couple job wizard service actions to immediately validate that the action has been executed correctly, also change order of methods to be grouped for each UI element

@pheyos pheyos added :ml test_ui_functional v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.5.0 labels Sep 30, 2019
@pheyos pheyos requested a review from a team as a code owner September 30, 2019 08:49
@pheyos pheyos self-assigned this Sep 30, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

Added a couple of unimportant nits, other than that LGTM

return (
<Fragment>
<EuiPage style={{ backgroundColor: 'inherit' }} data-test-subj="mlPageJobWizard">
<EuiPage style={{ backgroundColor: 'inherit' }} data-test-subj={'mlPageJobWizard ' + jobType}>
Copy link
Member

Choose a reason for hiding this comment

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

nit, we normally use template literals for string concatenation. this would be `mlPageJobWizard ${jobType}`

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed in c3ef36f

<div
style={{ width, height }}
data-test-subj={
eventRateChartData.length > 0 ? 'mlEventRateChart withData' : 'mlEventRateChart empty'
Copy link
Member

Choose a reason for hiding this comment

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

nit, could be:
mlEventRateChart ${eventRateChartData.length ? 'withData' : 'empty'}
but I don't know if it's much of an improvement, it just removes the duplication of mlEventRateChart

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed in c3ef36f

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

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.

One nit but otherwise all LGTM

}
onClick={startJobInRealTime}
data-test-subj="mlButtonUseFullData3"
data-test-subj="mlJobWizardButtonRunInRealtime"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit - but elsewhere we use RealTime, so should be mlJobWizardButtonRunInRealTime for consistency.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed in c3ef36f

'job processing to finish',
5 * 60 * 1000,
async () => (await testSubjects.exists('mlJobWizardButtonCreateJob')) === false
async () => await testSubjects.exists('mlJobWizardButtonRunInRealtime')
Copy link
Contributor

Choose a reason for hiding this comment

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

As above, better to use mlJobWizardButtonRunInRealTime with uppercase T for consistency.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed in c3ef36f

Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

LGTM, just added a question about data-test-subj naming.

return (
<Fragment>
<div style={{ minWidth: WIDTH }}>
<div style={{ minWidth: WIDTH }} data-test-subj={`jobWizardDateRange`}>
Copy link
Contributor

Choose a reason for hiding this comment

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

I see most data-test-subj have a ml prefix but this one does not - should we generally have it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. When I started creating test subjects, I've named them ml* with a rather long descriptive name, but over time I began to give shorter names to make parent-child relations better to read, e.g. testSubjects.getVisibleText(`dataSplit > splitCard front > splitCardTitle`). I'm still not perfectly sure what the best way of naming is, but ml namespacing is probably a good idea, so I will go through our test subjects in a follow-up and add it where needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in #47032

@elasticmachine
Copy link
Contributor

elasticmachine commented Sep 30, 2019

💔 Build Failed

Unrelated failure in x-pack ciGroup7

@pheyos
Copy link
Member Author

pheyos commented Sep 30, 2019

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

LGTM

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 test_ui_functional v7.5.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants