-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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] Transforms: Adds functional tests for transform cloning and editing. #69933
[ML] Transforms: Adds functional tests for transform cloning and editing. #69933
Conversation
Pinging @elastic/ml-ui (:ml) |
LGTM |
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.
Great to see functional tests for transform cloning and editing coming! 🎉
Left a few comments.
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.
A few minor text edits needed, but otherwise LGTM
expect(rows.filter((row) => row.id === transformConfig.id)).to.have.length(1); | ||
}); | ||
|
||
it('should display details for the update job in the job list', async () => { |
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.
It isn't clear to me what this test is for from the description, which refers to job
and job list
. Looks like it is checking each of the fields in the row for the updated transform. In which case, something like should display the updated fields for the transform in the transforms list
?
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.
Updated in a037c8d. It now says should display the updated job in the job list row cells
.
const testSubjects = getService('testSubjects'); | ||
|
||
return { | ||
async assertTransfromEditFlyoutExists() { |
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.
Typo! Should be assertTransformEditFlyoutExists
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.
Fixed in a2613ee.
await testSubjects.existOrFail('transformEditFlyout'); | ||
}, | ||
|
||
async assertTransfromEditFlyoutMissing() { |
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.
Typo! Should be assertTransformEditFlyoutMissing
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.
Fixed in a2613ee.
); | ||
expect(actualValue).to.eql( | ||
expectedValue, | ||
`Transform ${input} input text should be '${expectedValue}' (got '${actualValue}')` |
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.
As well as the suggested edit, should start with should
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.
@peteharverson I'm not sure I get your suggestion right. How do you think this expect
message should look like?
We usually have messages in the form of Expected xyz (got abc)
(example) or xyz should be ... (got abc)
(example).
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.
Sorry, you're right @pheyos , I was thinking this was one of test descriptions. Agree we should stick with the same format for these expect
messages.
Co-authored-by: Robert Oskamp <[email protected]>
Co-authored-by: Robert Oskamp <[email protected]>
Co-authored-by: Robert Oskamp <[email protected]>
…ana into ml-e2e-transform-clone-edit
Checked test stability in a flaky test runner CI job -> No failure in 50 runs ✔️ |
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 - great stuff and thanks for adding those tests @walterra! 🎉
💚 Build SucceededBuild metrics
History
To update your PR or re-run it, just comment with: |
…ing. (elastic#69933) Adds functional tests for transform cloning and editing.
Summary
Adds functional tests for transform cloning and editing.
Checklist