Add snapshot test suite for React Native support #328

eliperkins · 2018-02-07T22:04:14Z

What:
This adds a rudimentary test suite for the React Native build of Downshift, leveraging Jest snapshots. The goal of this suite is to ensure that Downshift can render React Native components, without calling into methods that are specific to ReactDOM.

Why:

Related to a conversation in #185 (comment)

How:

Adds in a new test suite in other/react-native, as well as a new command, npm test:native. To support testing in React Native, we require a few more dev dependencies: babel-jest, babel-preset-env, babel-preset-react-native, and of course react-native itself.

Checklist:

Documentation N/A
Tests
Ready to be merged
Added myself to contributors table N/A

eliperkins · 2018-02-07T22:06:26Z

other/react-native/jest.config.js

+
+module.exports = Object.assign(jestConfig, {
+  preset: 'react-native',
+  testEnvironment: 'node',


This song and dance felt pretty gnarly, but this is the base config I've found that allows for sharing of kcd-scripts/config and supporting React Native in react-test-renderer. This ensures that the react-native module gets transformed by Babel, tells Jest to look for sources and tests from the root, but then scopes tests down to only other/react-native/__tests__.

eliperkins · 2018-02-07T22:08:14Z

package.json

@@ -15,6 +15,7 @@
    "lint": "kcd-scripts lint",
    "test": "kcd-scripts test",
    "test:cover": "kcd-scripts test --coverage",
+    "test:native": "kcd-scripts test --config other/react-native/jest.config.js --rootDir .",


Additionally, doing some more reading around Jest and what people use for configs, using a config file combined with --rootDir . was the most flexible way to set this up:
jestjs/jest#2670
jestjs/jest#3613

eliperkins · 2018-02-07T22:10:43Z

other/react-native/BasicAutocomplete.js

+
+import React from 'react'
+import {Text, TextInput, TouchableOpacity, View} from 'react-native'
+import Downshift from '../../dist/downshift.native.cjs'


Ah, CI throws a linting error when importing a *.cjs file, but without specifying this, jest-resolve can't track this module down: https://travis-ci.org/paypal/downshift/builds/338729428#L563

Should we disable the linting rule here?

Yeah. You could probably do the same thing at the bottom of this file as we do at the bottom of this file 👍

kentcdodds

Thanks for this @eliperkins! Just a few thoughts and ideas.

kentcdodds · 2018-02-07T22:41:27Z

other/react-native/BasicAutocomplete.js

+
+import React from 'react'
+import {Text, TextInput, TouchableOpacity, View} from 'react-native'
+import Downshift from '../../dist/downshift.native.cjs'


Yeah. You could probably do the same thing at the bottom of this file as we do at the bottom of this file 👍

kentcdodds · 2018-02-07T22:42:48Z

other/react-native/__tests__/__snapshots__/render-tests.js.snap

@@ -0,0 +1,315 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP


I'm normally not a huge fan of huge snapshots (learn more). But I think this is ok because I don't think we plan on making changes that would affect this snapshot very often anyway. But if there's a way we can avoid these snapshots by making more explicit assertions I think I'd be happier with that.

Agreed. I tried thinking of a better way to get here, instead of using snapshots, but I'm unsure of what other methods we could use to test the React Native renderer.

We could emulate the Preact tests and use a spy to ensure that render successfully gets called, but I don't think that's any better than these snapshots.

I'll see if I can come up some better assertions here.

I ended up writing some tests that were similar to those Preact tests and I feel like it's much clearer as to what we're testing now! 216c531

Let me know what you think.

kentcdodds · 2018-02-07T22:47:50Z

package.json

@@ -23,7 +24,7 @@
    "storybook": "start-storybook -p 6006 -c stories",
    "storybook:build": "cd stories && npm install && cd .. && build-storybook -c stories",
    "setup": "npm install && npm run storybook:build -s && npm run validate",
-    "validate": "kcd-scripts validate lint,build-and-test,test:cover,test:ts,test:ssr",
+    "validate": "kcd-scripts validate lint,build-and-test,test:cover,test:ts,test:ssr,test:native",


We need to make sure that the test:native runs after the build finishes, so let's actually make this part of the npm run test:build script. Could we do this?

"test:build": "jest --projects other/misc-tests other/react-native"

I think that'll work. Might need to make a few changes (like set the rootDir in the config (using the path module?) rather than via a flag). That way it runs both of the tests in parallel, then we don't need to change the validate script, and we also wouldn't need to add a test:native script.

Does that make sense?

I think so!

Man, Jest rocks. Using --projects works perfectly!

kentcdodds · 2018-02-07T22:49:42Z

Thanks so much for your help! I've added you as a collaborator on the project. Please make sure that you review the other/MAINTAINING.md and CONTRIBUTING.md files (specifically the bit about the commit messages and the git hooks) and familiarize yourself with the code of conduct (we're using the contributor covenant). You might also want to watch the repo to be notified when someone files an issue/PR. Please continue to make PRs as you feel the need (you can make your branches directly on the repo rather than your fork if you want). Thanks! And welcome to the team :)

codecov-io · 2018-02-08T16:04:58Z

Codecov Report

Merging #328 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##           master   #328   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines         330    330           
  Branches       85     85           
=====================================
  Hits          330    330

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a80733...4d35baf. Read the comment docs.

eliperkins · 2018-02-08T17:36:21Z

other/react-native/__tests__/render-tests.js

+    }),
+  )
+  const tree = renderer.toJSON()
+  expect(tree).toMatchSnapshot()


We could remove this snapshot, but I think ensuring that we render the basic set of React Native components here, while applying the functions from Downshift, is a fair trade-off for using a snapshot.

kentcdodds

Looking great! Just a few questions.

kentcdodds · 2018-02-08T19:02:34Z

other/react-native/__tests__/render-tests.js

+  expect(renderSpy).toHaveBeenCalledTimes(1)
+})
+
+test('getInputProps composes onChange with onChangeText', () => {


I think this is more incidental. I think most people will want to be able to pass onChangeText instead of onChange if that's what they're used to. Is that currently possible? Could we have our tests ensure that?

Ah, this is actually a poor test, and one difference between React Native and ReactDOM that should be documented.

onInput is not a valid prop for TextInput, but in this case, Downshift is still composing the onChange prop with a new function for onInput, and then calling both. The onInput prop has no affect on the TextInput in this case.

To emulate this behavior on React Native, perhaps we would compose onChange with onChangeText, where onChangeText is just a convenience wrapper for onChange(event.nativeEvent.text). There's no other events that come down through the onChange prop on TextInput, like there would be on <input> in ReactDOM; TextInput has the prop onSelectChange to handle things like that.

When I get to documenting this difference (as we talked about in #265), I'll be sure to call this out. We might even be able to optimize this a bit for React Native by forgoing any prop composition, since this case isn't quite valid for us anyway.

Right now, the behavior of the Downshift RN implementation is for getInputProps to accept onChange as the function and apply it to the TextInput as onChangeText. Since onChange and onChangeText have different arguments, perhaps it'd be better to make a special case for this in the RN implementation that allows for using either function with getInputProps and applying it accordingly.

What do you think?

I want people to assume that the arguments to the prop getters to be the props they want applied. So if normally they would use onChangeText={this.handleOnChangeText} in their JSX, then they should use onChangeText: this.handleOnChangeText in the function call.

What's the status here?

Ah, sorry, the status of this is that I went on vacation for a bit 😬

I think there's an API change that'll need to happen to address this (for React Native). When I originally implemented this React Native support, I didn't have a full grasp of the Downshift API, so I allowed for the same onChange prop to be supplied, but then used it as onChangeText.

I think the correct API way forward would be to support both onChange and onChangeText, but that may be out of the scope of this PR, which is to simply add a React Native test suite.

I'll make an issue to track this.

I filed #361 to track this for now. I'll remove this test since it's not entirely valid.

kentcdodds · 2018-02-08T19:03:06Z

package.json

@@ -18,7 +18,7 @@
    "test:ssr": "kcd-scripts test --config other/ssr/jest.config.js --no-watch",
    "test:update": "npm run test:cover -s -- --updateSnapshot",
    "test:ts": "tsc --noEmit -p ./tsconfig.json",
-    "test:build": "kcd-scripts test --config other/misc-tests/jest.config.js --no-watch",
+    "test:build": "jest --projects other/misc-tests other/react-native --no-watch",


Can probably remove the --no-watch from this.

This adds a rudimentary test suite for the React Native build of Downshift, leveraging Jest snapshots. The goal of this suite is to ensure that Downshift can render React Native components, without calling into methods that are specific to ReactDOM.

Since we're using a unique "file extension" here (read: `downshift.native.cjs.js`), ESLint thinks the module isn't a valid module.

This leverages Jest projects to use both different configs for misc-tests and react-native

This emulates the style of the Preact tests, ensuring that we can use different parts of the Downshift API in another environment.

This is the default behavior for `jest` anyway.

This test doesn't assert the intended behavior, which is composing onChange and onChangeText

kentcdodds

This is super! Thanks!

* Add snapshot test suite for React Native support This adds a rudimentary test suite for the React Native build of Downshift, leveraging Jest snapshots. The goal of this suite is to ensure that Downshift can render React Native components, without calling into methods that are specific to ReactDOM. * Disable ESLint import/extensions and import/no-unresolved Since we're using a unique "file extension" here (read: `downshift.native.cjs.js`), ESLint thinks the module isn't a valid module. * Add React Native test to test:build command This leverages Jest projects to use both different configs for misc-tests and react-native * Create explicit assertions for React Native test cases This emulates the style of the Preact tests, ensuring that we can use different parts of the Downshift API in another environment. * Remove unnecessary --no-watch argument This is the default behavior for `jest` anyway. * Remove invalid test for React Native This test doesn't assert the intended behavior, which is composing onChange and onChangeText * Update snapshots * Add test to call onChangeText on TextInput components for React Native

eliperkins commented Feb 7, 2018

View reviewed changes

kentcdodds reviewed Feb 7, 2018

View reviewed changes

eliperkins commented Feb 8, 2018

View reviewed changes

kentcdodds reviewed Feb 8, 2018

View reviewed changes

eliperkins force-pushed the react-native-tests branch from 216c531 to ac62da5 Compare February 8, 2018 19:19

eliperkins mentioned this pull request Mar 6, 2018

Support onChange and onChangeText props for React Native #361

Closed

eliperkins added 5 commits March 6, 2018 16:52

Disable ESLint import/extensions and import/no-unresolved

2f6df10

Since we're using a unique "file extension" here (read: `downshift.native.cjs.js`), ESLint thinks the module isn't a valid module.

Add React Native test to test:build command

c2d4c95

This leverages Jest projects to use both different configs for misc-tests and react-native

Create explicit assertions for React Native test cases

c223adc

This emulates the style of the Preact tests, ensuring that we can use different parts of the Downshift API in another environment.

Remove unnecessary --no-watch argument

e5e9c44

This is the default behavior for `jest` anyway.

eliperkins force-pushed the react-native-tests branch from ac62da5 to e5e9c44 Compare March 6, 2018 21:54

eliperkins added 3 commits March 6, 2018 16:57

Remove invalid test for React Native

21baedb

This test doesn't assert the intended behavior, which is composing onChange and onChangeText

Update snapshots

4619b81

Add test to call onChangeText on TextInput components for React Native

4d35baf

kentcdodds approved these changes Mar 6, 2018

View reviewed changes

kentcdodds merged commit 87e8827 into downshift-js:master Mar 6, 2018

eliperkins deleted the react-native-tests branch March 26, 2018 19:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add snapshot test suite for React Native support #328

Add snapshot test suite for React Native support #328

eliperkins commented Feb 7, 2018 •

edited

Loading

eliperkins Feb 7, 2018

eliperkins Feb 7, 2018

eliperkins Feb 7, 2018

kentcdodds Feb 7, 2018

kentcdodds left a comment

kentcdodds Feb 7, 2018

kentcdodds Feb 7, 2018

eliperkins Feb 8, 2018 •

edited

Loading

eliperkins Feb 8, 2018

kentcdodds Feb 7, 2018

eliperkins Feb 8, 2018 •

edited

Loading

eliperkins Feb 8, 2018

kentcdodds commented Feb 7, 2018

codecov-io commented Feb 8, 2018 •

edited

Loading

eliperkins Feb 8, 2018

kentcdodds left a comment

kentcdodds Feb 8, 2018

eliperkins Feb 8, 2018

kentcdodds Feb 8, 2018

kentcdodds Mar 3, 2018

eliperkins Mar 6, 2018

eliperkins Mar 6, 2018

kentcdodds Feb 8, 2018

kentcdodds left a comment

Add snapshot test suite for React Native support #328

Add snapshot test suite for React Native support #328

Conversation

eliperkins commented Feb 7, 2018 • 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

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliperkins Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliperkins Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds commented Feb 7, 2018

codecov-io commented Feb 8, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

kentcdodds left a 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

kentcdodds left a comment

Choose a reason for hiding this comment

eliperkins commented Feb 7, 2018 •

edited

Loading

eliperkins Feb 8, 2018 •

edited

Loading

eliperkins Feb 8, 2018 •

edited

Loading

codecov-io commented Feb 8, 2018 •

edited

Loading