Cleaning up some warnings from tests and React runtime #337
Conversation
…n in Material-UI. This isn't really all that useful in the test, but I find it better to test for this and expect it (and thereby hiding it from the test runner) than cluttering the test output.
… around this deprecated option, I concluded we might as well silence the warning during tests, but follow the situation.
…hComponent. This caused React to complain, as <h2> should not be a child of <p>. Changing the underlying HTML tag for directly descending headers to be <div> instead of the default <p>.
| />, | ||
| ); | ||
|
|
||
| expect(shallowParagraphComponent).toMatchSnapshot(); |
There was a problem hiding this comment.
Sorry to be a bit picky about this, but we should not introduce any new snapshot tests, as they are problematic for several reasons.
They do not test explicit behavior, and thus changing the logic to do something else may not break the test in the way you think. Sure, the test may be red, but unless you read the test title (and the test title makes sense, and is updated when test logic changes), you can just commit the updated snapshot without really noticing that the new behavior is incorrect. From earlier experience, snapshot tests have a tendency to just get updated without anyone really paying attention to if the diff is something to worry about or not.
Please change to explicit tests (should render <div> as root element when text contains a heading and should render <p> as root element when text content is a string f.ex).
There are probably other scenarios that we should fix here as well, since there are other elements that are not valid children of <p>.
There was a problem hiding this comment.
You shouldn't be sorry! I'm a complete newbie when it comes to snapshot testing, so I hadn't considered problems like these. I fully agree.
Yep, there's potentially lots of things that shouldn't be inside a <p>, but I think headers were the most common case here. Either this evolves into a huge complexity, or we should just use <div> by default when the text is a react element. What do you think?
There was a problem hiding this comment.
Thanks for updating it, looks much better now 😁
I think fixing the immediate problem (checking for headers) is enough for now. There are so many cases to handle that I'm not really sure the approach where Paragraph component accepts any content as children is the best. It should probably only accept a string. But changing that would be breaking, so not something we can do easily - but at least something we need to consider for the next major version.
I started a issue with considerations for our next major version, and added this topic.
|
Kudos, SonarCloud Quality Gate passed! |
Description
This silences some knows warnings in unit tests, and tests for them instead of outputting them to the console. In addition, it cleans up some warnings shown in the console when running some apps.
Verification
Documentation
[ ] User documentation is updated with a separate linked PR in altinn-studio-docs. (if applicable)