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

Add mocks to fix: handleException is not a function (#29849) #30027

Closed
wants to merge 1 commit into from

Conversation

lukewlms
Copy link

@lukewlms lukewlms commented Sep 24, 2020

Summary

This solves two problems:

  1. When an exception is thrown by RN code during testing (e.g. with react-native-testing-library), the exception message is swallowed and instead this message is shown:

ExceptionsManager.handleException is not a function

  1. The inline require call that is removed in this commit is also incompatible with running a test suite (at least in react-native-testing-library) because the on-the-fly require fails. This throws the error:

ReferenceError: You are trying to import a file after the Jest environment has been torn down.

See issue: #29849

Changelog

[General] [Fix] - Fix issue where exceptions thrown during test runs could get swallowed and result in these messages:

  • ExceptionsManager.handleException is not a function
  • ReferenceError: You are trying to import a file after the Jest environment has been torn down.

Test Plan

Before: an exception generated during our test suite is swallowed and shows the errors above
After: no longer repros. exceptions logged during a test run output their messages to the console as expected

(I def would like verification from a code owner here, this code makes it work but I can't guarantee it's optimal. Thanks!)

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 24, 2020
@pull-bot
Copy link

Messages
📖

📋 Verify Changelog Format - A changelog entry has the following format: [CATEGORY] [TYPE] - Message.

CATEGORY may be:
  • General
  • iOS
  • Android
  • JavaScript
  • Internal (for changes that do not need to be called out in the release notes)

TYPE may be:

  • Added, for new features.
  • Changed, for changes in existing functionality.
  • Deprecated, for soon-to-be removed features.
  • Removed, for now removed features.
  • Fixed, for any bug fixes.
  • Security, in case of vulnerabilities.

MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.

Generated by 🚫 dangerJS against 3ea724d

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: abff021

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,367,878 0
android hermes armeabi-v7a 6,994,962 0
android hermes x86 7,808,374 0
android hermes x86_64 7,699,928 0
android jsc arm64-v8a 9,512,923 0
android jsc armeabi-v7a 9,132,155 0
android jsc x86 9,379,011 0
android jsc x86_64 9,959,933 0

Base commit: abff021

@Panda-ref
Copy link

Panda-ref commented Sep 25, 2020

It'll result in

Require cycle: node_modules/react-native/Libraries/Core/ExceptionsManager.js ->
 node_modules/react-native/Libraries/LogBox/Data/LogBoxData.js ->
 node_modules/react-native/Libraries/Core/ExceptionsManager.js

@@ -23,6 +23,9 @@ import type {
import parseErrorStack from '../../Core/Devtools/parseErrorStack';
import type {ExtendedError} from '../../Core/Devtools/parseErrorStack';
import NativeLogBox from '../../NativeModules/specs/NativeLogBox';

const ExceptionsManager = require('../../Core/ExceptionsManager');
Copy link

@eliw00d eliw00d Oct 2, 2020

Choose a reason for hiding this comment

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

This was originally in reportLogBoxError to avoid a circular dependency. If you move it back into reportLogBoxError, but keep the mock in jest/setup, does it still solve the issue?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for taking a look. In my experience this change is necessary to avoid Jest throwing:

ReferenceError: You are trying to import a file after the Jest environment has been torn down.

I'm not sure how to resolve the circular dependency issue :\

@wcandillon
Copy link
Contributor

I can confirm that this fixes the issue, how can we follow in which version of RN this fix will land?

@owencraston
Copy link

It would be great to get this error blocks a lot of apps that have testing infrastructure set up to upgrade.

@lukewlms lukewlms changed the title Mocks to fix: handleException is not a function (#29849) Add mocks to fix: handleException is not a function (#29849) Oct 23, 2020
@safaiyeh
Copy link
Contributor

@rickhanlonii could you take a look at this?

Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

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

Hey @lukewlms, thanks for working on this!

Looks like the tests are failing and @eliw00d is correct that we will need some other way to resolve the circular dependency.

@semcelik
Copy link

Is there any update or workaround on this bug?

@lukewlms
Copy link
Author

@semcelik Yes, this workaround is correcting the issue for us.

@github-actions
Copy link

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Jul 28, 2023
@github-actions
Copy link

github-actions bot commented Aug 5, 2023

This PR was closed because it has been stalled for 7 days with no activity.

@github-actions github-actions bot closed this Aug 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Stale There has been a lack of activity on this issue and it may be closed soon.
Projects
None yet
Development

Successfully merging this pull request may close these issues.