-
Notifications
You must be signed in to change notification settings - Fork 43
Configure Jest for mobile testing #3206
base: main
Are you sure you want to change the base?
Conversation
It looks like there may be some changes to native mobile code, which requires triggering a full app release. Please follow the instructions here: https://www.notion.so/audiusproject/When-to-bump-app-version-2644a8f772364a4d91f44abcba44ce0b?pvs=4. cc @nicoback2 @sliptype |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
amazing work getting this set up! excited to add more frameworks and see where we can take this.
@@ -22,7 +22,9 @@ export class SolanaClient { | |||
try { | |||
this.connection = new Connection(solanaClusterEndpoint!, 'confirmed') | |||
} catch (e) { | |||
console.error('Could create Solana RPC connection', e) | |||
if (process.env.JEST_WORKER_ID === undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fun! just wondering what the idea is here? is this to avoid extra logs when running tests? would this be the way we would handle logs like this going forward? Would we want to just mock out SolanaClient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this was to avoid big errors when testing, would probably be better to mock solana client. Let me try that!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also curious about using JEST_WORKER_ID? I thought JEST sets NODE_ENV='test'
. (https://jestjs.io/docs/environment-variables)
@@ -22,7 +22,9 @@ import { | |||
makeInitialState | |||
} from './lineup/reducer' | |||
|
|||
const urlParams = new URLSearchParams(window.location.search) | |||
const urlParams = new URLSearchParams( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah interesting, this is cool to see where we make assumptions. since this is in common... i wonder if we should update this at som point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah most definitely, shouldn't be relying on window.location in the reducer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pattern I've seen most often is to just throw any global access into its own module as a re-export such that you can do
import {location} from './location';
Then it's easy to automock it for testing.
@@ -1,7 +1,23 @@ | |||
module.exports = (api) => { | |||
const babelEnv = api.env() | |||
const plugins = [ | |||
['@babel/plugin-transform-react-jsx', { runtime: 'automatic' }] | |||
['@babel/plugin-transform-react-jsx', { runtime: 'automatic' }], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow amazing work making this work. is this based on an issue we could potentially reference? or maybe some comments? cause this is pretty wild
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, let me add a comment. For reference here is the issue:
streamich/react-use#483
https://github.com/streamich/react-use/blob/master/docs/Usage.md
@@ -0,0 +1,55 @@ | |||
// These are required to be transformed because | |||
// they are ESM and jest doesn't support ESM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ive heard decent things about https://vitest.dev/, i wonder if that would support esm, cause its pretty wild just doesn't. obvs not something for rn, just a long term thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly with the amount of config jest required I would be open to switching, I want to see if vitest supports react-native
|
||
global.navigator = { | ||
userAgent: 'node.js' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow incredible mocks! just checking that we need to manually mock things like @react-native-cookies/cookies
, cause i think if you simply say jest.mock('@react-native-cookies/cookies')
it will try and fill in the blanks and mock every exported module, not sure though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, for some reason the automocks didn't resolve the errors that were being thrown but manually mocking did 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Auto-mocking depends heavily on how the module exports things. I've rarely had it just work, unfortunately.
"@babel/runtime": "7.21.0", | ||
"@react-native-community/eslint-config": "3.2.0", | ||
"@svgr/plugin-svgo": "5.5.0", | ||
"@tsconfig/react-native": "2.0.3", | ||
"@types/array.prototype.flat": "1.2.1", | ||
"@types/bn.js": "5.1.0", | ||
"@types/hls.js": "0.13.0", | ||
"@types/jest": "24.9.1", | ||
"@types/jest": "29.5.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
big changes, nice
packages/web/package-lock.json
Outdated
@@ -35860,4 +35860,4 @@ | |||
} | |||
} | |||
} | |||
} | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wonder if we should keep for this pr?
@@ -6,7 +6,7 @@ import 'react-native' | |||
// Note: test renderer must be required after react-native. | |||
import renderer from 'react-test-renderer' | |||
|
|||
import { App } from 'app/app' | |||
import { App } from './App' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow it actually renders!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is our goal to render the whole app and test it in an integration or e2e style fashion?
In my experience, the smaller the scope for your tests, the less you need to do things like mocking all of the external libraries and browser API usage etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think some high level integration / smoke tests would be good to have, seeing as we have no tests for mobile at all right now. We at least have probers for web. But generally I agree that the scope of most tests should be small, the whole reason I went down this path was to unit test the deep-linking code lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah agreed we can start running smaller tests now, but it's great you covered all the bad cases that we might experience along the way.
It looks like there may be some changes to native mobile code, which requires triggering a full app release. Please follow the instructions here: https://www.notion.so/audiusproject/When-to-bump-app-version-2644a8f772364a4d91f44abcba44ce0b?pvs=4. cc @nicoback2 @sliptype |
It looks like there may be some changes to native mobile code, which requires triggering a full app release. Please follow the instructions here: https://www.notion.so/audiusproject/When-to-bump-app-version-2644a8f772364a4d91f44abcba44ce0b?pvs=4. cc @nicoback2 @sliptype |
Description
ts-jest
which wasn't working due to Flow files with.js
extensions in the react-native codebasereact-use
imports so that browser env specific hooks aren't importedComing next:
Dragons
Added
process.env
checks to determine if running via Jest. Will confirm that doesn't break anything in non-test envsHow Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide repro instructions & any configuration.
How will this change be monitored?
For features that are critical or could fail silently please describe the monitoring/alerting being added.
Feature Flags
Are all new features properly feature flagged? Describe added feature flags.