Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: Remove Check for Original Method Call When Swizzling #1383

Merged
merged 10 commits into from
Oct 14, 2021

Conversation

brustolin
Copy link
Contributor

@brustolin brustolin commented Oct 13, 2021

Using try-catch in ObjC is not recommended, and our solution to check whether any swizzled method calls the original implementation uses one.

To not use this solution in release, a build configuration was created for tests, and we do this check only in tests.
Once the tests confirm that swizzled methods call the original implementation, we don't need to have this check on release.

Fixes GH-1368

@philipphofmann philipphofmann changed the title ref: Swizzling check only in tests fix: Remove Check for Original Method Call When Swizzling Oct 14, 2021
@codecov-commenter
Copy link

codecov-commenter commented Oct 14, 2021

Codecov Report

Merging #1383 (bcecc60) into master (0f2826b) will decrease coverage by 0.16%.
The diff coverage is 100.00%.

❗ Current head bcecc60 differs from pull request most recent head 10e9e46. Consider uploading reports for the commit 10e9e46 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1383      +/-   ##
==========================================
- Coverage   95.61%   95.44%   -0.17%     
==========================================
  Files         152      152              
  Lines        5515     6743    +1228     
==========================================
+ Hits         5273     6436    +1163     
- Misses        242      307      +65     
Impacted Files Coverage Δ
Sources/Sentry/NSArray+SentrySanitize.m 100.00% <ø> (ø)
Sources/Sentry/NSData+Sentry.m 83.33% <ø> (ø)
Sources/Sentry/NSData+SentryCompression.m 100.00% <ø> (ø)
Sources/Sentry/NSDate+SentryExtras.m 100.00% <ø> (ø)
Sources/Sentry/NSDictionary+SentrySanitize.m 100.00% <ø> (ø)
Sources/Sentry/UIViewController+Sentry.m 100.00% <ø> (ø)
Sources/Sentry/include/SentrySwizzle.h 97.22% <100.00%> (+47.22%) ⬆️
Sources/Sentry/SentryCrashReportSink.m 65.62% <0.00%> (-15.15%) ⬇️
Sources/Sentry/SentryTransactionContext.m 30.00% <0.00%> (-7.50%) ⬇️
Sources/Sentry/SentryRequestOperation.m 69.23% <0.00%> (-5.77%) ⬇️
... and 74 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 90ff84e...10e9e46. Read the comment docs.

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

LGTM, awesome you managed to get rid of the duplicated code.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SwizzlingError - Original method not called
4 participants