refactor(enzyme): Moved tests of RefinementList component to enzyme #948
Conversation
pixelastic
force-pushed
the
poc/enzyme
branch
3 times, most recently
from
0b21722
to
66477eb
Compare
|
Yay \o/ Travis is happy 😀 |
|
Just updated to latest develop |
And contributors, features => looks like the promising testing solution. Skin-deep being more of a wrapper around some existing tooling Just want to avoid developers thinking we only choose projects based on stars. That's true but it's a secret |
| let actual = shallowRender(props); | ||
|
|
||
| // Then | ||
| expect(actual.hasClass('list')).toEqual(true); |
There was a problem hiding this comment.
You can use toBe(true) (easier to read).
Equal means deepEqual, toBe means ===
|
Overall its very good indeed, much more focused tests. I believe we may be missing some cases but we will learn on the long run so I am 💯 on this! The previous testing strategy was very noisy, here it's a lot better. |
| @@ -11,6 +11,15 @@ let baseConfig = { | |||
| loaders: [{ | |||
| test: /\.js$/, exclude: /node_modules/, loader: 'babel' | |||
| }] | |||
| }, | |||
| // enzyme does not work well with webpack: | |||
| // https://github.com/airbnb/enzyme/issues/47#issuecomment-165430136 | |||
|
As for clicking, we will need to find a way to do it, on RefinementList that's tricky, we are using a good number of weird things (the famous label input double click issue) |
|
There's one thing I am wondering: it seems we are missing a test for the structure. If I mess up with the JSX structure I am not sure any test will fail since we are using .find(..) |
|
@vvo Not sure what you mean. Could you give me an example? Once I better understand your point I'll fix your previous comments so this PR can be merged. It has been dormant for too long :) |
Fixes #974. `PaginationLinks` with the `isDisabled` props now render as `span` and not as `a`. We also remove the now unwanted `href`, `aria-label` and `onClick` props. There were no tests files for PaginationLink, and I did not added one. I'd like #948 to be merged before adding those kind of fine-grained tests.
|
My only last concern is this:
Basically, I am asking if I change the internal structure of the implementation by wrapping a What do you think? Example: |
If number of facet values is 5 and limit is 5 then we were still displaying the showMore button. Now fixed, no added tests, waiting for #948
|
I'm not sure that is a bad thing that the tests won't fail for that case. We will have so many React-wrapping divs in the rendered DOM that users won't be able to target specific elements, unless the one where we specified custom So letting the test pass if we add our own wrappers or change the inner markup is ok for me, as long as we keep the correct CSS classes on the correct UI elements. |
Exactly, thanks for clarification, our API is the css classes we have, people should not rely on the number of wrappers existing. |
If number of facet values is 5 and limit is 5 then we were still displaying the showMore button. Now fixed, no added tests, waiting for #948 This is a hotfix
So what is this PR about?
In the process of fixing #934, I tried to add a test to assert that we did not generated more urls than needed in the RefinementList component.
By doing so I discovered in the test file that we had no test to test the
showMorefeature (we had a comment saying basically "I don't know how to test that") and most of our tests where actually testing a whole bunch of fonctionnalities at once (using full JSX diff) instead of just tiny parts of it.This resulted in a lot of boilerplate (we needed to copy the whole props in each tests), and made the tests very brittle because they needed both an internal knowledge of the components and were all very dependent. Adding a new prop would break all tests, even those that are not relevant.
I then looked for a more granular way to test React components. At first I found skin-deep (through this blogpost) which in essence a set of utils to ease shallow rendering testing.
Then, after discussing with @vvo, we decided that if we were to change the testing tool, we should go with enzyme that does the same thing, but is created by airbnb and has many more followers and stars.
So, I tried to refactor our test file with enzyme, as a POC, to see if it was wortht it. Here are my findings:
The logic we had with our custom
rendermethod in the tests is the same, but all the small details are now integrated intoenzyme. We no longer need to expose globalitemPropsto pass them to eachJSXwe need to test, we can focus on defining only the props we need to test.This is a huge win in my opinion, because we can now create, for example, 4 different tests to test the 4 different custom CSS classes we add to generated content instead of one big test testing all possible outcomes.
enzymeprovides a few utilities that help keeping the tests shorts. You start by shallow rendering your component (not rendering the inner sub components). You can then do basic selection with.find('RefinementListItem')or.find({isRefined: true})to select only a few sub-nodes that you'd like to test.You have access to the number of matching elements through the
.lengthproperty, and you can access individual results with the.at(index)call. Once you have a component, you can read and write its props and state with.props(),setProps(),.state()and.setState(). This is also a big win, especially considering that updating the state does trigger all the dynamic modifications. It my example I can toggle thestate.isShowMoreOpenfromtruetofalseand having the number of.find(RefinementListItem).lengthautomatically updated.enzymeis also supposed to let you simulate events (click, etc). But in our case, I couldn't use it because we are using components to do the templating, so the shallow rendering does not display the content of the templates. And our click events are not bound on the template but on the root element generated inside the template. Long story short, I couldn't find a way to test click events with our current templating system (and trust me, I tried!).So in the end, instead of testing that we could click on the "Show More" link to toggle the number of element displayed, I simply tested that a change of the state changed the number of element displayed. This is not ideal. As Vincent suggested, it might be better to extract the "ShowMoreButton" into its own component and test it in isolation there.
I did not use the
mountand fullrendercapabilities ofenzyme. I tried them, but shallow rendering seemed to always be a best usage in my cases.In the end, I feel like this approach is better as it lets us do smaller tests that are in turn more readable and more robust.
All feedbacks welcome!