chore(testing): adds enzyme and updates existing tests #258

AllenBW · 2018-02-28T15:41:58Z

closes #257

Yah might be wondering, why doesn't this pr drastically increase test coverage! Scope. While many test files were touched, the changes are similar and new tests have not been written (ok well there are a few, but not many)

This also updates jest so we can use super fancy snapshotSerializers

This also adds a few needed coverage reporting deps semi related to: #jestjs/jest#5772

I'd really like to pull "react-test-renderer": "^16.2.0" from 📦 butttttt as new features are merged, they are using the old testing ⚙️, I just can't keep up 😩

coveralls · 2018-02-28T15:45:43Z

Pull Request Test Coverage Report for Build 938

1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.6%) to 71.084%

Totals
Change from base Build 929:	0.6%
Covered Lines:	1137
Relevant Lines:	1437

💛 - Coveralls

priley86 · 2018-03-02T13:52:06Z

src/components/AboutModal/__snapshots__/AboutModal.test.js.snap

@@ -1,3 +1,5 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP

 exports[`AboutModal renders correctly when hidden 1`] = `null`;
+
+exports[`AboutModal renders correctly when shown 1`] = `"<div role=\\"dialog\\"><div class=\\"modal-backdrop fade in\\"></div><div role=\\"dialog\\" tabindex=\\"-1\\" style=\\"display: block; padding-right: 0px;\\" class=\\"fade in modal\\"><div class=\\"modal-dialog\\"><div class=\\"modal-content about-modal-pf\\" role=\\"document\\"><div class=\\"modal-header\\"><button class=\\"close\\"><span aria-hidden=\\"true\\" class=\\"pficon pficon-close\\" title=\\"Close\\"></span><span class=\\"sr-only\\">Close</span></button></div><div class=\\"modal-body\\"><h1>Product Title</h1><div class=\\"product-versions-pf\\"><ul class=\\"list-unstyled\\"><li class=\\"\\"><strong>Label</strong> Version</li></ul></div><div class=\\"trademark-pf\\">Trademark and Copyright Information</div></div><div class=\\"modal-footer\\"><img src=\\"\\" alt=\\"Patternfly Logo\\"></div></div></div></div></div>"`;


is there any way to clean up this snapshot? the way component.html() is snapping is not so pretty...

When you say "not so pretty" and "clean up" I understand your meaning to be, that the single line escaped html output is difficult to read at a glance. The formatting is shite, is that about right?

Or is your issue with the type of output? I would argue the content rendered by a component far more meaningful than alternatives what is rendered when prop show = true.

Se how we use enzyme-to-json in foreman:
https://github.com/theforeman/foreman/blob/develop/webpack/assets/javascripts/react_app/components/common/Alert/AlertBody.test.js#L11

@sharvit greaaaaaaaaat suggestion! while the wrapper type is different, mount vs shallow used in foreman... it looks like the .render() output yields the same result... will go back and fixup a few other instances in this pr on monday

priley86 · 2018-03-02T13:56:22Z

@AllenBW do you mind summarizing your thoughts thus far? I guess i am still seeing Enzyme as a supplement to jest snapshots (not a replacement)...from the Jest docs: https://facebook.github.io/jest/docs/en/snapshot-testing.html#does-snapshot-testing-substitute-unit-testing). So Enzyme helps a lot with the event based/interactive tests, but we can get a lot of coverage from the simple prop based/snapshot based tests. I don't see the need to duplicate test coverage "just because" of older conventions I guess is what I am after... The snapshots scale really well w/ regressions (as we can update them quickly and visually see what's changing). For example, if you were to change the default class of the Button in this project, you would be able to immediately see every impacted component and that component's snapshots (and that is several regressions there). You would also be able to immediately update them with one command. Now that is scale at work! ;). The old way of doing this would require updating all those regression tests by hand...

Now the other thing I think we have to continue reminding folks of is - Jest/Enzyme snapshots run headlessly (no browser needed). So they give us the unit/integration level tests (but not e2e). But that is exactly the point... they run super fast and give us insights immediately as we develop and let us focus on fixing on our own bugs rather than the browsers 😸 .

https://kentcdodds.com/post/write-integration-tests/

priley86 · 2018-03-02T13:58:20Z

@AllenBW maybe you'd like to present on this topic at a future PF React Community meeting?

AllenBW · 2018-03-02T14:52:32Z

@priley86 tl;dr Thoughts, greater flexibility, less code, more natural setup and use, works to make snapshots more meaningful.

This is not a replacement for jest snapshots, if for anything, possibly react-test-renderer. This gives us more flexibility with the way we instantiate components (shallow mount or render) and a greater number of ways we can interact with those instances. I have found working with enzyme to be more "natural" and FAR more concise. Lets say I want to test a component button click...

import React from 'react'
import {mount} from 'enzyme'
import MyComponent from './foo'

it('foo should be able to click the button', () => {
    const mockFn = jest.fn();
    const element = mount(<Foo someAction={mockFn} />);
    element.find('button').simulate('click');
    expect(mockFn).toHaveBeenCalled();
  });

As yah can see, code is require, reading along is just a lil bit more natural.

Also yah, if this is something that sticks, yeah wouldn't mind presenting pfr community meeting.

Our coverage sucks. Why does our coverage suck? Because functions are not invoked during testing. Why are functions not being invoked during testing? Because stubbing live components yah can poke n prod can be tricky. Enzyme, this work, makes it less tricky by being able to either mount render or shallow instantiate components. With the apis each of those wrappers expose, devs are able to do a BUNCH of things, ones that I have done in this work, (still more to do, gimme a break i'm new at this one) include rendering components as html (rather than a giant jsx-ish json) assess the outcome of props rather than just their values...

🌮 💃 🥑

AllenBW · 2018-03-02T14:53:11Z

ps, still a wip.
pss, @priley86 thank you for talking 🤗 🙇‍♀️ 🍦

priley86 · 2018-03-02T15:16:35Z

I really appreciate all this W.I.P. discussion candy... it makes me so hungry though haha ^^ 🍨 🍬 😋

and appreciate the research already @AllenBW 👍

sharvit · 2018-03-05T10:33:23Z

@AllenBW I think you can learn a lot about enzyme/jest/snapshots from theforeman/foreman#5240

Especially from:
https://github.com/theforeman/foreman/pull/5240/files#diff-736af63fce546e2ed852e81792b1358e
https://github.com/theforeman/foreman/pull/5240/files#diff-0a888503d79311bb9407605e191eae95

priley86 · 2018-03-08T13:32:53Z

indirectly related... but we still need to find a way to test the react-overlays / Modals as well...and just mentioning an upstream response on this yesterday (not sure that it helps yet):

react-bootstrap/react-overlays#225 (comment)

https://github.com/patternfly/patternfly-react/blob/master/src/components/AboutModal/AboutModal.test.js

AllenBW · 2018-03-09T13:59:09Z

@priley86 did yah checkout the about modal snapshot? when a component is wrapped in a mount we're able to interact with it throughout the lifecycle

priley86 · 2018-03-12T22:58:14Z

i'm just watching the test coverage go up up up before my 👁 👁 's ... this is incredible...

AllenBW · 2018-03-14T15:31:12Z

@jeff-phillips-18 @patternfly-build @sharvit @anyone_else?

So steps forward for this pr, a few additional components have been added since this... (5 to be exact), can we get this in, circle back around and update those tests? (and then pull the depreciated dep) OR do it here, hopefully get this in before more new components are merged?

priley86 · 2018-03-16T02:24:34Z

@AllenBW please feel free to continue the Enzyme magic as you have time...this was a huge huge win for us already.

AllenBW · 2018-03-16T12:05:46Z

Thats the plan!! 😋 🙇‍♀️

jgiardino added the review label Feb 28, 2018

AllenBW force-pushed the #257-adds-enzyme branch from 66eaed6 to f2656c2 Compare February 28, 2018 15:43

AllenBW added chore and removed review labels Feb 28, 2018

AllenBW force-pushed the #257-adds-enzyme branch 7 times, most recently from 3b43500 to e4bf1ef Compare February 28, 2018 21:58

priley86 reviewed Mar 2, 2018

View reviewed changes

AllenBW force-pushed the #257-adds-enzyme branch 2 times, most recently from b487707 to 9ced7eb Compare March 3, 2018 13:18

AllenBW force-pushed the #257-adds-enzyme branch 4 times, most recently from a4c71c6 to 2e7b768 Compare March 7, 2018 14:34

AllenBW force-pushed the #257-adds-enzyme branch 3 times, most recently from ce15d86 to 7f07909 Compare March 9, 2018 18:29

priley86 mentioned this pull request Mar 9, 2018

Refactor #removeNode in DatastoresStepForm ManageIQ/manageiq-v2v#130

Merged

AllenBW changed the title ~~[WIP]chore(testing): adds enzyme and updates exisiting tests~~ chore(testing): adds enzyme and updates existing tests Mar 12, 2018

AllenBW force-pushed the #257-adds-enzyme branch from 7f07909 to 5bf4ee9 Compare March 14, 2018 05:00

chore(testing): adds enzyme and updates existing tests

7d2e928

AllenBW force-pushed the #257-adds-enzyme branch from 5bf4ee9 to 7d2e928 Compare March 14, 2018 05:47

priley86 approved these changes Mar 14, 2018

View reviewed changes

AllenBW mentioned this pull request Mar 14, 2018

feat(WizardPattern): Add WizardPattern and StatefulWizardPattern components #268

Merged

jeff-phillips-18 approved these changes Mar 14, 2018

View reviewed changes

jeff-phillips-18 merged commit 3d691c4 into patternfly:master Mar 14, 2018

AllenBW deleted the #257-adds-enzyme branch March 15, 2018 13:20

priley86 mentioned this pull request Mar 16, 2018

feat(NotificationDrawer): Notification, NotificationDrawer #262

Merged

2 tasks

AllenBW mentioned this pull request Mar 21, 2018

chore(testing): updates remaining tests to enzyme #282

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(testing): adds enzyme and updates existing tests #258

chore(testing): adds enzyme and updates existing tests #258

AllenBW commented Feb 28, 2018 •

edited

Loading

coveralls commented Feb 28, 2018 •

edited

Loading

priley86 Mar 2, 2018

AllenBW Mar 2, 2018

sharvit Mar 3, 2018 •

edited

Loading

AllenBW Mar 3, 2018

priley86 commented Mar 2, 2018 •

edited

Loading

priley86 commented Mar 2, 2018

AllenBW commented Mar 2, 2018

AllenBW commented Mar 2, 2018

priley86 commented Mar 2, 2018 •

edited

Loading

sharvit commented Mar 5, 2018

priley86 commented Mar 8, 2018

AllenBW commented Mar 9, 2018

priley86 commented Mar 12, 2018

AllenBW commented Mar 14, 2018

priley86 commented Mar 16, 2018

AllenBW commented Mar 16, 2018

chore(testing): adds enzyme and updates existing tests #258

chore(testing): adds enzyme and updates existing tests #258

Conversation

AllenBW commented Feb 28, 2018 • edited Loading

coveralls commented Feb 28, 2018 • edited Loading

Pull Request Test Coverage Report for Build 938

💛 - Coveralls

priley86 Mar 2, 2018

Choose a reason for hiding this comment

AllenBW Mar 2, 2018

Choose a reason for hiding this comment

sharvit Mar 3, 2018 • edited Loading

Choose a reason for hiding this comment

AllenBW Mar 3, 2018

Choose a reason for hiding this comment

priley86 commented Mar 2, 2018 • edited Loading

priley86 commented Mar 2, 2018

AllenBW commented Mar 2, 2018

AllenBW commented Mar 2, 2018

priley86 commented Mar 2, 2018 • edited Loading

sharvit commented Mar 5, 2018

priley86 commented Mar 8, 2018

AllenBW commented Mar 9, 2018

priley86 commented Mar 12, 2018

AllenBW commented Mar 14, 2018

priley86 commented Mar 16, 2018

AllenBW commented Mar 16, 2018

AllenBW commented Feb 28, 2018 •

edited

Loading

coveralls commented Feb 28, 2018 •

edited

Loading

sharvit Mar 3, 2018 •

edited

Loading

priley86 commented Mar 2, 2018 •

edited

Loading

priley86 commented Mar 2, 2018 •

edited

Loading