[Testing] Test layout in RelatedArtist + Gene test fix #409

sarahscott · 2016-12-02T21:39:12Z

Jest doesn't automatically call onLayout, so this test started failing after I updated Related Artists with a more responsive approach.

I put together a proof-of-concept test for injecting width into onLayout in the RelatedArtist component. The nice thing about this approach is we can specify any dimensions we want in the mocked nativeEvent and can make them correspond to actual expected values for iPhone and iPad.

sarahscott · 2016-12-02T22:31:15Z

lib/components/gene/__tests__/__snapshots__/about-test.js.snap

            }
          }>
          <ImageView
            imageURL=""
            style={
              Object {
-                "height": 105,


This was happening because the layout would rely on Dimensions, which would assume iPhone dimensions if window returned nothing.

sarahscott · 2016-12-02T22:32:56Z

lib/components/related_artists/__tests__/__snapshots__/layout-test.js.snap

+          Object {
+            "margin": 5,
+            "paddingBottom": 20,
+            "width": 242,


Now we have a calculated value for width based on the mocked nativeEvent width.

… onLayout

sarahscott · 2016-12-02T22:38:59Z

lib/components/related_artists/__tests__/layout-test.js

+  tree.props.onLayout(mockNativeEvent)
+
+  // re-render
+  tree = component.toJSON()


This dance is admittedly a bit cumbersome but could be wrapped?

I took a lot of inspiration from this: https://facebook.github.io/jest/docs/tutorial-react.html#snapshot-testing

mennenia · 2016-12-02T22:44:35Z

lib/components/related_artists/__tests__/layout-test.js

+
+jest.mock('../../opaque_image_view.js', () => 'ImageView')
+
+it('lays out correctly', () => {


so we could make a few of these, for iPhone and iPad, say?

mennenia · 2016-12-02T22:44:56Z

lib/components/related_artists/__tests__/layout-test.js

+  const mockNativeEvent = {
+    nativeEvent: {
+      layout: {
+        width: 768,


and then change the associated widths here - and get different dom outputs

Yes. The problem with this approach though is that the parent components can't propagate layout down to the children, so the higher-level snapshots will be sort of inaccurate. i.e. in this example, the Gene's About section snapshot has a RelatedArtists component with a width of 1.

So this is great for testing layout logic in the component itself but not in anything it instantiates. It would be lovely to have some sort of global LayoutEvent type thing, so I'll look into that next.

ohhh interesting. Thanks for explaining!

mennenia · 2016-12-02T22:46:39Z

lib/components/related_artists/__tests__/layout-test.js

+  }
+
+  let component = renderer.create(<RelatedArtists artists={artists}/>)
+  let tree = component.toJSON()


so you have to json-ify the component, trigger onlayout, and then jsonify again? e.g. skipping L29 wouldn't work? :o

Yeah... that's the annoying thing. I am using the approach here for manually triggering a callback.

This could be wrapped in a helper method though. Something like runOnLayout(component).

Agreed, the toJSON and tree var are a little confusing. I'd suggest having that helper take the dimensions and do the mocking in there as well.

yeah, this all looks 👍

mennenia · 2016-12-02T22:53:05Z

Great stuff!

sarahscott · 2016-12-02T22:57:42Z

Maybe we can mock LayoutEvents the way we mock ImageView here

orta · 2016-12-03T13:10:17Z

Shouldn't need to do mocking, as an event is just a normal javascript object - you can pass in whatever you want, here's a test where I pass in a SwitchEvent -> https://github.com/artsy/emission/pull/406/files#diff-0bc03f13698e18df3c3bb6d44c2f6e75R41

Ah yeah- that's exactly what you did

orta · 2016-12-03T13:41:59Z

Aye, looks a tad cumbersome - but that's what they're doing - seems like the right way to do it to me @sarahscott 💯

Want to look into a helper method in this PR or anohter?

sarahscott · 2016-12-05T15:14:57Z

I think I can timebox myself for the wrapper method in this PR - it would be a nice precedent to set. The reason I would consider mocking is so that a parent component like the About view could instantiate all of its children using the same LayoutEvent instead of having to pass that down through props or something.

I think if we make the assumption that parent components are only testing logic and fonts and maybe their own layouts, and that the children in their snapshots will be layout-less, the approach in this pr is fine. It would be nice if the snapshots were more accurate though.

orta · 2016-12-07T11:57:21Z

@sarahscott did you look at this?

sarahscott · 2016-12-07T18:14:01Z

All set for review ☑️

alloy · 2016-12-07T19:06:42Z

@sarahscott Awesome, loving that test helper 👌 💯

Just a question about the location on disk, though. I’m still getting used to the “tests next to the lib files” convention of Jest, is lib/testing_utilities the convention used in the community? And seeing as the directory is called testing_utilities shouldn’t individual helpers be in separate files (that use export default TheFunction) rather than having a single helpers.js file?

sarahscott · 2016-12-08T19:46:34Z

I was looking at how metaphysics organizes smaller helper functions and modeled the helpers.js file after this because I assume there will be other helper methods in the near future. I'm happy to adopt the convention of one-file-per-function if there's a good reason to, though.

I'll look into conventions within the community today. I was thinking the testing_utilities folder would be a place for methods that can be used by tests for any container or component, even though the tests themselves are next to the lib files.

orta · 2016-12-08T21:50:54Z

I feel things like these should probably go in the __tests__ folders, I wouldn't think to look right back at the root, closest to what they are the domain of, so I'd have expected this file in lib/containers/__test__/helpers/layout.js

I looked through the Jest source code, and couldn't find any examples of using helper functions in there tests. So I couldn't get anything useful from there.

orta · 2016-12-08T21:56:32Z

lib/testing_utilities/helpers.js

@@ -0,0 +1,19 @@
+import renderer from 'react-test-renderer'
+
+export const setupWithLayout = (component, layout) => {


this should probably be more descriptive, and ideally be typed - I wouldn't be able to know from the name what objects to put in, perhaps something like?

/** Renders a React Component after applying a nativeEvent to an onLayout function passed in to props */ export const jsonAfterApplyingNativeLayoutDimentions = (component: any, layout: { width?: number, height?:number } ) => {

I agree with adding the comments but the method name is too verbose 😉 !

sarahscott · 2016-12-08T22:26:55Z

But what if a component (instead of a container) needs those methods? Would we traverse from the components folder into the containers folder's __tests__?

sarahscott · 2016-12-08T22:33:03Z

I know I'm going to need the exact same method for testing components. If we really don't want a lib- or project-wide testing_utilities folder, I think the only other way would be to make a separate utilities folder or file for both components and containers. The reason I didn't do this here is because that would require code duplication. If it would really improve our testing experience though, I'm open to that.

alloy · 2016-12-09T21:58:03Z

lib/testing_utilities/helpers.js

@@ -1,6 +1,7 @@
 import renderer from 'react-test-renderer'

-export const setupWithLayout = (component, layout) => {
+/** Renders a React Component with specified layout using onLayout callback */
+export const renderWithLayout = (component: any, layout: { width?: number, height?:number }) => {


Inconsistent spacing in layout type info.

sarahscott · 2016-12-12T16:02:35Z

@orta bump

orta · 2016-12-12T16:24:10Z

Alright - yeah, that makes sense, those two are both siblings on the same hierarchy, so IMO it should probably go in lib/__tests_/helpers/file.js

sarahscott · 2016-12-12T16:49:17Z

I think using a __tests__ folder inside lib would be confusing because it implies there are actual tests in that folder when actually there are only utilities. I'd like to keep the folder named as it is for now and if it proves to be an issue we can change it later.

orta · 2016-12-12T17:20:00Z

Personally, I'm not sold, but if you're sure 👍 - we do need to put tests on the files in there anyway

But you shouldn't move the __mocks__ folder into that folder, I'm surprised my tests pass at all with that changed.

sarahscott · 2016-12-12T20:14:16Z

I see - __mocks__ is supposed to go in the same parent directory as node_modules (it was previously inside lib).

@alloy do you have an opinion on the name for this shared directory of testing utilities? Is __tests__ significantly clearer?

orta · 2016-12-12T21:40:13Z

I see - mocks is supposed to go in the same parent directory as node_modules (it was previously inside lib).

ah nice one 👍

My point is to not make a generic testing_utilities folder, but to say it provides functions whose scope is from the lib folder downwards, and that like all testing related code - you can find it inside a __tests__ folder.

alloy · 2016-12-13T03:20:24Z

@sarahscott I agree with @orta 👍

orta · 2016-12-13T16:33:37Z

👍

[Testing] Updated gene test

e7fce15

sarahscott assigned orta Dec 2, 2016

sarahscott commented Dec 2, 2016

View reviewed changes

sarahscott mentioned this pull request Dec 2, 2016

Figure out a way to test layouts that rely on onLayout callback #410

Closed

sarahscott commented Dec 2, 2016

View reviewed changes

[Testing] Added a test with mocked nativeEvent and manually triggered…

5548615

… onLayout

sarahscott force-pushed the sarah-test-fix branch from d7ce60f to 5548615 Compare December 2, 2016 22:35

sarahscott changed the title ~~[Testing] Updated gene test~~ [Testing] Test layout in RelatedArtist + Gene test fix Dec 2, 2016

sarahscott commented Dec 2, 2016

View reviewed changes

mennenia reviewed Dec 2, 2016

View reviewed changes

[Testing] Added a helper method for testing layout

1420893

orta reviewed Dec 8, 2016

View reviewed changes

[Testing] Added comments and a better name to helper

3d5152d

alloy reviewed Dec 9, 2016

View reviewed changes

Fix inconsistent spacing

01b884b

[Testing] Moved __mocks__ directory

0872631

sarahscott added 2 commits December 13, 2016 08:20

[Testing] Made a __tests__ directory in lib

ca6866c

[Testing] Renamed __tests__ directory to tests

b51fd01

orta merged commit 11906b4 into master Dec 13, 2016

orta deleted the sarah-test-fix branch December 13, 2016 16:33


		jest.mock('../../opaque_image_view.js', () => 'ImageView')

		it('lays out correctly', () => {

		@@ -0,0 +1,19 @@
		import renderer from 'react-test-renderer'

		export const setupWithLayout = (component, layout) => {

[Testing] Test layout in RelatedArtist + Gene test fix #409

[Testing] Test layout in RelatedArtist + Gene test fix #409

Conversation

sarahscott commented Dec 2, 2016 • edited Loading

sarahscott Dec 2, 2016 • edited Loading

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

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

mennenia commented Dec 2, 2016

sarahscott commented Dec 2, 2016

orta commented Dec 3, 2016 • edited Loading

orta commented Dec 3, 2016 • edited Loading

sarahscott commented Dec 5, 2016

orta commented Dec 7, 2016

sarahscott commented Dec 7, 2016

alloy commented Dec 7, 2016

sarahscott commented Dec 8, 2016

orta commented Dec 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarahscott commented Dec 8, 2016

sarahscott commented Dec 8, 2016

Choose a reason for hiding this comment

sarahscott commented Dec 12, 2016

orta commented Dec 12, 2016

sarahscott commented Dec 12, 2016

orta commented Dec 12, 2016 • edited Loading

sarahscott commented Dec 12, 2016

orta commented Dec 12, 2016

alloy commented Dec 13, 2016

orta commented Dec 13, 2016

sarahscott commented Dec 2, 2016 •

edited

Loading

sarahscott Dec 2, 2016 •

edited

Loading

orta commented Dec 3, 2016 •

edited

Loading

orta commented Dec 3, 2016 •

edited

Loading

orta commented Dec 12, 2016 •

edited

Loading