-
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
[Uptime] Fix breadcrumb for link back to monitor from waterfall page #87125
Conversation
💚 Build SucceededMetrics [docs]Module Count
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.
Looks great! Just a question about testing.
|
||
mountWithRouter( | ||
<MountWithReduxProvider> | ||
<KibanaContextProvider services={{ ...core }}> |
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.
Are there any helpers available for this type of wrapper? I know that in Observability, there is a test helper that includes these wrappers for react-testing-library. It seems like these wrappers are pretty common in Uptime tests, so a helper would be useful.
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 is no standard here, each developer just uses everything they seems fit, it's has been on my todo list to simplify the helper we have in uptime
x-pack/plugins/uptime/public/lib/helper/helper_with_router.tsx
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.
Andrew was concerned about this on one of my PRs during the break. Should we make an issue to cover this? Perhaps I can take a look at it since I may have less context to work on some of the larger bugs popping up for 7.11. It would be great if the helper was not opinionated, working only with a specific testing library, but rather spit out a component to be rendered with whatever library we choose.
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.
Yes, sure, that would be very nice. Just create an issue and go ahead.
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.
@dominiqueclarke for wrapper ideally i think we should use https://enzymejs.github.io/enzyme/docs/api/shallow.html#arguments
wrappingComponent prop, that means that part will also not go into snapshot and most use cases can be covered by shallow rendering.
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.
Anyway, let's continue this discussion in proper issue.
</MountWithReduxProvider> | ||
); | ||
|
||
expect(getBreadcrumbs()).toMatchInlineSnapshot(` |
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.
Another option could be to use react testing library to grab each breadcrumb element by text ("Uptime"), and check that the href is the expected.
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.
Thinking about this now, I'm realizing I'm wrong, since this is testing the output of the hook, not a specific render. Would the breadcrumbs html be rendered at all in this test?
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.
so breadcrumb are rendered outside uptime app, so testing via that means we will have to mount the whole app.
Pinging @elastic/uptime (Team:uptime) |
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 great! Just hoping we can continue the conversation re: testing utils!
Summary
Resolves: #86617
Fix breadcrumb for link back to monitor from waterfall page