Skip to content
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 svgr in storybook & jest #1428

Merged
merged 7 commits into from
May 16, 2022
Merged

Fix svgr in storybook & jest #1428

merged 7 commits into from
May 16, 2022

Conversation

EnzoVezzaro
Copy link
Contributor

Fixes #

Changes proposed in this PR:

  • fix svg loader in storybook

@vercel
Copy link

vercel bot commented May 13, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
market ✅ Ready (Inspect) Visit Preview May 16, 2022 at 3:49PM (UTC)
1 Ignored Deployment
Name Status Preview Updated
market-v3 ⬜️ Ignored (Inspect) May 16, 2022 at 3:49PM (UTC)

@EnzoVezzaro EnzoVezzaro changed the title fix svg loader fix svg loader on storybook May 13, 2022
@EnzoVezzaro EnzoVezzaro mentioned this pull request May 13, 2022
17 tasks
@EnzoVezzaro EnzoVezzaro marked this pull request as draft May 13, 2022 12:44
@EnzoVezzaro
Copy link
Contributor Author

moved to draft as tests are failing.

Screen Shot 2022-05-13 at 8 44 07 AM

@kremalicious
Copy link
Contributor

also tinkering around on this but no luck so far. What I don't like is that the tests don't fail in the end although they clearly should. It looks like all the storyshots will succeed even if they spit out console errors.

Quickly tried minimal Logo test, and this fails successfully with the same error where the storyshots don't fail:

import React from 'react'
import { render } from '@testing-library/react'
import Logo from './'

test('Logo', async () => {
  render(<Logo />)
})

So getting the test run to fail in this current state would be another goal

@EnzoVezzaro
Copy link
Contributor Author

EnzoVezzaro commented May 16, 2022

So getting the test run to fail in this current state would be another goal

yeah, this is cra! 😬 ok 👍🏽

@kremalicious kremalicious added this to the Unit Testing milestone May 16, 2022
@EnzoVezzaro
Copy link
Contributor Author

EnzoVezzaro commented May 16, 2022

Still no luck on this. I tried so many way to do the same thing, but nothing seems to be working. I'm sure at the end it'll be a trivial setup issue (hopefully), but all of those issues related to the same thing is kind of discouraging though. 😪

@kremalicious
Copy link
Contributor

kremalicious commented May 16, 2022

I'm sure at the end it'll be a trivial setup issue

BINGO!

I was deep in next/jest Vercel-Land to verify we are not just overwriting but appending config things, then after hours realized that maybe the moduleNameMapper rules are matched in specific order cause a matched thing can't be matched twice. And in our case svg rule clashed with the @images rule. Just moved the svg rule to the very top, so far could not find any unintended consequences for this.

No console errors and my dummy Logo test was green. Still concerned that Jest + Storybook didn't error out as it did Jest + Testing Library alone but that can be solved somewhere else. Was quickly dropping in https://github.com/ValentinH/jest-fail-on-console to see if I can get the Jest + Storybook combination to fail with console errors but that didn't work either.

@kremalicious kremalicious changed the title fix svg loader on storybook Fix svgr in storybook & jest May 16, 2022
@kremalicious kremalicious marked this pull request as ready for review May 16, 2022 15:49
@codeclimate
Copy link

codeclimate bot commented May 16, 2022

Code Climate has analyzed commit 7a44b55 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 0.7% (0.0% change).

View more on Code Climate.

@EnzoVezzaro
Copy link
Contributor Author

EnzoVezzaro commented May 16, 2022

oh boy, finally. I confirm the test is passing now.

Copy link
Contributor

@kremalicious kremalicious left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome! And as usual lots of research needed to get to a code change which is quite small in the end

Copy link
Contributor

@claudiaHash claudiaHash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

@kremalicious kremalicious merged commit 593039f into v4 May 16, 2022
@kremalicious kremalicious deleted the fix/fix-svg-loader-storybook branch May 16, 2022 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants