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

Fix/jest mock #295

Merged
merged 3 commits into from
Apr 6, 2022
Merged

Fix/jest mock #295

merged 3 commits into from
Apr 6, 2022

Conversation

fortmarek
Copy link
Contributor

@fortmarek fortmarek commented Apr 6, 2022

Description

We got a report from Shopify mobile that their tests are failing with our new FlashList mock. That is because we assumed we can always obtain the scrollViewComponent - but if there was no data, we display the empty component directly, never displaying any scroll view.

I fixed this by triggering automatically onLayout with optional chaining, so the code does not throw an exception when scroll view does not exist.

Checklist

@@ -2,7 +2,8 @@ jest.mock("@shopify/flash-list", () => {
const ActualFlashList = jest.requireActual("@shopify/flash-list").FlashList;
class MockFlashList extends ActualFlashList {
componentDidMount() {
this.rlvRef._scrollComponent._scrollViewRef.props.onLayout({
super.componentDidMount();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

onLayout was not called if data was empty since we never called the super implementation.

@@ -335,7 +335,7 @@ class FlashList<T> extends React.PureComponent<
return (
<View style={{ flex: 1 }}>
{this.header()}
{this.props.ListEmptyComponent || null}
{this.getValidComponent(this.props.ListEmptyComponent)}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before the change, it was invalid to pass a function component (instead of component itself directly)

import Warnings from "../errors/Warnings";
import AutoLayoutView from "../AutoLayoutView";

jest.mock("../FlashList", () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

using a similar mock as in jestSetup.js to better test the code there. We can't reuse the code directly due to different imports.

Copy link
Contributor

Choose a reason for hiding this comment

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

What do different imports mean? Curious

Copy link
Contributor Author

Choose a reason for hiding this comment

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

import ../FlashList is different from import @shopify/flash-list and jest needs to know which it is in advance.

@@ -181,4 +193,15 @@ describe("FlashList", () => {
elapsedTimeInMs: expect.any(Number),
});
});

it("loads an empty state", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, I wonder why a similar e2e test fails finding FlashList component when EmptyComponent is rendered

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One reason could be that we are building the release configuration for e2e tests and React will make optimizations that it won't do in the dev environment (which jest uses)

Copy link
Contributor

@ElviraBurchik ElviraBurchik left a comment

Choose a reason for hiding this comment

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

LGTM

@fortmarek fortmarek merged commit 285fd3e into main Apr 6, 2022
@fortmarek fortmarek deleted the fix/jest-mock branch April 6, 2022 13:32
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.

2 participants