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 integration tests for console errors + ExceptionManager #46636

Closed
wants to merge 2 commits into from

Conversation

rickhanlonii
Copy link
Member

Summary:
Adds more integration tests for LogBox (currently incorrect, but fixed in a later diff).

Changelog: [Internal]

Differential Revision: D63349614

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner labels Sep 24, 2024
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D63349614

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D63349614

rickhanlonii added a commit to rickhanlonii/react-native that referenced this pull request Sep 25, 2024
…#46636)

Summary:
Pull Request resolved: facebook#46636

Adds more integration tests for LogBox (currently incorrect, but fixed in a later diff).

Changelog: [Internal]

Reviewed By: javache

Differential Revision: D63349614
Differential Revision: D63349616
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D63349614

rickhanlonii added a commit to rickhanlonii/react-native that referenced this pull request Sep 25, 2024
…#46636)

Summary:
Pull Request resolved: facebook#46636

Adds more integration tests for LogBox (currently incorrect, but fixed in a later diff).

Changelog: [Internal]

Reviewed By: javache

Differential Revision: D63349614
…#46636)

Summary:
Pull Request resolved: facebook#46636

Adds more integration tests for LogBox (currently incorrect, but fixed in a later diff).

Changelog: [Internal]

Reviewed By: javache

Differential Revision: D63349614
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D63349614

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Sep 26, 2024
@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 2f85e28.

blakef pushed a commit that referenced this pull request Oct 7, 2024
Summary:
Pull Request resolved: #46636

Adds more integration tests for LogBox (currently incorrect, but fixed in a later diff).

Changelog: [Internal]

Reviewed By: javache

Differential Revision: D63349614

fbshipit-source-id: 8f5c6545b48a1ed18aea08d4ecbecd7a6b9fa05a
blakef added a commit that referenced this pull request Oct 7, 2024
- "Fix errors with component stacks reported as warnings (#46637)": 2da46a8
- "Refactor LogBox tests to spies (#46638)": 8926364
- "Add integration tests for console errors + ExceptionManager (#46636)": 094f036
- "Re-enable integration tests (#46639)": bf40710
blakef pushed a commit that referenced this pull request Oct 30, 2024
Summary:
Pull Request resolved: #46636

Adds more integration tests for LogBox (currently incorrect, but fixed in a later diff).

Changelog: [Internal]

Reviewed By: javache

Differential Revision: D63349614

fbshipit-source-id: 8f5c6545b48a1ed18aea08d4ecbecd7a6b9fa05a
@blakef blakef mentioned this pull request Oct 30, 2024
cipolleschi pushed a commit that referenced this pull request Oct 31, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
* Re-enable integration tests (#46639)

Summary:
Pull Request resolved: #46639

These tests were skipped when we were switching to component stacks, which also hid a bug later in the stack. Re-enable them.

Changelog: [Internal]

Reviewed By: javache

Differential Revision: D63349616

fbshipit-source-id: ccde7d5bb3fcd9a27adf4af2068a160f02f7432a

* Add integration tests for console errors + ExceptionManager (#46636)

Summary:
Pull Request resolved: #46636

Adds more integration tests for LogBox (currently incorrect, but fixed in a later diff).

Changelog: [Internal]

Reviewed By: javache

Differential Revision: D63349614

fbshipit-source-id: 8f5c6545b48a1ed18aea08d4ecbecd7a6b9fa05a

* Refactor LogBox tests to spies (#46638)

Summary:
Pull Request resolved: #46638

This is annoying, but in the next diff that fixes a bug I need to test using the default warning filter instead of a mock (really, all this mocking is terrible, idk why I did it this way).

Unfortunately, in Jest you can't just reset mocks from `jest.mock`, `restoreMocks` only resets spies and not mocks (wild right).

So in this diff I converted all the `jest.mock` calls to `jest.spyOn`. I also corrected some of the mocks that require `monitorEvent: 'warning',` like the warning filter sets.

I also added a test that works without the fix.

Changelog: [Internal]

Reviewed By: javache

Differential Revision: D63349615

fbshipit-source-id: 4f2a5a8800c8fe1a10e3613d3c2d0ed02fca773e

* Fix errors with component stacks reported as warnings (#46637)

Summary:
Ok so this is a doozy.

## Overview
There was a report that some console.error calls were being shown as warnings in LogBox but as console.error in the console. The only time we should downlevel an error to a warning is if the custom warning filter says so (which is used for some noisy legacy warning filter warnings internally).

However, in when I switched from using the `Warning: ` prefix, to using the presence of component stacks, I subtly missed the default warning filter case.

In the internal warning filter, the `monitorEvent` is always set to something other than `unknown` and if it's set to `warning_unhandled` then `suppressDialog_LEGACY` is always false.

However, the default values for the warning filter are that `monitorEvent = 'unknown'` and `suppressDialog_LEGACY = true`. In this case, we would downlevel the error to a warning.

## What's the fix?
Change the default settings for the warning filter.

## What's the root cause?

Bad configuration combinations in a fragile system that needs cleaned up, and really really bad testing practices with excessive mocking and snapshot testing (I can say that, I wrote the tests)

## How could it have been caught?
It was, but I turned off the integration tests while landing the component stack changes because of mismatches between flags internally and in OSS, and never turned them back on.

Changelog: [General] [Fixed] - Fix logbox reporting React errors as Warnings

Pull Request resolved: #46637

Reviewed By: huntie

Differential Revision: D63349613

Pulled By: rickhanlonii

fbshipit-source-id: 32e3fa4e2f2077114a6e9f4feac73673973ab50c

* [LOCAL] using older version of React Dev Tools

- Older version has old URL, updated tests
- Comments on test don't match what's being tested.  Updated.

---------

Co-authored-by: Rick Hanlon <[email protected]>
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. fb-exported Merged This PR has been merged. p: Facebook Partner: Facebook Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants