test: Add test to ImageZoom component by jouderianjr · Pull Request #548 · gitpoint/git-point

jouderianjr · 2017-10-21T20:49:17Z

So, this is my test proposal for ImageZoom 😄

I'm accepting any type of suggestions 💃

alejandronanez

I think this are not "real" unit tests, for example, you're checking if ImageZoom props are applied to some child components, testing this is not worth it because it would be testing how React works. What you're doing here looks more like a manual snapshot.

The goal of unit tests is to verify interactions/conditional renders/class' methods.

I think you can do some valuable tests here like:

1. should render Modal if state.imgZoom is truthy
2. should not render Modal if state.imgZoom is truthy
3. should call the closeModal when onRequestClose Modal is called
4. should call closeModal method when the user press TouchableOpacity (line 72)
5. should call openModal when the user presses touchableHighlight

If you need to find an specific TouchableHightlight component, you can add a nativeId prop to it and use Enzyme's wrapper.find({ nativeId: 'the-id' })
If you need to simulate the onPress interaction, you can do it by doing something like theElement.simulate('press')

You can find some examples here:

#531
#547

Let me know if you have any question about this.

andrewda

Ok just a couple things. Also, I think we need to have a discussion on how we're gunna organize these tests. Should we have nested describes or only use its. Usually I don't do any describe nesting as it can get a little confusing, and using only it makes everything very clear (which is the point of tests). Open to all suggestions on this matter (maybe we should open a discussion issue).

andrewda · 2017-10-22T03:52:50Z

__tests__/tests/components/ImageZoom.js

+
+  beforeEach(() => {
+    wrapper = shallow(<ImageZoom uri={uri} style={style} />);
+  });


Using a lot of beforeEach's can get a little confusing (usually it's used for environment configuration, such as initiating a database or something). I think a new wrapper should be defined inside of each test.

andrewda · 2017-10-22T03:53:03Z

__tests__/tests/components/ImageZoom.js

+    beforeEach(() => {
+      touchableHighlight = wrapper.first();
+      image = touchableHighlight.children().first();
+    });


Same thing as I said above – this stuff should go in each test.

andrewda · 2017-10-22T03:53:45Z

__tests__/tests/components/ImageZoom.js

+  describe('when zoom is applied', () => {
+    beforeEach(() => {
+      wrapper.first().simulate('press');
+    });


Also should be in each test.

jouderianjr · 2017-10-22T21:09:53Z

Awesome everyone, I'll refactor. Thanks for the suggestions 😄

coveralls · 2017-10-28T21:38:03Z

Coverage increased (+0.9%) to 38.124% when pulling 6b044c1 on jouderianjr:testes/imageZoom into 6780754 on gitpoint:master.

jouderianjr · 2017-10-28T21:38:58Z

Hey @alejandronanez @andrewda , I recreated the tests, What do you think guys? 😄

andrewda · 2017-10-29T00:41:12Z

__tests__/tests/components/ImageZoom.js

+    expect(clickableImg.length).toBe(1);
+  });
+
+  it('should render Modal when the user presses Touchable', () => {


Modal is lowercase everywhere else, keep it like that?

andrewda

^ meant for my last review to be request changes

jouderianjr · 2017-10-29T01:38:24Z

Nice catch!

coveralls · 2017-10-29T01:54:29Z

Coverage increased (+0.9%) to 38.124% when pulling bc42919 on jouderianjr:testes/imageZoom into 6780754 on gitpoint:master.

alejandronanez · 2017-11-02T16:07:47Z

__tests__/tests/components/ImageZoom.js

+    uri: { uri: 'dummy.png' },
+  };
+
+  it('should render clickable image', () => {


it should render a clickable image when.... (describe some scenario here)

alejandronanez · 2017-11-02T16:08:37Z

__tests__/tests/components/ImageZoom.js

+    expect(clickableImg.length).toBe(1);
+  });
+
+  it('should render modal when the user presses Touchable', () => {


I'd find the element you want to simulate the press event on, and the do the check.

alejandronanez · 2017-11-02T16:10:00Z

__tests__/tests/components/ImageZoom.js

+    const wrapper = shallow(<ImageZoom {...defaultProps} />);
+    wrapper.setState({ imgZoom: true });
+
+    wrapper.props().onRequestClose();


I get this test and it's ok, but it will be better if you check that the modal close after simulating a press (for example) on X or Y component. By doing this, you'll ensure that a real event did close the modal.

alejandronanez · 2017-11-02T16:10:47Z

src/components/image-zoom.component.js

-            <CloseButton onPress={() => this.closeModal()}>
+            <CloseButton
+              nativeId="image-zoom-close-button"
+              onPress={() => this.closeModal()}


Now that you're here, can you change this for onPress={this.closeModal} this is better for perf.

alejandronanez · 2017-11-02T16:11:10Z

src/components/image-zoom.component.js

-      <Touchable onPress={() => this.openModal()} underlayColor="transparent">
+      <Touchable
+        nativeId="image-zoom-clickable-img"
+        onPress={() => this.openModal()}


Same for this one {onPress={this.openModal}}

alejandronanez · 2017-11-02T16:11:23Z

src/components/image-zoom.component.js

-            <StyledPhotoView onTap={() => this.closeModal()} source={uri} />
+            <StyledPhotoView
+              nativeId="image-zoom-photo-view"
+              onTap={() => this.closeModal()}


onTap={this.closeModal}

jouderianjr · 2017-11-03T02:49:23Z

@alejandronanez I think all the changes are done. Thanks for all suggestions 💃

coveralls · 2017-11-03T02:52:53Z

Coverage increased (+0.8%) to 40.18% when pulling 6757a3c on jouderianjr:testes/imageZoom into 34b84d2 on gitpoint:master.

chinesedfan · 2018-01-06T04:31:13Z

I checked suggestions by @alejandronanez had been applied. So this PR should be able to merged.

jouderianjr force-pushed the testes/imageZoom branch from 52bd2d7 to 757baa8 Compare October 21, 2017 20:50

alejandronanez requested changes Oct 22, 2017

View reviewed changes

andrewda requested changes Oct 22, 2017

View reviewed changes

chinesedfan added the tests label Oct 22, 2017

andrewda mentioned this pull request Oct 22, 2017

Add more tests! #518

Open

63 tasks

jouderianjr added the 🏗️ work in progress label Oct 22, 2017

jouderianjr force-pushed the testes/imageZoom branch from 757baa8 to 6b044c1 Compare October 28, 2017 21:29

andrewda approved these changes Oct 29, 2017

View reviewed changes

andrewda requested changes Oct 29, 2017

View reviewed changes

jouderianjr added ready for review and removed 🏗️ work in progress labels Oct 29, 2017

andrewda approved these changes Oct 30, 2017

View reviewed changes

alejandronanez requested changes Nov 2, 2017

View reviewed changes

jouderianjr added 4 commits November 2, 2017 22:32

test: Add tests to ImageZoom component

dfd3c57

chore: Fix verbiage in ImageZoom test

ee309ad

perf: Improve method calls in image-zoom component

1b01eca

test: Improve ImageZoom test

6757a3c

jouderianjr force-pushed the testes/imageZoom branch from bc42919 to 6757a3c Compare November 3, 2017 02:43

chinesedfan merged commit 280210a into gitpoint:master Jan 6, 2018

Conversation

jouderianjr commented Oct 21, 2017

Uh oh!

alejandronanez left a comment

Choose a reason for hiding this comment

Uh oh!

andrewda left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jouderianjr commented Oct 22, 2017

Uh oh!

coveralls commented Oct 28, 2017

Uh oh!

jouderianjr commented Oct 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewda left a comment

Choose a reason for hiding this comment

Uh oh!

jouderianjr commented Oct 29, 2017

Uh oh!

coveralls commented Oct 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jouderianjr commented Nov 3, 2017

Uh oh!

coveralls commented Nov 3, 2017

Uh oh!

chinesedfan commented Jan 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants