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

Can the Jest serializer list the original class names instead of removing the generated class names? #343

Closed
swese44 opened this issue Feb 17, 2023 · 4 comments
Labels
❓question Further information is requested

Comments

@swese44
Copy link
Contributor

swese44 commented Feb 17, 2023

Our repo relies heavily on component snapshots for unit tests. We're on FluentUI 8 so we have styles serialized into the snapshots, but we're not feeling confident about snapshots in FluentUI 9 + Griffel.

It would give us confidence to have a test like "With color blue it matches its snapshot" show the snapshot output with the original descriptive classes like:

<div className="myComponent myComponent_color_blue">content</div>

Currently that example component's snapshot tests with Griffel would not be very helpful:

With color blue it matches its snapshot: <div className="">content</div>
With color red it matches its snapshot: <div className="">content</div>
With color green it matches its snapshot: <div className="">content</div>

Is there a reason the serializer doesn't add the original class names, are the original classes difficult to access?

If this is a deliberate design choice, do you have a different recommendation to achieve confidence in unit tests? Note that we have visual tests for our presentational component library, but this is expensive to maintain and is not a scalable alternative for thousands of components which change often with business logic.

Thanks!

@layershifter
Copy link
Member

I am not sure to what classes you're referring 🐱 Let's try clarify 💪


makeStyles() produces Atomic CSS and atomic classes, i.e.:

const useClasses = makeStyles({
  root: {
    backgroundColor: 'red',
    paddingTop: '10px',
  },
});

function App() {
  const classes = useClasses();

  return <div className={classes.root} />
}

In this case the snapshot will look like:

<div class="___10kjqil f3xbvq9 f1809wu7"></div>

If you want to have these classes in snapshots, just don't use the serializer 😸 However, it seems that something else is needed...


Some partners were explicit about the requirement and asked us to remove atomic classes from snapshots, that's how the current behavior appeared in Jest serializer. It will produce following:

<div class=""></div>

The nice part: with this you can still assert for exact styles to be applied:

expect(element).toHaveStyle(`
  white-space: nowrap;
  overflow-x: hidden; 
  overflow-y: hidden;
`);

note: .toHaveStyle() is a part of @testing-library/jest-dom


However, the request is to implement following as I understood:

const useStyles = makeStyles({
  root: { color: 'red' },
  rootPrimary: { color: 'blue' },
});

function Component(props) {
  const classes = useStyles();

  return <div className={mergeClasses(classes.root, props.primary && classes.rootPrimary)} />;
}

That outputs following:

<div class="root rootPrimary"></div>

The problem is that from Griffel's perspective, slots aren't actually classnames ➡️ they are a logical grouping of styles. Let me provide a clearer example:

const useClassesA = makeStyles({
  root: { color: 'red' },
});
const useClassesB = makeStyles({
  root: { color: 'blue' },
});

function Component(props) {
  const classesA = useClassesA();
  const classesB = useClassesB();

  return <div className={mergeClasses(classesA.root, classesB.root)} />;
}

If there will be a such behavior built in, it will output following:

<div class="root root"></div>

Yup, double "root". That's a reason why we don't have it built in 🙄

Another issue with this is that it's very hard to restore a slot name from classes i.e. "___10kjqil f3xbvq9 f1809wu7" => "root" as these are just logical groups. Here is another example:

const useClasses = makeStyles({
  rootA: {
    backgroundColor: 'red',
    paddingTop: '10px',
  },
 rootB: {
    backgroundColor: 'red',
    paddingTop: '10px',
  },
});

function App() {
  const classes = useClasses();

  return (
    <div>
      <div className={classes.rootA} />
      <div className={classes.rootB} />
    </div>
   )
}

⬇️⬇️⬇️

<div>
  <div class="___10kjqil f3xbvq9 f1809wu7"></div>
  <div class="___10kjqil f3xbvq9 f1809wu7"></div>
</div>

However, if you still want to get this option working, you don't need to get anything from Griffel 😅 Just use a custom mock:

jest.mock("@griffel/react", () => {
  const actual = jest.requireActual("@griffel/react");

  return {
    ...actual,
    makeStyles: (stylesBySlots) => () => {
      return Object.keys(stylesBySlots).reduce((acc, curr) => {
        acc[curr] = curr;
        return acc;
      }, {});
    },
    mergeClasses: (...classes) =>  classes.filter(x => !!x).join(" "),
  };
});

Note: you can also put as a global mock, https://jestjs.io/docs/manual-mocks#mocking-user-modules

Another option is to have additional functionality in a serializer that will output applied styles:

expect(element.classList).toMatchInlineSnapshot(`
  white-space: nowrap;
  overflow-x: hidden; 
  overflow-y: hidden;
`);

However, we haven't any requests for that. Hope that it's clearer now, let us know what could work for you and matches your expectations 🐱

@layershifter layershifter added the ❓question Further information is requested label Feb 20, 2023
@swese44
Copy link
Contributor Author

swese44 commented Feb 21, 2023

@layershifter thank you for such a detailed explanation! My question was not very clear but your example nailed it. I think from a testing standpoint we want to verify that the component is applying the correct original classNames based on props/state, so the global mock should do the trick.

@rese
Copy link

rese commented Feb 22, 2023

Hi @layershifter, thank you so much for your detailed response. We went ahead with the global mock as part of our jest config and that definitely addressed our needs. Thank you so much for your time and guidance.

@layershifter
Copy link
Member

@swese44 @rese cool 👍

There is a motivation to not include that mock into Griffel, so I am going to close the issue as a workaround exists. Feel free to follow up if there will be issues 🤙

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
❓question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants