-
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
[Lens] Add functional tests on chart transitions and pie chart #74083
[Lens] Add functional tests on chart transitions and pie chart #74083
Conversation
86dc30a
to
ba7221c
Compare
02f797d
to
78cf61a
Compare
Pinging @elastic/kibana-app (Team:KibanaApp) |
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.
This looks great, thanks a lot! I added two small nits about asserting a little more, but even without that it's very helpful.
@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.
Overall looking good, but added some nitpicks that I'd like to get another iteration on
/** | ||
* Gets text of the specified datatable header cell | ||
* | ||
* @param index - index of th element in datatable |
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 don't typically use the position of the element to make assertions, could this be done in a less-brittle way?
* Gets text of the specified datatable cell | ||
* | ||
* @param rowIndex - index of row of the cell | ||
* @param colIndex - index of column of the cell |
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.
This looks brittle again, like I wouldn't want to reuse this function for other uses.
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.
How we were testing it before, was to check only the first element, so the index could have been not specified explicitly but it worked the same. I'm not exactly understanding what you mean by this being brittle and why you wouldn't reuse this function for other uses – you're more than welcome to write the example, please 🙏
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 was mostly expecting the selector to work backwards. Here, you provide the expected index and get the text at that index. Is there a way to provide the text, and get the index of the text?
@elasticmachine merge upstream |
a4c70fb
to
67f8497
Compare
💚 Build SucceededBuild metricspage load bundle size
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.
Overall LGTM, the discussion about index vs text selection is interesting but mostly nitpicky
Thanks @wylieconlon for your comment! I'll try to find some patterns of how to test tables, maybe there's a better way. |
Summary
Part of #72585
This PR adds tests that test the following functionalities:
Apart from that, I've refactored few methods (by making them more universal and moving them to lens pageObject) and moved dashboard specific tests to separate suite.
I ran the tests both in flaky-test-runner and cloud-flaky-test-runner:
data-test-subj
that I introduced in this code (eglnsPie_sliceByDimensionPanel
)