-
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
[ML] Add AD job validation UI tests #179358
[ML] Add AD job validation UI tests #179358
Conversation
- Add job wizard validation suite - Add job wizard model change annotation recommendation callout test subject - Add job wizard model time picker test subject within a div, as the EUI documents are incorrect about data-test-subjs for this component - Add test subjects for two steps: Time Range and Job Details - Add more methods to job wizard common service
Pinging @elastic/ml-ui (:ml) |
Use selectors over visible text. Change assertions based on lessons learned from previous code reviews.
x-pack/plugins/ml/public/application/components/callout/callout.tsx
Outdated
Show resolved
Hide resolved
dateFormat={dateFormat} | ||
minDate={startMoment} | ||
/> | ||
<div data-test-subj="mlJobWizardDatePickerRangeEndDate"> |
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.
https://eui.elastic.co/#/forms/date-picker supports data-test-subj
, hence you do not need a wrapper.
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 tried it, following the docs per our zoom, but it did not work. I'll try again though!
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.
@darnautov perhaps a longer timeout?
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.
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.
@wayneseymour you set a data-test-subj
prop in the wrong place. It should be for EuiDatePickerRange
, not endDateControl
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.
Ah ok, so many dates lol. Thanks @darnautov 🙉
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.
yeah I dont think either of those work, so I've switched it to a span
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.
no more span!
1f9611a
x-pack/plugins/ml/public/application/jobs/new_job/pages/new_job/wizard_horizontal_steps.tsx
Outdated
Show resolved
Hide resolved
async getAnnotationSwitchCheckedState(): Promise<boolean> { | ||
const subj = 'mlJobWizardSwitchAnnotations'; | ||
const isSelected = await testSubjects.getAttribute(subj, 'aria-checked'); | ||
return isSelected === 'true'; | ||
}, | ||
|
||
async getAnnotationsSwitchCheckedState(expectedValue: boolean) { | ||
const actualCheckedState = await this.getAnnotationSwitchCheckedState(); | ||
expect(actualCheckedState).to.eql( | ||
expectedValue, | ||
`Expected annotations switch to be '${expectedValue ? 'enabled' : 'disabled'}' (got '${ | ||
actualCheckedState ? 'enabled' : 'disabled' | ||
}')` | ||
); | ||
}, | ||
|
||
async toggleAnnotationSwitch(toggle: boolean) { | ||
const subj = 'mlJobWizardSwitchAnnotations'; | ||
if ((await this.getAnnotationSwitchCheckedState()) !== toggle) { | ||
await retry.tryForTime(5 * 1000, async () => { | ||
await testSubjects.clickWhenNotDisabledWithoutRetry(subj); | ||
await this.getAnnotationSwitchCheckedState(); | ||
}); | ||
} | ||
}, |
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.
We have a similar code snippet copy-pasted in so many places. Can you please create reusable methods in x-pack/test/functional/services/ml/common_ui.ts
?
- getSwitchChechedState(testSubj: string)
- toggleSwitchIfNeeded(testSubj: string, targetState: boolean)
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.
}, | ||
]; | ||
|
||
describe('job wizard validation', function () { |
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 reckon you should not create a dedicated test suite for that, and go through all the wizard steps again.
We already have multiple tests for job wizards. Simply add a test suite to assert job validation.
For instance in the multi-metric wizards tests:
await ml.testExecution.logTestStep('job creation displays the validation step');
await ml.jobWizardCommon.advanceToValidationSection();
Add your assertion after these lines
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.
@darnautov One of the reasons I created this is not have a nested suite.
I'll see if I can fit these in elsewhere without a nested suite.
Co-authored-by: Dima Arnautov <arnautov.dima@gmail.com>
…our/kibana into ml-add-job-validation-ui-tests
…t.tsx Co-authored-by: Dima Arnautov <arnautov.dima@gmail.com>
…our/kibana into ml-add-job-validation-ui-tests
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@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.
Looks good overall, left a few suggestions.
x-pack/test/functional/apps/ml/anomaly_detection_jobs/single_metric_job.ts
Outdated
Show resolved
Hide resolved
x-pack/test/functional/apps/ml/anomaly_detection_jobs/single_metric_job.ts
Outdated
Show resolved
Hide resolved
x-pack/test/functional/apps/ml/anomaly_detection_jobs/single_metric_job.ts
Outdated
Show resolved
Hide resolved
x-pack/test/functional/apps/ml/anomaly_detection_jobs/single_metric_job.ts
Outdated
Show resolved
Hide resolved
x-pack/test/functional/apps/ml/anomaly_detection_jobs/single_metric_job.ts
Outdated
Show resolved
Hide resolved
x-pack/test/functional/apps/ml/anomaly_detection_jobs/single_metric_job.ts
Outdated
Show resolved
Hide resolved
x-pack/test/functional/apps/ml/anomaly_detection_jobs/single_metric_job.ts
Outdated
Show resolved
Hide resolved
…etric_job.ts Co-authored-by: Robert Oskamp <traeluki@gmail.com>
…etric_job.ts Co-authored-by: Robert Oskamp <traeluki@gmail.com>
…etric_job.ts Co-authored-by: Robert Oskamp <traeluki@gmail.com>
…etric_job.ts Co-authored-by: Robert Oskamp <traeluki@gmail.com>
…etric_job.ts Co-authored-by: Robert Oskamp <traeluki@gmail.com>
…etric_job.ts Co-authored-by: Robert Oskamp <traeluki@gmail.com>
Co-authored-by: Robert Oskamp <traeluki@gmail.com>
Co-authored-by: Robert Oskamp <traeluki@gmail.com>
Co-authored-by: Robert Oskamp <traeluki@gmail.com>
@elasticmachine merge upstream |
…our/kibana into ml-add-job-validation-ui-tests
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Async chunks
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
Summary
popperProps