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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue with Sentry React Native SDK's enableNative variable #3093

Merged
merged 7 commits into from
Jun 13, 2023

Conversation

mateioprea
Copy link
Contributor

@mateioprea mateioprea commented May 30, 2023

馃摙 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

馃摐 Description

This commit addresses a problem with the Sentry React Native SDK where the this.enableNative variable was not correctly reinitialized to true after reinitializing the SDK. The issue arose when calling the closeNativeSdk() method, which set this.enableNative to false. Without reinitializing it to true, subsequent reinitializations of the SDK would prevent stopping the SDK afterwards.

To fix this, I added a line in the initNativeSdk function to explicitly set this.enableNative = true after reinitializing the native SDK.

This change resolves the problem and ensures that the enableNative variable is properly managed during SDK reinitialization, allowing for the correct functioning of the Sentry React Native SDK.

馃挕 Motivation and Context

The motivation behind this change is to ensure that the enableNative variable is correctly managed during SDK reinitialization, allowing for the proper functioning of the Sentry React Native SDK.

This change fixes the issue and ensures that the enableNative variable is reset to true after reinitializing the native SDK, thereby enabling the native SDK functionality as intended.

馃挌 How did you test it?

I conducted testing of this change on my local application to ensure its effectiveness. The following steps were taken during the testing process:

  1. Set up a local development environment with the Sentry React Native SDK integrated into my application.
  2. Created a scenario where the closeNativeSdk() method is called to disable the native SDK and subsequently attempted to reinitialize the SDK.
  3. Verified that prior to the change, the enableNative variable was not reinitialized to true after reinitializing the SDK, leading to the native SDK functionality not being enabled.
  4. Applied the proposed change to the codebase, which includes resetting the enableNative variable to true in the initNativeSdk function.
  5. Repeated the scenario where the closeNativeSdk() method is called and then attempted to reinitialize the SDK.
  6. Verified that after applying the change, the enableNative variable was correctly reinitialized to true, enabling the native SDK functionality upon reinitialization.
  7. Conducted additional testing by calling the closeNativeSdk() method again after reinitializing the SDK.
  8. Ensured that the native SDK was successfully stopped and disabled as expected, preventing any further native SDK functionality.
  9. Ensured that the rest of the application's functionality remained unaffected by the change.
  10. By following these steps and observing the expected behavior, I confirmed that the change effectively resolves the issue with the enableNative variable and allows for subsequent reinitializations of the SDK while still being able to stop and disable the native SDK when desired.

馃摑 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled
  • All tests passing
  • No breaking changes

馃敭 Next steps

This commit addresses a problem with the Sentry React Native SDK where the this.enableNative variable was not correctly reinitialized to true after reinitializing the SDK. The issue arose when calling the closeNativeSdk() method, which set this.enableNative to false. Without reinitializing it to true, subsequent reinitializations of the SDK would fail to enable the native SDK functionality.

To fix this, I added a line in the initNativeSdk function to explicitly set this.enableNative = true after reinitializing the native SDK. This ensures that the native SDK is correctly enabled when reinitializing.

This change resolves the problem and ensures that the enableNative variable is properly managed during SDK reinitialization, allowing for the correct functioning of the Sentry React Native SDK.
@krystofwoldrich
Copy link
Member

krystofwoldrich commented May 30, 2023

Hi,
thank you for the PR and the detailed description.

Since this is mainly a matter of handling the enableNative flag, could you please update the tests in wrapper.test.ts. The tests are already checking the flag value but they are using the default value, so the issue you have described doesn't occur. If we set the flag to the opposite value than expected, we can be sure that the tested methods are setting the flag correctly.

Here

test('does not initialize with autoInitializeNativeSdk: false', async () => {
logger.warn = jest.fn();

    test('does not initialize with autoInitializeNativeSdk: false', async () => {
+      NATIVE.enableNative = false;
      logger.warn = jest.fn();

and

test('closeNativeSdk calls native bridge', async () => {
await NATIVE.closeNativeSdk();

    test('closeNativeSdk calls native bridge', async () => {
+      NATIVE.enableNative = true;
      await NATIVE.closeNativeSdk();

@mateioprea
Copy link
Contributor Author

@krystofwoldrich sure! Just did. Thank you!

@krystofwoldrich
Copy link
Member

@mateioprea It's looking good, you can ignore the e2e tests falling.

But the unit test failed too.

The flag will also need to be set here https://github.com/getsentry/sentry-react-native/pull/3093/files#diff-80149da618e89fad93a582eabad62eb682bd2774a2fdb209b84001813708f0f8R183 true to enabled native modules for manual initialization and here https://github.com/getsentry/sentry-react-native/pull/3093/files#diff-80149da618e89fad93a582eabad62eb682bd2774a2fdb209b84001813708f0f8R190 false when DSN is missing.

@mateioprea
Copy link
Contributor Author

@krystofwoldrich thank you! I just pushed a fix for that.

@mateioprea
Copy link
Contributor Author

@krystofwoldrich i don't understand why the e2e tests are failing now. everything should be good.
But I see that some tests don't even get to the relevant phase, like:
[RnDiffApp] Running script Upload Debug Symbols to Sentry
or
Error cloning certificates git repo, please make sure you have access to the repository - see instructions above

@krystofwoldrich
Copy link
Member

@mateioprea The e2e build tests fail because the auth tokens are missing for the community PR. The tests fail on uploading the source maps. But that is okay.

@mateioprea
Copy link
Contributor Author

@mateioprea The e2e build tests fail because the auth tokens are missing for the community PR. The tests fail on uploading the source maps. But that is okay.

ok. perfect. so is this something else that I should this on this PR?

@krystofwoldrich
Copy link
Member

@mateioprea The for the follow-up.

Yes, one of the unit tests in client.test.ts broke by the changes. This should fix it.

  describe('nativeCrash', () => {
    test('calls NativeModules crash', () => {
      const RN: MockedReactNative = require('react-native');

      const client = new ReactNativeClient({
        ...DEFAULT_OPTIONS,
+        dsn: EXAMPLE_DSN,

And as last, a changelog update is needed.

## Unreleased

+ ### Fixes
+ 
+ - Native wrapper methods don't throw disabled error after re-initializing [#3093](https://github.com/getsentry/sentry-react-native/pull/3093)
+ 
### Dependencies

@mateioprea
Copy link
Contributor Author

@krystofwoldrich thank you for guidance. I've updated the files accordingly.

Copy link
Member

@krystofwoldrich krystofwoldrich left a comment

Choose a reason for hiding this comment

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

Thank you for the PR and the work. 馃殌

@mateioprea
Copy link
Contributor Author

Thank you @krystofwoldrich. Have a great day!

@krystofwoldrich krystofwoldrich merged commit 22c9338 into getsentry:main Jun 13, 2023
24 of 37 checks passed
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.

None yet

2 participants