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

Storybook: Add StoryShots integration to generate unit tests #18031

Merged
merged 6 commits into from Nov 26, 2019

Conversation

@gziolo
Copy link
Member

gziolo commented Oct 18, 2019

Description

Part of #17973.
Depends on #18030.

StoryShots adds automatic Jest Snapshot Testing for Storybook.

I followed the docs at this page:
https://github.com/storybookjs/storybook/tree/master/addons/storyshots/storyshots-core

Testing

Run npm run test-unit packages/components/storybook/

Known issue

There is a known issue with using React hooks with components in stories:
storybookjs/storybook#8177

I'm seeing the following:

  ● @wordpress/components › Checkbox Control › Default

    Invariant Violation: Invalid hook call. Hooks can only be called inside of the body of a function component. This could happen for one of the following reasons:
    1. You might have mismatching versions of React and the renderer (such as React DOM)
    2. You might be breaking the Rules of Hooks
    3. You might have more than one copy of React in the same app
    See https://fb.me/react-invalid-hook-call for tips about how to debug and fix this problem.

      17 | 
      18 | export const _default = () => {
    > 19 |      const [ isChecked, setChecked ] = useState( true );
         |                                        ^
      20 |      return (
      21 |              <CheckboxControl
      22 |                      label="Is author"

This issue was resolved by refactoring #18030. This PR was rebased with update/storybook-enhancements to solve it here as well.

@gziolo gziolo requested review from mkaz and epiqueras Oct 18, 2019
@gziolo gziolo self-assigned this Oct 18, 2019
@gziolo

This comment has been minimized.

Copy link
Member Author

gziolo commented Oct 21, 2019

I figured out that some tests can't work in this setup. ClipboardButton is one of them:
Screen Shot 2019-10-21 at 14 38 54

We can skip such stories using the convention. In this case, it would be DontTest keyword in the title. It shows up in the UI at the moment, but it should be easy to introduce a helper function which appends this pattern only in the test env.

@gziolo

This comment has been minimized.

Copy link
Member Author

gziolo commented Oct 21, 2019

cc @diegohaz and @ItsJonQ who recently spent some time on writing unit tests for UI components. I'd love to hear your feedback on the presented approach.

@epiqueras

This comment has been minimized.

Copy link
Contributor

epiqueras commented Oct 21, 2019

I figured out that some tests can't work in this setup. ClipboardButton is one of them:

What makes it break? Is there anyway we can mock it?

In this case, it would be DontTest keyword in the title.

We can also just add an explicit list of (?!) to the storyKindRegex since the list will probably be somehow limited.

@gziolo gziolo force-pushed the update/storybook-enhancements branch 2 times, most recently from 8249379 to cceee54 Oct 22, 2019
@gziolo

This comment has been minimized.

Copy link
Member Author

gziolo commented Oct 22, 2019

I figured out that some tests can't work in this setup. ClipboardButton is one of them:

What makes it break? Is there anyway we can mock it?

In this case, it would be DontTest keyword in the title.

We can also just add an explicit list of (?!) to the storyKindRegex since the list will probably be somehow limited.

I figured out that some tests can't work in this setup. ClipboardButton is one of them:

This is the full error:

  ● @wordpress/components › ClipboardButton › Default

    TypeError: Cannot read property 'firstChild' of null

      27 |      componentDidMount() {
      28 |              const { container, getText, onCopy } = this;
    > 29 |              const button = container.firstChild;
         |                                       ^
      30 | 
      31 |              this.clipboard = new Clipboard( button, {
      32 |                      text: getText,

      at ClipboardButton.firstChild [as componentDidMount] (packages/components/src/clipboard-button/index.js:29:28)
      at commitLifeCycles (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:10930:22)
      at commitLayoutEffects (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:13583:7)
      at HTMLUnknownElement.callCallback (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:10487:14)
      at invokeEventListeners (node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:193:27)
      at HTMLUnknownElementImpl._dispatch (node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:119:9)
      at HTMLUnknownElementImpl.dispatchEvent (node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:82:17)
      at HTMLUnknownElementImpl.dispatchEvent (node_modules/jsdom/lib/jsdom/living/nodes/HTMLElement-impl.js:30:27)
      at HTMLUnknownElement.dispatchEvent (node_modules/jsdom/lib/jsdom/living/generated/EventTarget.js:157:21)
      at Object.invokeGuardedCallbackDev (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:10537:16)
      at invokeGuardedCallback (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:10590:31)
      at commitRootImpl (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:13355:9)
      at unstable_runWithPriority (node_modules/scheduler/cjs/scheduler.development.js:643:12)
      at runWithPriority (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:1887:10)
      at commitRoot (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:13184:3)
      at runRootCallback (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:12420:20)
      at node_modules/react-test-renderer/cjs/react-test-renderer.development.js:1935:24
      at unstable_runWithPriority (node_modules/scheduler/cjs/scheduler.development.js:643:12)
      at runWithPriority (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:1887:10)
      at flushSyncCallbackQueueImpl (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:1931:7)
      at flushSyncCallbackQueue (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:1920:3)
      at scheduleUpdateOnFiber (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:12297:9)
      at scheduleRootUpdate (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:15087:3)
      at updateContainerAtExpirationTime (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:15115:10)
      at updateContainer (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:15134:10)
      at create (node_modules/react-test-renderer/cjs/react-test-renderer.development.js:15824:5)
      at getRenderedTree (node_modules/@storybook/addon-storyshots/dist/frameworks/react/renderTree.js:24:16)
      at node_modules/@storybook/addon-storyshots/dist/test-bodies.js:21:18
      at Object.<anonymous> (node_modules/@storybook/addon-storyshots/dist/api/snapshotsTestsTemplate.js:44:33)

What makes it break? Is there anyway we can mock it?

I guess there is something going on with how it gets mounted. I don't think it's so much important and we can skip it but feel free to investigate further.

In this case, it would be DontTest keyword in the title.

We can also just add an explicit list of (?!) to the storyKindRegex since the list will probably be somehow limited.

We can start with the blacklist at first. Good idea 👍

@gziolo gziolo force-pushed the add/storybook-storyshots branch from 0d56672 to 3684187 Oct 22, 2019
@epiqueras

This comment has been minimized.

Copy link
Contributor

epiqueras commented Oct 22, 2019

Sounds good, let's do it.

@gziolo gziolo force-pushed the update/storybook-enhancements branch from cceee54 to 34a2aa2 Oct 24, 2019
@gziolo gziolo force-pushed the add/storybook-storyshots branch from 3684187 to 7c6e7b3 Oct 24, 2019
@gziolo

This comment has been minimized.

Copy link
Member Author

gziolo commented Oct 24, 2019

With 7c6e7b3 we have a list of skipped components in the Storyshots configuration. It should be ready to go. This branch is based on the #18030 so we can merge it there once approved.

Copy link
Contributor

epiqueras left a comment

I left 2 suggestions I think we should implement and then I think we should merge this.

On Snapshots vs. Screenshots

Screenshot testing is a nice-to-have when using paid services with smart diff heuristics, but most of the time it adds unnecessary noise and fails with the most minor of changes. The benefits are marginal if any. If a component has been touched in a PR, developers are expected to see what has changed visually before approving it and they will do a much better job at judging whether the changes are acceptable or not than a computer.

Snapshot testing does add a lot of value to projects like this though. WP cares a lot about markup and class names, because other code relies on it. Sometimes PRs might slip through where visually there weren't any significant changes. And, even when inspecting the code, things looked fine. But, simple logic changes can change markup and class names in specific cases and break things that relied on them. A Storyshot would have caught those changes and surfaced them to reviewers for them to judge whether they were acceptable or not.

In short, visual regression testing is something we already do manually in every PR and that we will still have to do even if a computer does it too, but DOM snapshot testing is something we don't do (We look at the declarative React code that declares all possible state outputs, but that makes it hard to catch breaking changes.) and that we would benefit from having a computer do.

On a11y

We already use the Storybook a11y plugin which uses axe under the hood, which we can easily integrate into the Storyshots.

@@ -4,5 +4,10 @@ module.exports = function( api ) {
return {
presets: [ '@wordpress/babel-preset-default' ],
plugins: [ 'babel-plugin-inline-json-import' ],
env: {
test: {

This comment has been minimized.

Copy link
@epiqueras

This comment has been minimized.

Copy link
@gziolo

gziolo Oct 30, 2019

Author Member

It's already scoped to stories with the current setup:

  • it is scoped only for Storybook because the Babel config is local for the tool
  • it gets activated only when running tests

I don't think there is any difference between those two options and they also don't share any preference in their docs. In addition to that, macros struggle with the cache invalidation for the dependent files. See the comment in create-react-app facebook/create-react-app@11737bc#diff-b1524f1e1ccabdb327a12f23b432b20bR13:

// Babel Macros are notoriously hard to cache, so they shouldn't be
// https://github.com/babel/babel/issues/8497
// We naively detect macros using their package suffix and insert a random
// caller name, a valid option accepted by Babel, to compose a one-time
// cacheIdentifier for the file. We cannot tune the loader options on a per
// file basis.

Related Babel issue which might resolve this issue in the future: babel/babel#8497.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Oct 30, 2019

Contributor

That makes sense, thanks for clarifying.

packages/components/storybook/test/index.js Outdated Show resolved Hide resolved
test/unit/config/register-context.js Show resolved Hide resolved
test/unit/jest.config.js Show resolved Hide resolved
@gziolo

This comment has been minimized.

Copy link
Member Author

gziolo commented Oct 30, 2019

On a11y

We already use the Storybook a11y plugin which uses axe under the hood, which we can easily integrate into the Storyshots.

That's cool, we should definitely explore that as a follow-up task. I'd love to see integrated with the default configuration. Thanks for sharing 💯

@gziolo gziolo force-pushed the add/storybook-storyshots branch from 9e4bcb8 to fdaed2e Nov 12, 2019
@ItsJonQ

This comment has been minimized.

Copy link
Contributor

ItsJonQ commented Nov 12, 2019

I'd love to hear your feedback on the presented approach.
@gziolo Hallo! Apologies for the delay!

From my experience, snapshot testing (in general) is definitely better than nothing, but I prefer having and writing unit/integration tests.

I also think writing unit tests should also be easier, which can be achieved by having testing utils (if we're staying with Enzyme) or leveraging another testing solution.

Personally, I haven't used this particular addon for Storybook.

I'm okay with keeping this in to see where it goes/how it feels :)

Maybe it can automate a portion of manually writing basic render tests, freeing up time/focus to write unit/integration tests for more complex scenarios.

Copy link
Member

jorgefilipecosta left a comment

I was able to run the tests with success by using "npm run test-unit storybook/test/index.js".

I'm not sure if these tests will be useful in catching regressions or not but I think the best way is to merge them and check if they helped us or they added developer overhead to update the snapshots.
We also have some interesting potential follow-ups pending like the a11y tests, so I guess we should merge this PR and see where these type of tests leaves us.

storybook/test/index.js Outdated Show resolved Hide resolved
storybook/config.js Show resolved Hide resolved
@gziolo gziolo force-pushed the add/storybook-storyshots branch from fdaed2e to ccc705b Nov 13, 2019
@gziolo

This comment has been minimized.

Copy link
Member Author

gziolo commented Nov 13, 2019

I invested more time into this issue and I managed to mock those nodes which use refs with the following commits:
1a59025
ccc705b

This means, we no longer skip any stories from snapshot testing. However, the side effect is that if there is some special use case for refs, it might require custom logic in the mocking helper. The following seem to work for now:

		createNodeMock: ( element ) => {
			if ( story.kind === 'Components|ClipboardButton' ) {
				return {
					firstChild: document.createElement( 'button' ),
				};
			}
			return element.type && document.createElement( element.type );
		},

I followed links shared by @epiqueras:

It looks like it's a general issue with snapshot testing in React apps.

@diegohaz

This comment has been minimized.

Copy link
Member

diegohaz commented Nov 13, 2019

One thing I noticed is that storyshot tests will still fail if a console warning is emitted. On #17875, I created a story for a deprecated thing so I could see the UI worked and the warning was shown.

image

On normal tests I can check if the warning was shown. Can I do that with Storyshots?

@gziolo gziolo requested a review from epiqueras Nov 14, 2019
@gziolo

This comment has been minimized.

Copy link
Member Author

gziolo commented Nov 14, 2019

One thing I noticed is that storyshot tests will still fail if a console warning is emitted. On #17875, I created a story for a deprecated thing so I could see the UI worked and the warning was shown.

This raises the question of whether we should keep the legacy version of Toolbar in Storybook 🤔

I could make the point that it should be removed from there as it should be discouraged from using in new code. Having it included in the docs might create the temptation for folks to copy the example and use it in their projects when they find that it fits their needs.

I'm aware that it isn't probably the type of answers you expected but this is something we should discuss more broadly before we jump into finding a technical solution on how to mitigate it. Maybe we don't have to do it. The only downside is that this story is very useful in the process of developing the migration path for the Toolbar component.

@gziolo gziolo force-pushed the add/storybook-storyshots branch from ccc705b to 7abdc2e Nov 14, 2019
@gziolo gziolo mentioned this pull request Nov 15, 2019
6 of 6 tasks complete
@gziolo gziolo force-pushed the add/storybook-storyshots branch 2 times, most recently from f5d6912 to 85c7ac2 Nov 19, 2019
@gziolo gziolo force-pushed the add/storybook-storyshots branch 2 times, most recently from 09f32e2 to 83fd01e Nov 26, 2019
@gziolo gziolo force-pushed the add/storybook-storyshots branch from 83fd01e to 92f46dd Nov 26, 2019
@gziolo

This comment has been minimized.

Copy link
Member Author

gziolo commented Nov 26, 2019

I included documentation in 92f46dd.

@gziolo gziolo force-pushed the add/storybook-storyshots branch from 628b459 to a2d5a2c Nov 26, 2019
@gziolo gziolo merged commit 3cbb291 into master Nov 26, 2019
1 of 2 checks passed
1 of 2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Errored
Details
@gziolo gziolo deleted the add/storybook-storyshots branch Nov 26, 2019
@youknowriad youknowriad added this to the Gutenberg 7.1 milestone Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.