Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding tests for React Navigation Drawer Navigator #624

Merged
merged 7 commits into from
Jan 8, 2021
Merged

Adding tests for React Navigation Drawer Navigator #624

merged 7 commits into from
Jan 8, 2021

Conversation

p-syche
Copy link
Contributor

@p-syche p-syche commented Dec 8, 2020

Summary

Testing the Drawer Navigator with the current setup described on the website raised an error:

Zrzut ekranu 2020-12-8 o 14 49 07

I found the solution for this problem in the official React Navigation docs -> testing section. What was missing was a Reanimated mock.

The Stack Navigator test is not raising any errors, so I didn't change it. However passing the jest.mock('react-native/Libraries/Animated/src/NativeAnimatedHelper') through a config file instead of adding it in test files seems like a pretty good solution.

I added the Drawer Navigator test to the website as well.

Test plan

Run yarn test in the example app for React Navigation.

Copy link
Member

@satya164 satya164 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I'd prefer have a single setup instruction instead of one for each navigator. It's nice to have separate examples, but the setup is almost same for all navigators, except some navigators need a Reanimated mock.

examples/reactnavigation/jest.config.js Outdated Show resolved Hide resolved
examples/reactnavigation/src/DrawerApp.js Outdated Show resolved Hide resolved
examples/reactnavigation/src/DrawerApp.js Outdated Show resolved Hide resolved
@@ -0,0 +1,38 @@
import 'react-native-gesture-handler';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This must be done in the entry file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!
I moved the import to the entry file. I also moved the import in the Stack Navigator example file, because it was in the same place.
(I copied the stack navigator example files and changed the navigation for the drawer)

preset: 'react-native',
setupFiles: ['./jest-mocks.js'],
transformIgnorePatterns: [
'node_modules/(?!(jest-)?react-native|@react-native-community|@react-navigation)',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'node_modules/(?!(jest-)?react-native|@react-native-community|@react-navigation)',
'node_modules/(?!(jest-)?react-native|@react-native-community)',

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the @react-navigation here makes the tests fail:

Zrzut ekranu 2020-12-10 o 17 22 07

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I assumed Jest already handled asset files with react-native preset cc @thymikee

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jest doesn't transpile node_modules by default. RN preset adjusts the configuration so that only node_modules other than react-native* and @react-native-community* are not transpiled. @react-navigation is not there, so we need to add it manually.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thymikee I'm talking about assets, React Navigation's code doesn't need to be compiled to run in Node.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

png is passed to a custom transformer: https://github.com/facebook/react-native/blob/master/jest-preset.js#L19 and transformers obey "transformIgnorePatterns", so here's why it fails.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be fixed by using moduleNameMapper instead of transformer: https://jestjs.io/docs/en/webpack#handling-static-assets

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thymikee do you know why it doesn't use moduleNameMapper? probably so people wouldn't have to specify it again when overriding?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why, would have to dig through git blame. Jest handles merging moduleNameMapper gracefully, same as transforms

Install the packages required for React Navigation. For this example, we will use a [drawer navigator](https://reactnavigation.org/docs/drawer-navigator/) to transition between a home screen and an additional screen.

```
$ yarn add @react-native-community/masked-view @react-navigation/native @react-navigation/drawer react-native-gesture-handler react-native-reanimated react-native-safe-area-context react-native-screens
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$ yarn add @react-native-community/masked-view @react-navigation/native @react-navigation/drawer react-native-gesture-handler react-native-reanimated react-native-safe-area-context react-native-screens
$ yarn add @react-navigation/native @react-navigation/drawer react-native-gesture-handler react-native-reanimated react-native-safe-area-context react-native-screens

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@satya164 You are suggesting the masked-view is not needed? It's listed in the install instructions on the React Navigation website: https://reactnavigation.org/docs/getting-started#installing-dependencies-into-a-bare-react-native-project

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's only necessary for stack navigator.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this PR would create one set of instructions for both the stack navigator and the drawer navigator - should I just leave the masked view here? Or add instructions for the stack navigator only?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me

const { Screen, Navigator } = createDrawerNavigator();

export default function Navigation() {
const options = {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems unnecessary

import React from 'react';
import {createDrawerNavigator} from '@react-navigation/drawer';

const { Screen, Navigator } = createDrawerNavigator();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to use the style followed in official docs without destructring

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@p-syche p-syche requested a review from thymikee January 7, 2021 22:18
@p-syche
Copy link
Contributor Author

p-syche commented Jan 7, 2021

I hope this can get merged now :)

@thymikee thymikee merged commit 199de62 into callstack:master Jan 8, 2021
@thymikee
Copy link
Member

thymikee commented Jan 8, 2021

Thanks @p-syche!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants