-
-
Notifications
You must be signed in to change notification settings - Fork 1k
[jest] Fixing enabled prop in buttons #3612
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
Conversation
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.
Have you checked if Pressable
works? 😅
@@ -1,7 +1,16 @@ | |||
jest.mock('./src/RNGestureHandlerModule', () => require('./src/mocks')); | |||
jest.mock('./src/components/GestureButtons', () => require('./src/__mocks__/GestureButtons')); |
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.
As I read in jest docs, __mocks__
directory should be used to mock node modules. User modules can be mocked, but should be placed in the same directory (docs).
Could you check if mocking GestureButtons
like this works? Something tells me that we don't even need this directory for the current use case, but I wouldn't change that.
So to summarize, if we need to create separate file with button mocks, then lets either make it in components/__mocks__
, or create our own mocks
directory and use them for both, current mocks.tsx
and GestureButtons
mock.
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.
fixed, I managed to find an easier solution, we don't need seperate GestureButtons
mock file. However, in the jestSetup we still must mock source files separetely to mocks
jest.mock('./lib/commonjs/RNGestureHandlerModule', () => | ||
require('./lib/commonjs/mocks') | ||
); | ||
jest.mock('./lib/commonjs/components/GestureButtons', () => | ||
require('./lib/commonjs/mocks') |
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.
As far as I can see bob
doesn't generate __mocks__
directory when running yarn build
. That's ok, but does this actually work, or does it work by accident because this file already contains these mocks?
What I want to say - do we need GestureButtons
mock file?
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 checked it, it's not by accident. I managed to find an easier solution, we don't need seperate GestureButtons
mock file. However, in the jestSetup we still must mock source files separetely to mocks
jest.mock('./lib/module/RNGestureHandlerModule', () => | ||
require('./lib/module/mocks') | ||
); | ||
jest.mock('./lib/module/components/GestureButtons', () => | ||
require('./lib/module/mocks') |
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.
Same as above
bd5f5d4
to
88470f8
Compare
@@ -31,14 +31,15 @@ const LongPressGestureHandler = View; | |||
const PinchGestureHandler = View; | |||
const RotationGestureHandler = View; | |||
const FlingGestureHandler = View; | |||
const RawButton = ({ enabled, ...rest }: any) => ( | |||
export const RawButton = ({ enabled, ...rest }: any) => ( | |||
<TouchableNativeFeedback disabled={!enabled} {...rest}> |
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.
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.
This isn't really an issue with this PR
It is not, but it would be great to fix it in this one (cc @akwasniewski)
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.
Fixed, and tested. Now it works as intended
7f5048f
to
85e1a94
Compare
85e1a94
to
60b8285
Compare
0499b7e
to
14053f3
Compare
What about deprecating |
I have nothing against it, since these won't be used anymore once the mocks are fixed. Although it'd be best to notify (c.c. @mdjastrzebski) |
Yes I agree, I wanted to know what do you think about deprecation 😅 |
ecdd6e4
to
e3782a6
Compare
e3782a6
to
98b52db
Compare
Description
Passing
enabled={false}
to buttons in jest tests now has effect. I properly configured the corresponding mocks. Change was motivated by issue #2385.Test plan
Tested using React Native 80.1