Snapshotting logic does not provide an adequate className in a testing environment. #14357
Comments
|
Tests are failing on my pull, and solving them is a bit murky. Another solution I would like to try is passing an option to Seems like a good opportunity to do so but the context does not stay the same, and this is an old context API. I also see two |
Why is it not appropriate? When testing you mock, fake, setup etc all sorts of things. Whether you wrap your tests in a custom JSSProvider or change the className logic for test environments inside the implementation changes nothing about the fact that you don't "mirror native implementation". emotion has Seems like it would be more consistent with the ecosystem if we would release our own package for snapshot testing instead of changing the implementation for everyone. Maybe there exists already snapshot serializer that makes the classnames in snapshots deterministic and is framework agnostic. |
I don't. There is no precedent in the ecosystem and an existing solution. |
|
@eps1lon What do you mean by existing solution? I believe that we would have to build our own serializer package. We could very well decide to stop investing time and ressource in JSS and to start looking into a first class integration with emotion or styled-components? |
|
Wrapping in a |
|
Don't know if it can help @oliviertassinari I actually use this mock const styles = jest.requireActual('@material-ui/core/styles');
const theme = styles.createMuiTheme();
const withStyles = style => component => {
const classes = typeof style === 'function' ? style(theme) : style;
component.defaultProps = { ...component.defaultProps, classes };
return component;
};
module.exports = { ...styles, withStyles };This way I have snapshot like this one I prefer having the css properties instead of a classname to detect UI regressions. With the v4 I now use this mock import theme from '../../src/theme/muiTheme';
const styles = jest.requireActual('@material-ui/styles');
const makeStyles = style => props => {
// Apply theme to classes
const classes = typeof style === 'function' ? style(theme) : style;
// Apply props to every key of each class, which is every key of classes
const classesByProps = {};
Object.keys(classes).forEach(classKey => {
const classByProps = {};
Object.keys(classes[classKey]).forEach(key => {
classByProps[key] =
typeof classes[classKey][key] === 'function' ? classes[classKey][key](props) : classes[classKey][key];
});
classesByProps[classKey] = classByProps;
});
return classesByProps;
};
module.exports = { ...styles, makeStyles }; |
|
@VincentLanglet Thanks, it helps. |
|
@oliviertassinari No problem. const useStyles = makeStyles(
(theme) => ({
option: (props) => ({
backgroundColor: props.isRed ? 'red' : '',
}),
}),
);Instead of const useStyles = makeStyles(
(theme) => ({
option: ({
backgroundColor: (props) => props.isRed ? 'red' : '',
}),
}),
);Because of issue with the actual types definitions of StyleRule/CSSProperties, ... I now use this (simpler) mock import theme from '../../src/theme/muiTheme';
const styles = jest.requireActual('@material-ui/styles');
const makeStyles = style => props => {
// Apply theme to classes
const classes = typeof style === 'function' ? style(theme) : style;
// Apply props to every class, which is every key of classes
const classesByProps = {};
Object.keys(classes).forEach(classKey => {
classesByProps[classKey] = typeof classes[classKey] === 'function' ? classes[classKey](props) : classes[classKey];
});
return classesByProps;
};
module.exports = { ...styles, makeStyles }; |
|
Any progress on this issue? |
|
@andrey-semin |
|
@VincentLanglet how would you import createMuiTheme inside your theme? I'm getting the following error since i'm assuming '@material-ui/styles' is being mocked and the '@material-ui/core/styles' is importing '@material-ui/styles Looks like it's failing on _styles.withThemeCreator in withTheme.js |
|
@Kaustix I did not But if you want to import a non-mocked function, use |
|
@VincentLanglet sorry don't think i explained very well. I'm importing createMuiTheme inside my themes.js which is causing the issue so not sure how to fix the error. causes the error: |
|
It's been month I didn't touch my project. This is what I had I see you wrote |
|
@VincentLanglet thank you so much!! since the latter was going after index.js which brought '@material-ui/styles' |
|
I'm going to bump this. This is one of the major cons of using material-ui. My team are currently building a design system based on I looked into Would that be a possibility? |
|
@Thisen Did you try the solution proposed by @VincentLanglet? |
|
@oliviertassinari, I don't want a mock, I want an actual serializer for Jest. Can't we expose the stylesheet as PRIVATE, like styled-components? |
|
@Thisen see #6115, people using |
|
@oliviertassinari What does mean that you're going to deprecate |
|
@Thisen we will try to move the maintainance of the package to react-jss. We will work on making the migration is as easy as possible. |
|
I'm totally agree with @Thisen, ideally we should have serializer for Jest, like But as a temporal solution, I modified workaround from @VincentLanglet and mock as a result, a don't want to generate styles object and put it to // __mocks__/@material-ui/styles.js
import theme from '../../muiTheme';
const styles = jest.requireActual('@material-ui/styles');
const makeStyles = (style, options) => () => {
// Apply theme to classes
const classes = typeof style === 'function' ? style(theme) : style;
// Generate class name with correct component name without any index
const classesByProps = {};
Object.keys(classes).forEach((key) => {
classesByProps[key] = options ? `${options.name}-${key}` : key;
});
return classesByProps;
};
const withStyles = (style, options) => {
const useStyles = makeStyles(style, options);
return (Component) => {
const classes = useStyles();
const MockComponent = (props) => (
<Component classes={classes} {...props} />
);
MockComponent.displayName = (options && options.name) || Component.displayName;
return MockComponent;
};
};
module.exports = {
...styles,
makeStyles,
withStyles,
};Don't forget to add const useStyles = makeStyles((theme) => ({
root: {
backgroundColor: theme.palette.primary.main,
},
}), { name: 'Button' });export default withStyles((theme) => ({
root: {
backgroundColor: theme.palette.primary.main,
},
}), { name: 'Button' })(MuiButton);as mentioned by @Kaustix, we need to import as a result, we have generated snapshot: |
|
I took an approach where I mocked @testing-library/react to return all render results with StyleProvider and the generateClassName function that takes away the numerical suffixes Some random extra notes here |
|
I am fine with using a custom generateClassName that does not include the numbers and I am already doing that. That said, the ORDER of the class names is ALSO coming out different on CI vs on my Dev machine. |
|
Using the code snippets provided in this thread, I had to make a couple of tweaks to get Hope this helps someone! |
|
Guys, I'm using this as But I'm having a prop-types error since I'm using a I could manage to fix it by using What am I missing to get an snapshot like this? Any help would be appreciated, thanks in advance. |
|
Just adding my opinion that this is a major time suck. Hours of Googling across multiple days. Trying and failing to find a solution to get consistent snapshots. I finally find this and #9492 only to see it's been an issue for 3 years and it seems core devs think this is fine. I want my 3 days back. Instead I now have to figure out which of the hacks on this page may minimize additional time required. |
|
@michaelpward I'm sorry to hear that you such a frustrating experience. #9492 (comment) is an example implementation that should cover 90% of the use cases. What did you find lacking from this example? The reason why we're hesitant to add documentation for this is that we
Considering the number of upvotes and that nobody released a helper package is indicative that this is either already sufficiently solved by createGenerateClassName or low popularity of snapshot testing. This means that isn't a priority for us to address. |
I can get a pretty output if I use react-test-renderer: |
|
@effortlesscoding your example doesn't include withStyles though? that is the main cause of the issue here |
|
Hi devs, I'm having issues with the generated class names too. But I noticed that the difference of a new class name appears to be consistent, not a random generated each time you run a test. So I took a quick look to const suffix = `${rule.key}-${ruleCounter}`;So I please correct me if I'm wrong @oliviertassinari:
If that's the case, maybe having those diffs in the snapshots is not so bad 🤔 |
|
@hisapy I think that the suggestion by @VincentLanglet is the best so far on this thread. At this point, we have a working workaround, we should focus on improving the DX. Ideally, the problem would be handled by default. But if not possible, we would combine some code + documentation. |
|
Thx! For the moment, if the generated suffix in the className is not random then I think I can live with it for a while 😄 |
|
In case anyone is interested in another approach, I managed to remove the incremental suffix using a snapshot serialiser. First, I followed the solution proposed by @cmdcolin here. But it does not work fine with The problem lies in the ...stuff
<button
aria-label="Delete"
class="MuiButtonBase-root-8 MuiIconButton-root-2 CustomButton-root"
tabindex="0"
type="button"
>
...more stuffBased on how const KEY = '_mui_cleaned_'
const getNodes = (node, nodes = []) => {
if (typeof node === 'object') {
nodes.push(node)
}
if (node.children) {
Array.from(node.children).forEach(child => getNodes(child, nodes))
}
return nodes
}
const getClassNamesFromDOM = node => Array.from(node.classList)
const getClassNamesFromProps = node => {
const classNameProp = node.props && (node.props.class || node.props.className)
if (classNameProp) {
return classNameProp.trim().split(/\s+/)
}
return []
}
const getClassNames = nodes =>
nodes.reduce((classNames, node) => {
let newClassNames = null
if (global.Element && node instanceof global.Element) {
newClassNames = getClassNamesFromDOM(node)
} else {
newClassNames = getClassNamesFromProps(node)
}
newClassNames.forEach(className => classNames.add(className))
return classNames
}, new Set())
const markNodes = nodes => nodes.forEach(node => (node[KEY] = true))
const removeIncrementalSuffix = (code, classNames) =>
Array.from(classNames)
.filter(className => className.match(/-\d+$/))
.reduce(
(acc, val) => acc.replace(val, val.slice(0, val.lastIndexOf('-'))),
code
)
expect.addSnapshotSerializer({
test: function(val) {
return (
val &&
!val[KEY] &&
(val.$$typeof === Symbol.for('react.test.json') ||
(global.Element && val instanceof global.Element))
)
},
print: function(val, print) {
const nodes = getNodes(val)
const classNames = getClassNames(nodes)
markNodes(nodes)
return removeIncrementalSuffix(print(val), classNames)
}
})Now the snapshot looks like this: ...stuff
<button
aria-label="Delete"
class="MuiButtonBase-root MuiIconButton-root CustomButton-root"
tabindex="0"
type="button"
>
...more stuffHope it helps. Buena suerte. |
Thanks for this. 👍 Passing a string to <Styled(WithStyles(ForwardRef(Container)))>
<WithStyles(ForwardRef(Container))
className="WithStyles(ForwardRef(Container))-root-120 WithStyles(ForwardRef(Container))-root-249"
>
<ForwardRef(Container)
className="WithStyles(ForwardRef(Container))-root-120 WithStyles(ForwardRef(Container))-root-249"
>
<div
className="MuiContainer-root WithStyles(ForwardRef(Container))-root-120 WithStyles(ForwardRef(Container))-root-249 MuiContainer-maxWidthLg"Since // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions#Escaping
const escapeRegExp = (string) => string.replace(/[.*+\-?^${}()|[\]\\]/g, '\\$&');
const removeIncrementalSuffix = (code, classNames) => Array.from(classNames)
.filter((className) => className.match(/-\d+$/))
.reduce(
(acc, val) => {
const exp = new RegExp(escapeRegExp(val), 'g');
return acc.replace(exp, val.slice(0, val.lastIndexOf('-')));
},
code,
); |
|
@emoriarty Great idea for components using the legacy withStyles and thank you @jalovatt for the regex change. For me though I am still not having consistent snapshots. I'm also generating classnames too without a counter appended. I'm at the point, just like others have said that I am going to say pencils down and just walk away from snapshots as I've unfortunately spent entirely too much time on this. It's just not worth the headache here and if I end up writing more functional tests then that's even better. I'm sure at some point this will be solved for 100% of use cases, or something new will come along. All the best. |
Thanks much for sharing it. Still found it helpful.
|
|
For those still trying to solve this, I wrote a utility that loops through all children of a rendered tree and regex replace class names to remove the integers on the end. I'd rather have this as part of a mock function, but I preferred having style names match prod exactly but without number. Then in a snapshot, import the utility and: |
|
@angusmccloud : this worked with minor adjustments, thanks so much! const cleanClasses = (tree: any) => {
const r = new RegExp("-[0-9a-zA-Z]{1,}( .*)?", "g");
// There are occasions where MUI has a "null" child or entire tree, skip those for our purposes
if(tree) {
// Some item trees are returned as arrays instead of objects
// If it's an array, make sure we clean each item in that array
if(tree.length > 1) {
for (let i = 0; i < tree.length; i++) {
cleanClasses(tree[i]);
}
}
// If this level of the tree has children
// Clean the children's className
// And pass any children of that child back to this function
if(tree.children) {
for(let i = 0; i < tree.children.length; i++) {
if(tree.children[i] && tree.children[i].className) {
try
{
tree.children[i].className = tree.children[i].className.replace(r, '');
} catch (e) {
}
}
if(tree.children[i].children && tree.children[i].children) {
for(let ii = 0; ii < tree.children[i].children.length; ii++) {
cleanClasses(tree.children[i].children[ii]);
}
}
}
}
// Clean the className of the current item in the nest
// Makes sure the top-level is always covered
if(tree && tree.className) {
try
{
tree.className = tree.className.replace(r, '');
}
catch (e) {}
}
}
// Return the cleaned tree
return tree;
}
export default cleanClasses; |
|
Closing as in v5, we rely on emotion and styled-components that have their own plugin https://emotion.sh/docs/testing, https://github.com/styled-components/jest-styled-components. |
|
@oliviertassinari I'm not sure I understand. If I use material-ui components, I can use emotion testing? |
and others
This was a previously #9492 however it does not actually provide a fix for the core problem which is that in testing environments we should be able to
mount(<Component>)that are created fromwithStyles()and have snapshots be reliable without wrapping them.Expected Behavior 🤔
Snapshots should create reliable labels for withStyles() used by both custom and defaut material-ui components like in the red portion of this Snapshot:
Current Behavior 😯
One can modify a custom component to have
However, this only modifies overwritten styles in material UI, default ones are actually not modified at all:
## Steps to Reproduce 🕹 This can't be easily reproduced in a sandbox environment. Link:ruleCounterincreases.Context 🔦
I would like to be able to mount() and call Jest's native
.toMatchSnapshot()without worrying about the ruleCounter adding random numbers. It is not appropriate to wrap every object in a as stated in the docs of https://material-ui.com/customization/css-in-js/#creategenerateclassname-options-class-name-generator because tests should mirror their native implementation.Your Environment 🌎
Ma
The text was updated successfully, but these errors were encountered: