-
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
[Lens] Add more chart editor tests based on the debug state #86750
Conversation
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.
Looks pretty good to me, just have one question about waiting for the chart to load
|
||
await PageObjects.lens.closeDimensionEditor(); | ||
|
||
await retry.tryForTime(3000, 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.
can we replace this with a await PageObjects.header.waitUntilLoadingHasFinished()
along with the other occurences below? That's also what's used in x-pack/test/functional/apps/lens/chart_data.ts
dimension: 'lnsXY_yDimensionPanel > lns-empty-dimension', | ||
operation: 'avg', | ||
field: 'bytes', | ||
}); |
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.
Could you load one of the existing pre-saved visualizations that are used by the other charts? Does it need to be the Terms agg?
expect( | ||
state.bars![0].bars.map((bar) => ({ | ||
x: bar.x, | ||
y: Math.round(bar.y * 100) / 100, |
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.
Could you comment on why this rounding is needed? Or maybe the expect
library has a number precision parameter?
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's because the data table rounds numbers to two decimal points, but the xy chart does not
assertMatchesExpectedData(data!); | ||
}); | ||
|
||
it.skip('should render pie chart', 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.
Please comment on the skipped tests
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Async chunks
Distributable file count
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
* master: (108 commits) [DOCS] Refreshes Data Visualizer screenshot (elastic#87017) [Security Solution] Change 'anti-virus' text to 'antivirus' (elastic#87001) [securitySolution/cypress] temporarily limit to PRs [AppServices/Examples] Add the example for Reporting integration (elastic#82091) [Build Chromium] Improve git checkout (elastic#83225) Deprecate `services.callCluster` in alerts and actions executors (elastic#86474) [Security Solution] Use system node version for Cypress and increase exec command timeout (elastic#86985) [Lens] Add more chart editor tests based on the debug state (elastic#86750) [Lens] Integrate searchSessionId into Lens app (elastic#86297) skip "pagination updates results and page number" elastic#86975 skip "Custom detection rules" elastic#83772 [logging/json] use merge from kbn/std (elastic#86330) skip network and timeline inspection. elastic#85677, elastic#85678 skip "adds correctly a filter to the global search bar" elastic#86552 [ftr/flags] improve help text (elastic#86971) skip "Fields Browser rendering. elastic#60209 skip "Closes and opens alerts" elastic#83773 [Security Solution] Skip failing Cypress tests (elastic#86967) Removes archives (elastic#86537) [ML] Fix charts grid on the Anomaly Explorer page (elastic#86904) ...
* master: (72 commits) [DOCS] Refreshes Data Visualizer screenshot (elastic#87017) [Security Solution] Change 'anti-virus' text to 'antivirus' (elastic#87001) [securitySolution/cypress] temporarily limit to PRs [AppServices/Examples] Add the example for Reporting integration (elastic#82091) [Build Chromium] Improve git checkout (elastic#83225) Deprecate `services.callCluster` in alerts and actions executors (elastic#86474) [Security Solution] Use system node version for Cypress and increase exec command timeout (elastic#86985) [Lens] Add more chart editor tests based on the debug state (elastic#86750) [Lens] Integrate searchSessionId into Lens app (elastic#86297) skip "pagination updates results and page number" elastic#86975 skip "Custom detection rules" elastic#83772 [logging/json] use merge from kbn/std (elastic#86330) skip network and timeline inspection. elastic#85677, elastic#85678 skip "adds correctly a filter to the global search bar" elastic#86552 [ftr/flags] improve help text (elastic#86971) skip "Fields Browser rendering. elastic#60209 skip "Closes and opens alerts" elastic#83773 [Security Solution] Skip failing Cypress tests (elastic#86967) Removes archives (elastic#86537) [ML] Fix charts grid on the Anomaly Explorer page (elastic#86904) ...
…86750) Co-authored-by: Joe Reuter <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync. |
Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync. |
1 similar comment
Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync. |
Summary
This PR is a rebased version of #84502 with more testing for chart XY axis.
Checklist