feat(priceRanges): Add BEM classes and tests #390

pixelastic · 2015-10-30T10:38:19Z

Fixes #387

BREAKING CHANGE: ais-price-ranges--range are now named
ais-price-ranges--item and are wrapped in
a ais-price-ranges--list.

I've moved the bottom form into it's own PriceRangesForm component,
along with its own tests. I've fixed a minor typo where the component
was internally named PriceRange (without the final s).

I factorize some logic form the render in individual methods and
manage to individually test them. This was not an easy task. I had to
mock the default render (so it does nothing) before instanciating
the component. Then, I was able to call each inner method
individually. This requires to stub prototype methods in beforeEach,
then restore them in afterEach. I've added a few helper methods, this
can surely be simplified again but this gives nice granularity in
testing.

I've renamed the range items to item and wrapped them in a list.
I've also added classes to all elements we add (label, separator,
etc). I've removed the empty spans.

redox · 2015-10-30T13:10:06Z

components/PriceRanges/PriceRangesForm.js

+        {fromInput}
+        <span className={this.props.cssClasses.separator}> {this.props.labels.separator} </span>
+        {toInput}
+        <button className={this.props.cssClasses.button} type="submit">{this.props.labels.button}</button>


The only issue with that code is that there won't be any space between the toInput and the button, right?

Right, but why do you need one? This should be handled by CSS

I'm happy with that; it's the same for the checkboxes in the refinementList widget anyway.

redox · 2015-10-30T14:05:41Z

Are you able to rebase so we can merge?

pixelastic · 2015-10-30T14:07:26Z

On it

vvo · 2015-11-02T09:11:38Z

components/PriceRanges/PriceRanges.js

+            return this.getItemFromFacetValue(facetValue);
+          })}
+        </div>
+        {form}


Why not directly call the <Form> instead of calling a function? Not sure this is a common pattern in React community.

The <Form> call is only 5 LOC and it would be ovious to read the render() then?

The goal was to be able to:

Unit test the getForm method separatly

Mock the getForm when testing render

This lets me focus on one thing in each test. In the end I think I did not unit test getForm (my bad, I might have forgotten).

I'll add the missing test and rebase

The goal was to be able to:

Unit test the getForm method separatly
Mock the getForm when testing render
This lets me focus on one thing in each test. In the end I think I did not unit test getForm (my bad, I might have forgotten).

I think we should try to avoid mocking when we can, you can always store the expected <Form> in a variable in your test rather than modifying the implementation to make it easily testable.

The least mocking we have in testing render() the better?

Small detail in the end, if you test getForm() that's ok

This is maybe due to the fact that most components we always only test the render() method even if that's some boilerplate to add. This way we can just refactor the whole component really easily.

In the end testing a React component output is just asserting some JSX on render(), the underlying implementation should not have to be tested when we can avoid it (sometimes for handlers we need to test it).

vvo · 2015-11-02T09:12:14Z

really really awesome :D

pixelastic · 2015-11-02T09:52:14Z

Missing test added: https://github.com/algolia/instantsearch.js/blob/fix/price-ranges-bem/components/PriceRanges/__tests__/PriceRanges-test.js#L169-L192

vvo · 2015-11-02T10:14:40Z

components/PriceRanges/__tests__/PriceRangesForm-test.js

+    it('starts a refine on submit', () => {
+      // Given
+      let refine = sinon.spy();
+      let handleSubmitMock = sinon.spy(PriceRangesForm.prototype, 'handleSubmit');


Why not test the instance method rather than the prototype:

let component = <PriceRangesForm refine={refine} />; TestUtils.renderIntoDocument(<PriceRangesForm refine={refine} />);

If you keep the prototype, you will also need to PriceRangesForm.prototype.handleSubmit.restore();

Also here I think it's just a "spy" rather than a full mock. http://sinonjs.org/docs/#mocks

Found it strange to read "mock" here

Why not test the instance method rather than the prototype:

Well... I tried but failed. <PriceRangesForm refine={refine} /> does not have an handleSubmit method so I cannot spy it. Once "rendered into document" it does have one, but it is then too late to spy on it as it has already been called. That's why I ended up spying on the prototype.

But you're right that I should restore afterwards (I did it in the other tests, forgot about it here).

And I tend to call everything mock as a generic term (spies and stubs), but I'll rename to make it clearer.

If you have a better solution for not spying on the prototype let me know and I'll update the PR. Otherwise I'll simply fix the restore/naming.

If you have a better solution for not spying on the prototype let me know and I'll update the PR. Otherwise I'll simply fix the restore/naming.

The PR was merged no big deal but here it is how it was done before: https://github.com/algolia/instantsearch.js/pull/390/files#diff-40651fdb5ca1bd7385705422de4e7029L102

Well... I tried but failed. does not have an handleSubmit method so I cannot spy it. Once "rendered into document" it does have one, but it is then too late to spy on it as it has already been called. That's why I ended up spying on the prototype.

Weird 1. it does not have one, I thought we already did the same tests in other components? 2. is it really called on render? Because it should be called only on submit

The PR was merged no big deal but here it is how it was done before:

The way we did it before was not checking that our handleSubmit method was called when submitting the form, it was checking the side effects of calling the handleSubmit. Not exactly the same thing and I'd rather test the call to the method when submitting, and testing the effect of the method independently.

is it really called on render? Because it should be called only on submit

Guess I posted too fast. Will have to check again. I remember the whole testing plumbing here was hard to implement, I might have overlooked something.

it was checking the side effects of calling the handleSubmit

Nope it was actually simulating changing the form and submiting it: https://github.com/algolia/instantsearch.js/pull/390/files#diff-40651fdb5ca1bd7385705422de4e7029L115

Which is the interface we want to test:

input: submit form

output: call my spy refine with the right values

Testing that an underlying prototype implementation was called is not the same, here you enforce your implementation to have a certain method versus testing the INPUT/OUTPUT you expose

Let me start by saying I love this discussion :)

You're actually right. I think I forgot a test on handleSubmit. What I wanted to test was:

Calling handleSubmit calls the passed refine props with the correct args

Submitting the for calls handleSubmit with values from the inputs

I did only half the job and thus, the previous test was better. Don't hesitate to stall the merging for issues like that next time.

Fixes #387 BREAKING CHANGE: `ais-price-ranges--range` are now named `ais-price-ranges--item` and are wrapped in a `ais-price-ranges--list`. I've moved the bottom form into it's own PriceRangesForm component, along with its own tests. I've fixed a minor typo where the component was internally named PriceRange (without the final __s__). I factorize some logic form the render in individual methods and manage to individually test them. This was not an easy task. I had to mock the default `render` (so it does nothing) before instanciating the component. Then, I was able to call each inner method individually. This requires to stub prototype methods in beforeEach, then restore them in afterEach. I've added a few helper methods, this can surely be simplified again but this gives nice granularity in testing. I've renamed the `range` items to `item` and wrapped them in a `list`. I've also added classes to all elements we add (`label`, `separator`, etc). I've removed the empty `span`s.

feat(priceRanges): Add BEM classes and tests

vvo · 2015-11-03T11:34:44Z

components/PriceRanges/__tests__/PriceRangesForm-test.js

+
+      // Then
+      expect(handleSubmitMock.calledOnce).toBe(true);
+      expect(refine.calledWith(10, 20)).toBe(true);


Actually you are testing both the input output and your internal implementation here. I would get rid of the check on handleSubmit because there's no need of it. It's not a mock since the behavior is not changed. You are just checking that your code follow an internal path you specified in your test but there's no added value versus checking (refine.calledWith)

Yep, I now see what you mean. I was convinced I did two tests: one for checking that handleSubmit was called on submit, and another to test that handleSubmit correctly calls refine. I did not and tested everything in the same test, my bad

pixelastic added the in progress label Oct 30, 2015

redox reviewed Oct 30, 2015
View reviewed changes

pixelastic force-pushed the fix/price-ranges-bem branch from 388adc8 to bfcc4a8 Compare October 30, 2015 14:09

vvo reviewed Nov 2, 2015
View reviewed changes

pixelastic force-pushed the fix/price-ranges-bem branch from bfcc4a8 to bd63acd Compare November 2, 2015 09:47

vvo reviewed Nov 2, 2015
View reviewed changes

pixelastic force-pushed the fix/price-ranges-bem branch from bd63acd to ad58d7a Compare November 2, 2015 10:16

vvo pushed a commit that referenced this pull request Nov 2, 2015

Merge pull request #390 from algolia/fix/price-ranges-bem

84dca16

feat(priceRanges): Add BEM classes and tests

vvo merged commit 84dca16 into develop Nov 2, 2015

vvo deleted the fix/price-ranges-bem branch November 2, 2015 10:55

vvo removed the in progress label Nov 2, 2015

pixelastic mentioned this pull request Nov 3, 2015

priceRanges: Making BEM classes more consistent #387

Closed

3 tasks

vvo reviewed Nov 3, 2015
View reviewed changes

aymeric-giraudet pushed a commit that referenced this pull request Dec 8, 2022

chore(deps): update dependency @types/node to v8.10.49 (#390)

cfb8d4d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(priceRanges): Add BEM classes and tests #390

feat(priceRanges): Add BEM classes and tests #390

pixelastic commented Oct 30, 2015

redox Oct 30, 2015

pixelastic Oct 30, 2015

redox Oct 30, 2015

redox commented Oct 30, 2015

pixelastic commented Oct 30, 2015

vvo Nov 2, 2015

pixelastic Nov 2, 2015

pixelastic Nov 2, 2015

vvo Nov 2, 2015

vvo Nov 2, 2015

vvo Nov 2, 2015

vvo commented Nov 2, 2015

pixelastic commented Nov 2, 2015

vvo Nov 2, 2015

vvo Nov 2, 2015

pixelastic Nov 3, 2015

vvo Nov 3, 2015

pixelastic Nov 3, 2015

vvo Nov 3, 2015

pixelastic Nov 5, 2015

vvo Nov 3, 2015

pixelastic Nov 5, 2015

feat(priceRanges): Add BEM classes and tests #390

feat(priceRanges): Add BEM classes and tests #390

Conversation

pixelastic commented Oct 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redox commented Oct 30, 2015

pixelastic commented Oct 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vvo commented Nov 2, 2015

pixelastic commented Nov 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment