[Spike] EUI snapshot serializer #6769
Conversation
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_6769/ |
There was a problem hiding this comment.
@JasonStoltz @tkajtoch, I'd love your thoughts on this spike and how (or if) we should even move forward with it.
Questions that may be helpful to answer:
- Does this code (in particular the regexes) feel as fragile to you as they do to me?
- Is this something we want to keep moving forward with? Is this worth the time, or would maintaining this work cost us more time than simply updating snapshots with every upgrade?
| export { euiEnzymeShallowSnapshotSerializer } from './shallow'; | ||
| export { euiEnzymeMountSnapshotSerializer } from './mount'; | ||
| export { euiEnzymeRenderSnapshotSerializer } from './render'; |
There was a problem hiding this comment.
Just want to illustrate a large part of the problem - there's 4 different render methods (Enzyme has 3, RTL has 1) that need to be solved for when writing these serializers, which effectively quadruples the amount of work and testing needed.
| /** | ||
| * Snapshot serializer | ||
| */ | ||
| export const euiEnzymeShallowSnapshotSerializer = { |
There was a problem hiding this comment.
Shallow is the most robust of all the serializers, because... well.. it's basically already what we want 🥲 We wouldn't be in this situation if everyone in Kibana were using shallow snapshots however.
| export const euiRTLRenderSnapshotSerializer = { | ||
| test: (val: any) => { | ||
| if (val instanceof HTMLElement) { | ||
| return val.className?.startsWith('eui'); |
There was a problem hiding this comment.
TBH, I think even just taking the RTL render snapshot serializer alone is where you start to realize things are getting kinda dicey. Basically what I'm doing is regexing HTML which everyone knows is a wild path to go down.
It also feels somewhat fragile because it relies on us correctly setting className="euiSomeComponent" as a class hook/API on everything we render, which to be fair, we do do 90% of the time, but then don't for some things like nested DOM, internal components, etc.
| expect(rtlRender(pageTest).container.firstChild).toMatchInlineSnapshot(` | ||
| <EuiPageTemplate> | ||
| <main | ||
| class="emotion-euiPageInner" | ||
| id="EuiPageTemplateInner_generated-id" | ||
| > | ||
| <section | ||
| class="emotion-euiPageSection-l-top-transparent" |
There was a problem hiding this comment.
See above comment - the reason these aren't getting properly converted to, e.g. <EuiPageInner> or <EuiPageSection> is because of class naming convention 💀
| <div | ||
| data-focus-guard="true" | ||
| style="width: 1px; height: 0px; padding: 0px; overflow: hidden; position: fixed; top: 1px; left: 1px;" | ||
| tabindex="-1" | ||
| /> | ||
| <EuiDataGrid /> | ||
| <div | ||
| data-focus-guard="true" | ||
| style="width: 1px; height: 0px; padding: 0px; overflow: hidden; position: fixed; top: 1px; left: 1px;" | ||
| tabindex="-1" | ||
| /> |
There was a problem hiding this comment.
Just want to highlight this - components with focus traps or sibling components also add another vector to account for that's difficult to predict - we're starting to get into the business of writing serializers/regexes for 3rd party library DOM 😬
So, e.g., EuiModal and EuiFlyout etc will be super difficult to get a clean snapshot for.
| expect(render(gridTest)).toMatchInlineSnapshot(` | ||
| Array [ | ||
| <div | ||
| data-focus-guard="true" | ||
| style="width:1px;height:0px;padding:0;overflow:hidden;position:fixed;top:1px;left:1px" | ||
| tabindex="-1" | ||
| />, | ||
| <div | ||
| class="euiDataGrid__focusWrap" | ||
| data-focus-lock-disabled="disabled" | ||
| > |
There was a problem hiding this comment.
Also want to highlight this particular incredibly annoying quirk of Enzyme's/Cheerio's render - it's the only one of the render options that can return an array of nodes (not actual DOM nodes, mind you, but their pseudo node objects which can't actually be parsed as DOM/HTML...). This makes it so sibling scenarios need a for loop to parse, but also simultaneously super difficult to correctly parse individually because their API documentation is very lackluster and/or I'm just bad at using it 🥲
Otherwise, the EuiDataGrid component here would have printed less, but because the output is an array, it's completely getting skipped by the serializer
| } | ||
| }; | ||
|
|
||
| export const fixIndentation = (children: string, startingIndentLevel = 1) => { |
There was a problem hiding this comment.
As an aside, this function was probably unnecessary for the spike and super extra but I'm lowkey pretty proud of this indentation logic 😂
|
Spike done - we won't be moving forward with this approach. One idea for a different approach would be to try to evangelize Kibana to use fewer snapshots instead. |
Summary
This PR is a 1-2 day spike at writing an exportable set of EUI snapshot serializer. The goal of the serializer is to flatten EUI components in snapshots only (i.e., not full
jest.mock()scenarios that replacement the component in all test scenarios) to reduce exposure to the inner workings of our components (e.g. classes, styles, DOM). The reduced snapshotted DOM would ideally result in smaller downstream snapshot churn in Kibana, and as such fewer codeowner pings.This PR was opened mostly to preserve a copy of the spike code as well as provide a place where certain parts of the code can be discussed - it is not meant to be merged in.