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

fix: SentryUIViewControllerSwizzlingTests #3089

Merged
merged 1 commit into from
Jun 7, 2023

Conversation

brustolin
Copy link
Contributor

Fixed SentryUIViewControllerSwizzlingTests because it broke with latest swift/xcode updates.

#skip-changelog

@codecov
Copy link

codecov bot commented Jun 7, 2023

Codecov Report

Merging #3089 (7027ddc) into main (29e1264) will increase coverage by 2.015%.
The diff coverage is 92.857%.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3089       +/-   ##
=============================================
+ Coverage   86.943%   88.958%   +2.015%     
=============================================
  Files          451       495       +44     
  Lines        47086     53464     +6378     
  Branches     17030     19139     +2109     
=============================================
+ Hits         40938     47561     +6623     
+ Misses        5287      4945      -342     
- Partials       861       958       +97     
Impacted Files Coverage Δ
Sources/Sentry/SentryNSProcessInfoWrapper.mm 66.666% <90.000%> (+26.666%) ⬆️
...troller/SentryUIViewControllerSwizzlingTests.swift 98.818% <100.000%> (ø)

... and 154 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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

@github-actions
Copy link

github-actions bot commented Jun 7, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1212.34 ms 1244.58 ms 32.24 ms
Size 20.76 KiB 393.37 KiB 372.61 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
3f366ee 1242.28 ms 1260.80 ms 18.52 ms
89d72e7 1222.60 ms 1252.78 ms 30.18 ms
455619d 1231.40 ms 1237.70 ms 6.30 ms
31ac438 1244.29 ms 1264.06 ms 19.78 ms
fb53d97 1235.00 ms 1241.88 ms 6.88 ms
83d2d84 1211.31 ms 1227.34 ms 16.03 ms
b2f82fa 1237.78 ms 1256.02 ms 18.24 ms
e8b2fb4 1230.70 ms 1242.84 ms 12.14 ms
4977fbc 1231.55 ms 1239.80 ms 8.25 ms
c0ff306 1218.92 ms 1240.64 ms 21.72 ms

App size

Revision Plain With Sentry Diff
3f366ee 20.76 KiB 427.84 KiB 407.08 KiB
89d72e7 20.76 KiB 431.87 KiB 411.11 KiB
455619d 20.76 KiB 432.87 KiB 412.11 KiB
31ac438 20.76 KiB 393.36 KiB 372.60 KiB
fb53d97 20.76 KiB 425.80 KiB 405.04 KiB
83d2d84 20.76 KiB 419.66 KiB 398.90 KiB
b2f82fa 20.76 KiB 419.62 KiB 398.86 KiB
e8b2fb4 20.76 KiB 427.23 KiB 406.46 KiB
4977fbc 20.76 KiB 419.86 KiB 399.10 KiB
c0ff306 20.76 KiB 434.65 KiB 413.89 KiB

Copy link
Member

@markushi markushi left a comment

Choose a reason for hiding this comment

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

LGTM!

@brustolin brustolin merged commit 60dd0f5 into main Jun 7, 2023
@brustolin brustolin deleted the fix/UIViewController-Swizzling-tests branch June 7, 2023 09:24
{
_executablePath = path;
}
# define EXECUTABLE_PATH _executablePath;
Copy link
Member

Choose a reason for hiding this comment

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

Please always prefix macro names with SENTRY_. While this instance won't break compilation, this name collides with a build setting, so it's easy to imagine it causing confusion later:
image

Comment on lines +21 to +23
}
# define EXECUTABLE_PATH NSBundle.mainBundle.executablePath;
#endif
Copy link
Member

Choose a reason for hiding this comment

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

We should just avoid breaking up syntactically complete constructs with #if/#else. We only recently had a discussion about how complex it makes things here: #2858 (comment)

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.

3 participants