-
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
fix locally broken unit tests #149811
fix locally broken unit tests #149811
Conversation
Pinging @elastic/apm-ui (Team:APM) |
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
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, but:
It actually took me some time to figure out why a key
property is needed here since there's no loop to create this annotation. But I see that this is needed because we wrapped the result of getAnnotations
in []
. Maybe you cold remove this [getAnnotations()]
from here https://github.com/elastic/kibana/blob/main/x-pack/plugins/apm/public/components/shared/charts/timeseries_chart_with_context.tsx#L108, and return an array from the getAnnotations
itself. You could also convert to a standard function instead of an arrow function, this will avoid the TimeseriesChart
to render unnecessarily because of these two things.
@cauemarcondes Great catch on the performance improvement part. Updated the code |
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
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.
Nice job! Were you able to repro locally, or how did you figure this out?
@cauemarcondes Nice perf improvement 👍 |
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
## Summary Due to an old Pull request, 2 of the Unit Tests started to fail locally. Somehow they won't fail on the CI which made them tough to identify initially. This PR fixes those broken unit tests.
Summary
Due to an old Pull request, 2 of the Unit Tests started to fail locally. Somehow they won't fail on the CI which made them tough to identify initially.
This PR fixes those broken unit tests.