Storybook: Fix warnings inside our tests
When running tests for cra-kitchen-sink, we have lots of warnings and messages thrown for react things.
Some of them are simply fixing the usage examples, some are resolving things react likes to complain about.
It'd be nice to have a clean test.
danielduan
All 18 comments
If no one's looking into this yet I can take a go at it
syneva-runyan
on 8 Nov 2017
@syneva-runyan if you can, that would be great. thanks!
danielduan
on 9 Nov 2017
@danielduan are the console.info statements good to be left in? For example, the ones that print out from app/react/src/server/babel_config.test.js
syneva-runyan
on 11 Nov 2017
I don't see any info statements in that file. Can you link it?
danielduan
on 11 Nov 2017
@danielduan @syneva-runyan Any thoughts on making console.error and console.warn fail tests? I could put together a PR for it, and they it can be merged in after @syneva-runyan gets rid of all the warnings.
dangreenisrael
on 12 Nov 2017
@dangreenisrael I think it's a good idea
Hypnosphi
on 12 Nov 2017
@Hypnosphi Should this be onto master or release 3.3?
dangreenisrael
on 12 Nov 2017
I think tests that don't significantly impact the code we ship to users can be put on master.
In general, bug fixes and developer oriented tooling can go on master.
danielduan
on 12 Nov 2017
dangreenisrael
on 12 Nov 2017
@danielduan, line 86 in app/react/src/server/babel_config.js for instance prints out => Loading custom .babelrc
logger.info('=> Loading custom .babelrc');
We could mock the logger so the messages don't show up in the console when running tests.
syneva-runyan
on 14 Nov 2017
@syneva-runyan that would be great, thanks!
I think a todo in the future would be to have a silent mode for webpack and babel so these info outputs can be turned off for CI builds to avoid polluting logs too.
danielduan
on 14 Nov 2017
Regarding a specific issue from tree/master/addons/info:
An Invalid DOM nesting error is being thrown in the first test, \
cannot be a decendant of \This error comes from the following:
withInfo(
'# Test story \n## with markdown info \ncontaing bold, cursive text andcode'
)The provided markdown is rendered inside a \
tag, as definied in text.js, \
.
When the rest of the markdown parsed with marksy, the
codeportion of the info is rendered in a \tag, which causes the invalid DOM nesting error.A similar error would be thrown if a link was provided in the markdown, ex
withInfo(
'# Test story \n## with markdown info \ncontaing [I'm an inline-style link with title]'
)Possible solutions to this problem would be:
- Change \
to render content inside of a \
or other containing element. (markdown/text.js)- Change \
to render content inside a \ or other element. (markdown/code.js) (Would not fix the link scenario)- A technical solution involving understanding what markdown is being passed to component and responding appropriately or throwing an error.
Thoughts on the best approach here? @danielduan
- Change \
It should probably just be a <div> if there aren't any huge regressions visually.
danielduan
on 14 Nov 2017
Raised a pull request, https://github.com/storybooks/storybook/pull/2343 Sorry for taking so long!
Regarding warning from addons/info/scr/index.test.js line 10:
I thought about putting in a fix for an issue that came up here where using marksy to output a function (rather than a string) spat out a warning, but figured that may be a separate issue (or maybe a marksy issue?) to deal with. Instead, I've added a .toString() to the passed function, suppressing the warning for now.
syneva-runyan
on 20 Nov 2017
If this is merged into master, should the issue be closed? @Hypnosphi. This (https://github.com/storybooks/storybook/issues/1138) seems to imply that we want to get issues closed on the sooner side.
dangreenisrael
on 21 Nov 2017
Let's close it when the same is done for release/3.3
Hypnosphi
on 21 Nov 2017
Put in pull request for the warnings in release/3.3 https://github.com/storybooks/storybook/pull/2381/files
syneva-runyan
on 27 Nov 2017
Fixed with #2343 and #2381
Hypnosphi
on 30 Nov 2017
Related issues
shilman
路
3Comments
sakulstra
路
3Comments
ZigGreen
路
3Comments
shilman
路
3Comments
oriSomething
路
3Comments
Most helpful comment
@syneva-runyan that would be great, thanks!
I think a todo in the future would be to have a silent mode for webpack and babel so these info outputs can be turned off for CI builds to avoid polluting logs too.