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.

Sign up for GitHub

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

Jump to bottom

feat: Add @testing-library/react-native #5197

Merged
merged 16 commits into from
Aug 3, 2021
Merged

feat: Add @testing-library/react-native #5197

merged 16 commits into from
Aug 3, 2021

Conversation

gkartalis
Copy link
Member

The type of this PR is: Enhancement

This PR resolves []

Description

  • Added @testing-library/react-native
  • Fixed problems with timers
  • Fixed relay mockenvironment problems that were surfaced

Another thing that is worth mentioning here is that the version is one minor release behind the latest cause of this issue fixed in this PR but not yet released.

Problems

The problem at the moment is that 4 tests are failing more specifically:

  1. OrderDetails-tests.tsx
  2. RegistrationFlow-tests.tsx
  3. MyBids-tests.tsx
  4. Consignments/tests/index-tests.tsx

~~Reason: ~~

Failed: "TypeError: Cannot read property '_responseForQueryIDWithVariables' of undefined"
Failed: "TypeError: Cannot read property '_setResponseForQueryIDWithVariables' of undefined"

These two come from the following:

Artsy/eigen/src/lib/NativeModules/GraphQLQueryCache.ts
Artsy/eigen/emission/Pod/Classes/GraphQL/ARGraphQLQueryCache.m

The above where solved when added these lines to mock createEnvironment to setupJest.ts.

PR Checklist (tick all before merging)

  • I have included screenshots or videos to illustrate my changes, or I have not changed anything that impacts the UI.
  • I have tested my changes on iOS and Android.
  • I have added tests for my changes, or my changes don't require testing, or I have included a link to a separate Jira ticket covering the tests.
  • I have added a feature flag, or my changes don't require a feature flag. (How do I add one?)
  • I have documented any follow-up work that this PR will require, or it does not require any.
  • I have added an app state migration, or my changes do not require one. (What are migrations?)
  • I have added a changelog entry below or my changes do not require one.
Changelog updates

Changelog updates

Cross-platform user-facing changes

iOS user-facing changes

Android user-facing changes

Dev changes

  • introduces @testing-library/react-native - gkartalis

@ArtsyOpenSource
Copy link
Contributor

ArtsyOpenSource commented Jul 29, 2021 •
edited
Loading

This PR contains the following changes:

  • Dev changes (introduces @testing-library/react-native - gkartalis)

Generated by 🚫 dangerJS against 057205d

@gkartalis gkartalis force-pushed the gkartalis/@testing-library/react-native branch from 4a6a7e4 to 45dcd3f Compare July 29, 2021 21:49
@gkartalis gkartalis self-assigned this Jul 30, 2021
@lordkiz
Copy link
Contributor

lordkiz commented Jul 30, 2021

Looking forward to this @gkartalis 🙌

damassi
damassi reviewed Jul 30, 2021
View reviewed changes
src/lib/Scenes/VanityURL/__tests__/VanityURLPossibleRedirect-tests.tsx
@@ -13,18 +13,18 @@ import { VanityURLPossibleRedirect } from "../VanityURLPossibleRedirect"

beforeEach(() => {
__globalStoreTestUtils__?.setProductionMode()
fetchMock.mockReset()
fetchMock.resetMocks()
Copy link
Member

Choose a reason for hiding this comment

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

Know this one well 😄

pvinis
pvinis reviewed Jul 30, 2021
View reviewed changes
src/lib/Scenes/Sale/__tests__/Sale-tests.tsx
@@ -60,10 +59,11 @@ describe("Sale", () => {
)

expect(navigate).toHaveBeenCalledTimes(0)
jest.advanceTimersByTime(1000)
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

pvinis
pvinis reviewed Jul 30, 2021
View reviewed changes
src/setupJest.ts
Comment on lines +49 to +52
jest.mock("lib/relay/createEnvironment", () => ({
defaultEnvironment: require("relay-test-utils").createMockEnvironment(),
}))

Copy link
Contributor

Choose a reason for hiding this comment

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

Consistency feels nice :D

Copy link
Contributor

Choose a reason for hiding this comment

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

(just tagging @pepopowitz so he can check the solution!)

pvinis
pvinis reviewed Jul 30, 2021
View reviewed changes
src/lib/Scenes/OrderHistory/OrderDetails/__tests__/OrderDetails-tests.tsx
Comment on lines -126 to -129
it("Loads OrderHistoryQueryRender with PageWithSimpleHeader", () => {
const tree = renderWithWrappers(<OrderDetailsQueryRender orderID="21856921-fa90-4a36-a17e-dd52870952d2" />)
expect(tree.root.findAllByType(PageWithSimpleHeader)).toHaveLength(1)
})
Copy link
Contributor

Choose a reason for hiding this comment

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

For clarification, this test is removed because it tests the exact same thing as the test above.

pvinis
pvinis approved these changes Jul 30, 2021
View reviewed changes
Copy link
Contributor

@pvinis pvinis left a comment

Choose a reason for hiding this comment

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


So good!! Thanks for sticking with it and figuring it out!

@gkartalis
Copy link
Member Author

@pvinis only one thing that I didn't finish yet (maybe we can add a ticket for that) is changing the way that we are mocking fetch inside AuthModel-tests.ts cause the tests where too many and spent a lot of time without getting anywhere.

Happy to write the ticket! Is it okay to merge?

@pvinis
Copy link
Contributor

pvinis commented Aug 2, 2021

I think we should merge this, yes!

It would be great if you could make the ticket, yes, thank you! That ticket should probably apply to https://github.com/artsy/eigen/pull/5208/files#diff-6ea405af9cca72603df862425acd31b1762cf1f94a9247adde1f91119b89f302R14-R21 too, so I would make it to that we change the "old" way of fetch mocking to the "new", and point out a couple of files that use the old still, and give some code that whoever picks the ticket can "search in project" and find all the places.

Thanks again for your work on this! Merge whenever you like.

@gkartalis gkartalis merged commit aad8add into master Aug 3, 2021
@gkartalis gkartalis deleted the gkartalis/@testing-library/react-native branch August 3, 2021 08:01
Gon4ar5 pushed a commit that referenced this pull request Oct 27, 2021
@gkartalis @pvinis
feat: Add @testing-library/react-native (#5197)
93784b8 
* deps: add rn-testing-library dependencies

* chore: setup rn-testing-library

* feat: add renderWithWrappersTL

* fix: changed rntl version

* chore: removed test that doesn't add value

* fix: removed unused import

* chore: testing out stuff

* chore: remove unused import

* fix: fixed timer problems

* spec: moved relay createEnv mock to setupJest

* chore: cleanup

* fix: refactor fetchmock and added helper

Co-authored-by: Pavlos Vinieratos <pvinis@gmail.com>

* chore: fix url

* fix: attempt to fix ci failing test

* fix: fetch await problem ci fix

* chore: use jest-fetch-mock for mocking fetch

Co-authored-by: Pavlos Vinieratos <pvinis@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@damassi damassi damassi left review comments

@pvinis pvinis pvinis approved these changes

@MounirDhahri MounirDhahri Awaiting requested review from MounirDhahri

@lordkiz lordkiz Awaiting requested review from lordkiz

Assignees

@gkartalis gkartalis

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gkartalis @ArtsyOpenSource @lordkiz @pvinis @damassi