refactor(code_block): remove all snapshot tests and redundant describe blocks #8185
Conversation
- remove the wrapping describe, - update the assertion to toHaveAttribute
weronikaolejniczak
changed the title
weronikaolejniczak
changed the title
packages/eui/.loki/reference/chrome_mobile_Editors_Syntax_EuiCodeBlock_Highlighted_Lines.png
Outdated
Show resolved
Hide resolved
| export const NoPaddingSize: Story = { | ||
| tags: ['vrt-only'], | ||
| args: { | ||
| children: htmlCode, | ||
| language: 'html', | ||
| paddingSize: 'none', | ||
| }, | ||
| }; | ||
|
|
||
| export const SmallPaddingSize: Story = { | ||
| tags: ['vrt-only'], | ||
| args: { | ||
| children: htmlCode, | ||
| language: 'html', | ||
| paddingSize: 's', | ||
| }, | ||
| }; | ||
|
|
||
| export const MediumPaddingSize: Story = { | ||
| tags: ['vrt-only'], | ||
| args: { | ||
| children: htmlCode, | ||
| language: 'html', | ||
| paddingSize: 'm', | ||
| }, | ||
| }; | ||
|
|
||
| export const LargePaddingSize: Story = { |
There was a problem hiding this comment.
I think my only hesitation with manually writing all these tests one-by-one is 1. it's not DRY, and 2. it's incredibly easy to miss a size. Ideally I'd prefer it if Loki could go through our controls and take a screenshot of each option, but I don't know if that's available yet.
My 2c: until we can address this holistically across all components I'd prefer not to add VRT for each prop and its options, but I'll defer to @mgadewoll on that
There was a problem hiding this comment.
I agree 💯. After writing these stories, I had the exact same thought: "Is it scalable, and is it maintainable?". My answer was "no and no". I decided to push this anyway to get your insights because maybe you'd have an idea on how we could improve it.
if Loki could go through our controls and take a screenshot of each option
There's no automated solution for testing all controls implicitly. The way Loki works, you have to create a story per each prop combination you want it to take a screenshot of. Besides, I don't think it's sensible for all of the controls to be visually tested for regression. Some do not have visual results or are not as crucial.
My 2c: until we can address this holistically across all components I'd prefer not to add VRT for each prop and its options
I can agree here. We can investigate this on a separate task and make a decision together. If not to somehow automate it, at least to establish the protocol of what type of prop combinations do we want to test. It's not without a cost after all.
We still have to test this interface somehow though. Snapshot tests - no, as we discussed before, unit tests - what, asserting a class? It's implementation details and very prone to breaking. VRT really seems to be the best suited for the job.
There was a problem hiding this comment.
I'd agree to not add the granular stories for VRT just now and instead we should first look into these things:
- a) is there a way to automate this better within Loki's limitations
- b) we eventually want/need to migrate off Loki considering it's non-maintained by now and we can't use it in CI, so adding additional VRT stories now might be unnecessary work if the new solution can handle this - hopefully - more elegantly
So if the question is: if we remove the snapshots here now, what do we do? Then maybe we don't remove the snapshots for those just yet? 🤔
Edit: Or we accept class/attribute assertions in unit tests for now until we can migrate to more fitting VRT.
There was a problem hiding this comment.
@cee-chen @mgadewoll how about I write those tests again but using assertions for classes? I'll remove the stories and we think about Loki separately?
There was a problem hiding this comment.
Yep, of course. 👍 We don't need to solve the Loki issue in this PR. It's a full grown investigation by itself.
| @@ -174,40 +130,38 @@ describe('EuiCodeBlock', () => { | |||
| {code} | |||
| </EuiCodeBlock> | |||
| ); | |||
| expect(container.firstChild).toMatchSnapshot(); | |||
|
|
|||
| expect(container).toBeInTheDocument(); | |||
There was a problem hiding this comment.
Depending on the size of the snapshot, I wouldn't hate this being left as a snapshot - it's not bad to get an idea of what kind of DOM our virtualization library renders and what inline styles it's adding.
If it's a crapton of lines that would cause people's eyes to glaze over however, I'm good with leaving it as an assertion:
| expect(container).toBeInTheDocument(); | |
| expect(container.querySelector('[class*="euiCodeBlock__code-isVirtualized"]')).toBeInTheDocument(); |
There was a problem hiding this comment.
Depending on the size of the snapshot, I wouldn't hate this being left as a snapshot - it's not bad to get an idea of what kind of DOM our virtualization library renders and what inline styles it's adding.
That's what I thought initially but after revisiting our discussion about snapshot tests, reading different opinions and reflecting on it myself, I came to a conclusion that testing the component structure is too prone to misinterpretation and human error while deciding if the diff is expected or not. It's easy to skip by just updating the snapshot. The core aspects we want to test (like rendering, important classes, or roles) can be tested with proper assertions.
Still, the suggestion you made ☝🏻 I don't like that we have to query by a class like that but there's no other way I see in a unit test and I do like that the class points more to whether the virtualization is applied or not. BUT I believe the better way to test it is with Cypress, so making sure only the visible items are rendered with EuiCodeBlock that has a isVirtualized prop set to true.
Let me know what you think and we decide together how it's better to test virtualization.
There was a problem hiding this comment.
Actually, I just added back the snapshot 😄 If this is useful nevertheless then it doesn't hurt to leave it while we remove all the other prop snapshots which are more tedious than useful 👍🏻
Motivation: to avoid scalability and maintenance issues
- add back snapshot tests for initial render and virtualization - simplify some assertions - add unit tests for the: transparentBackground, overflowHeight, paddingSize and fontSize
|
Preview staging links for this PR:
|
💚 Build Succeeded
History
|
|
|
||
| describe('whiteSpace', () => { | ||
| it('renders a pre block tag with a css class modifier', () => { | ||
| test.each<{ fontSize: EuiCodeBlockFontSize; expected: string }>([ |
There was a problem hiding this comment.
Amazing use of test.each! 👏
I really like this use of Jest's API - my only comment is that we do lose some automatic checking against source code by no longer importing PADDING_SIZES or FONT_SIZES (aka, if a dev adds a new size to those arrays and forgets to update tests, there's no automated check/failure to tell them they missed adding a test).
Overall though I think the much nicer syntax is worth it - maybe we can look into a way later (typescript or some other automation) to check the number of tests expected.
There was a problem hiding this comment.
@cee-chen actually, this suggestion comes from @mgadewoll (thank you again, Lene 🙌🏻)!
Using PADDING_SIZES and FONT_SIZES makes the tests less clear (what exactly they're testing, asserting and how many there are, you have to jump around the definitions) but as you say, at the same time we lose the "automatic check" benefit.
maybe we can look into a way later (typescript or some other automation) to check the number of tests expected.
This sounds very interesting! Noted ✏️
Summary
As discussed on #8176, the
EuiCodeBlockcomponent's testing suite could use improvements, mainly:describeblock over only one test case (comment),toMatchSnapshot(aka snapshot testing) to relevant assertions using Jest matchers (comment).Some additional comments
I added some stories (Annotations, Highlighted Lines and Start Value) to showcase the component's capabilities with
linesNumberprop. You cannot easily control it with the playground controls. Additionally, we can test the annotation behavior and have a VRT test.I removed all snapshots (except for initial render). A snapshot can still get overwritten without much notice, targeted assertions are better.
QA
Unit Tests
yarn workspace @elastic/eui test-unitoryarn workspace @elastic/eui test-unit code_block(to target only theEuiCodeBlockcomponent).Visual Regression Tests
yarn workspace @elastic/eui storybookto open the Storybook.yarn workspace @elastic/eui visual-regressionoryarn workspace @elastic/eui --storiesFilter EuiCodeBlock(to target only theEuiCodeBlockcomponent).