Fix warnings inside our tests

[danielduan]

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.

[syneva-runyan]

If no one's looking into this yet I can take a go at it

[danielduan]

@syneva-runyan if you can, that would be great. thanks!

[syneva-runyan]

@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

[danielduan]

I don't see any info statements in that file. Can you link it?

[dangreenisrael]

@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.

[Hypnosphi]

@dangreenisrael I think it's a good idea

[dangreenisrael]

@Hypnosphi Should this be onto master or release 3.3?

[danielduan]

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.

[dangreenisrael]

#2297

[syneva-runyan]

@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.

[danielduan]

@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.

[syneva-runyan]

Regarding a specific issue from tree/master/addons/info:

An Invalid DOM nesting error is being thrown in the first test, <pre> cannot be a decendant of <p>

This error comes from the following:

withInfo(
'# Test story \n## with markdown info \ncontaing bold, cursive text and code'
)

The provided markdown is rendered inside a <p> tag, as definied in text.js, <P>.

When the rest of the markdown parsed with marksy, the code portion of the info is rendered in a <pre> 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 <p> to render content inside of a <div> or other containing element. (markdown/text.js)
• Change <code> to render content inside a <span> 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

[danielduan]

It should probably just be a <div> if there aren't any huge regressions visually.

[syneva-runyan]

Raised a pull request, #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.

[dangreenisrael]

If this is merged into master, should the issue be closed? @Hypnosphi. This (#1138) seems to imply that we want to get issues closed on the sooner side.

[Hypnosphi]

Let's close it when the same is done for release/3.3

[syneva-runyan]

Put in pull request for the warnings in release/3.3 https://github.com/storybooks/storybook/pull/2381/files

[Hypnosphi]

Fixed with #2343 and #2381