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

πŸ’š Enable babel transpiling of react-native as well #1193

Merged
merged 4 commits into from
Dec 21, 2024

Conversation

shankari
Copy link
Contributor

This fixes the code coverage github action.

Without this change, we were getting the error

i FAIL  www/__tests__/DateSelect.test.tsx
  ● Test suite failed to run

    SyntaxError: /private/tmp/e-mission-phone/node_modules/react-native/Libraries/vendor/emitter/EventEmitter.js: Unexpected token, expected "]" (39:5)

The last successful run was from Oct 17.
The first failed run was from Oct 23.

We have not made any changes to the react-native components that we directly import since Sept 2024. However, we use a carat semver, so we will pull newly released minor versions. One of our dependencies bumped up the version of RN that they use, and released a new minor version, which broke this.

After fruitlessly searching around for a root cause (as documented in the PR comments), since we import a lot of components and it was laborious to check when each of them have been updated, I did a brute force search in GitHub for this error and found both the same issue
facebook/react-native#48228 and a fix that worked
facebook/react-native#48228 (comment)

I am surprised this has not been reported widely, but I guess it is fairly new! Thanks to @Basil-Code for the suggestion!

This fixes the code coverage github action.

Without this change, we were getting the error

```
i FAIL  www/__tests__/DateSelect.test.tsx
  ● Test suite failed to run

    SyntaxError: /private/tmp/e-mission-phone/node_modules/react-native/Libraries/vendor/emitter/EventEmitter.js: Unexpected token, expected "]" (39:5)
```

The last successful run was from Oct 17.
The first failed run was from Oct 23.

We have not made any changes to the react-native components that we directly
import since Sept 2024. However, we use a carat semver, so we will pull newly
released minor versions. One of our dependencies bumped up the version of RN
that they use, and released a new minor version, which broke this.

After fruitlessly searching around for a root cause (as documented in the PR
comments), since we import a lot of components and it was laborious to check
when each of them have been updated, I did a brute force search in GitHub for
this error and found both the same issue
facebook/react-native#48228
and a fix that worked
facebook/react-native#48228 (comment)

I am surprised this has not been reported widely, but I guess it is fairly new!
Thanks to @Basil-Code for the suggestion!
@shankari
Copy link
Contributor Author

I thought this would be a simple fix so I didn't file an issue to track progress.
It is a simple fix (one-line), but the investigation was long and complex, so documenting it here for the record.

@shankari
Copy link
Contributor Author

Similar issues were apparently caused due to not using the correct version of node
puppeteer/puppeteer#10509

But bumping up node to 22.12.0 did not fix anything

Copy link

codecov bot commented Dec 21, 2024

Codecov Report

All modified and coverable lines are covered by tests βœ…

Project coverage is 29.78%. Comparing base (ee85fb0) to head (e9abaee).
Report is 29 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1193      +/-   ##
==========================================
- Coverage   29.98%   29.78%   -0.20%     
==========================================
  Files         123      123              
  Lines        4933     4955      +22     
  Branches     1091     1142      +51     
==========================================
- Hits         1479     1476       -3     
- Misses       3454     3477      +23     
- Partials        0        2       +2     
Flag Coverage Ξ”
unit 29.78% <ΓΈ> (-0.20%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 3 files with indirect coverage changes

@shankari
Copy link
Contributor Author

We have not made any changes to the react-native components since Sept 2024. Did this break around that
time?

2eab6c5

No, the last successful run was from Oct 17.
The first failed run was from Oct 23.

The most likely cause would be an underlying library change, but we have pinned dependencies.

Where does EventEmitter.js come from anyway? It is (c) Meta, and is in the
node_modules/react-native/Libraries/vendor/emitter directory so appears to
be part of the core react-native. Wait, core react-native implements the interface IEventEmitter through NativeEventEmitter. Since we use RNweb, I wonder if this is coming from that package.

Nope, RNWeb just uses the EventEmitter.
https://github.com/search?q=repo%3Anecolas%2Freact-native-web%20EventEmitter&type=code

Aha! the vendor directory means that it comes from somewhere else
https://stricker.digital/posts/how-to-vendor-npm-dependencies/

So we do have to focus on the react-native package.
However, I could not figure out where this vendor package was specified, although I checked the package.json

We do note that the versions are specified with a carat ^, so there could have been an upgrade of the version as long as it was a minor version.

Alas, since I don't have an older instance of the "serve" so I can't compare to see what the pinned versions were earlier. But I do note that we have node_modules/@react-native-community/cli, version 11.3.10 in the native repos and node_modules/@react-native/community-cli-plugin, version 0.76.5 locally.

@shankari
Copy link
Contributor Author

I do see the error

There seems to be an issue with your configuration that prevents React Native Testing Library from working correctly.
    Please check if you are using compatible versions of React Native and React Native Testing Library.

RNtesting library comes directly from this project

[email protected] /private/tmp/e-mission-phone
β”œβ”€β”€ @testing-library/[email protected]

We get react-native from

β”œβ”€β”¬ [email protected]
β”‚ β”œβ”€β”€ [email protected] deduped
β”œβ”€β”¬ [email protected]
β”‚ β”œβ”€β”¬ [email protected]
β”‚ β”‚ β”œβ”€β”€ [email protected] deduped
β”‚ β”œβ”€β”€ [email protected] deduped
β”œβ”€β”¬ [email protected]
β”‚ β”œβ”€β”€ [email protected] deduped
β”œβ”€β”¬ [email protected]
β”‚ β”œβ”€β”€ [email protected] deduped
β”œβ”€β”¬ @react-navigation/[email protected]
β”‚ β”œβ”€β”¬ [email protected]
β”‚ β”‚ β”œβ”€β”¬ @react-native/[email protected]
β”‚ β”‚ β”‚ β”œβ”€β”€ [email protected] deduped
β”œβ”€β”¬ @react-navigation/[email protected]
β”‚ β”‚ β”œβ”€β”€ [email protected] deduped

React native testing library was released Nov 27. What if we forcibly downgrade it to 12.3.0?

downgraded, still failing

[email protected] /private/tmp/e-mission-phone
β”œβ”€β”€ @testing-library/[email protected]

@shankari
Copy link
Contributor Author

Also found https://dev.to/bytebodger/how-i-fixed-the-unexpected-token-error-in-jest-4o1j and tried to change the command to jest --transformIgnorePatterns \"node_modules/(?!react-native)/\" --env=jsdom but that didn't work.

Running the test using `npx jest` instead of `npm run test`
means that passing in command line arguments to jest needs to either be coded
in two places, or that jest would be potentially invoked inconsistently.

This changes the invocation to use `npm` so that the command line args (if any)
can be passed in to a single location.
e-mission#1193 (comment)
shankari added a commit to shankari/e-mission-phone that referenced this pull request Dec 21, 2024
We use `^` versions for our dependencies, which means that any compatible minor
version could be pulled. However, we recently ran into an issue
(e-mission#1193 (comment))
in which a change to a minor version broke our tests. Sifting through all our
dependencies to find the ones that were bumped, and which dependencies they
bumped in turn is time consuming.

For example in the issue above, the dependency that caused the break was
`react-native`, which we don't include directly in our project.

One option to identify changes would be to check the `package-lock.json`, but
unless we are constantly reinstalling npm, we won't necessarily know what
changed around the time that the tests broke.

Uploading the `package-lock.json` from runs allows us to see the versions from
before and after the break and narrow down the potential changes.
We use `^` versions for our dependencies, which means that any compatible minor
version could be pulled. However, we recently ran into an issue
(e-mission#1193 (comment))
in which a change to a minor version broke our tests. Sifting through all our
dependencies to find the ones that were bumped, and which dependencies they
bumped in turn is time consuming.

For example in the issue above, the dependency that caused the break was
`react-native`, which we don't include directly in our project.

One option to identify changes would be to check the `package-lock.json`, but
unless we are constantly reinstalling npm, we won't necessarily know what
changed around the time that the tests broke.

Uploading the `package-lock.json` from runs allows us to see the versions from
before and after the break and narrow down the potential changes.
We currently run tests and check code coverage in the same workflow,
and fail if the code coverage goes down. But our current coverage is fairly
limited (< 20%) so we have a lot of failures from changes to code that
currently does not have any tests.

Let's still check the code coverage for the patch, but disable it for the
project, so it doesn't mask errors in the tests.

We should not have to enable this since
https://docs.codecov.com/docs/common-recipe-list#set-project-coverage-checks-on-a-pull-request
> By default, Codecov will only show git diff coverage checks on a PR.
> "Project coverage" checks and "project coverage" reporting is not available for Private Repos on the Codecov team plan.
> For all other private repos, and for all public repos, here's how you can also show project coverage checks on a PR.

But it is clearly failing and is the only test failing, so let's make the
change anyway.

Once we improve coverage to cover more areas, we can re-enable this.
@shankari shankari force-pushed the fix_javascript_tests branch from 2c975b8 to e9abaee Compare December 21, 2024 21:18
@shankari
Copy link
Contributor Author

@JGreenlee for visibility

@shankari shankari merged commit 89127bc into e-mission:master Dec 21, 2024
7 checks passed
@shankari shankari deleted the fix_javascript_tests branch December 21, 2024 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Tasks completed
Development

Successfully merging this pull request may close these issues.

1 participant