Skip to content

Conversation

itaybre
Copy link
Contributor

@itaybre itaybre commented Sep 2, 2025

#skip-changelog

Copy link

linear bot commented Sep 2, 2025

Copy link

codecov bot commented Sep 2, 2025

Codecov Report

❌ Patch coverage is 94.75983% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.639%. Comparing base (8203ac0) to head (889f5e7).

Files with missing lines Patch % Lines
Sources/Swift/SentryCrash/SentryCrashWrapper.swift 94.444% 12 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #6047       +/-   ##
=============================================
+ Coverage   86.602%   86.639%   +0.036%     
=============================================
  Files          432       432               
  Lines        36956     37049       +93     
  Branches     17426     17440       +14     
=============================================
+ Hits         32005     32099       +94     
+ Misses        4907      4905        -2     
- Partials        44        45        +1     
Files with missing lines Coverage Δ
Sources/Sentry/SentryANRTrackerV1.m 99.212% <ø> (ø)
Sources/Sentry/SentryANRTrackerV2.m 98.863% <ø> (ø)
Sources/Sentry/SentryANRTrackingIntegration.m 99.404% <ø> (+0.595%) ⬆️
Sources/Sentry/SentryAppStartTrackingIntegration.m 100.000% <ø> (ø)
Sources/Sentry/SentryAppStateManager.m 100.000% <ø> (ø)
Sources/Sentry/SentryBaseIntegration.m 95.512% <ø> (ø)
Sources/Sentry/SentryCrashIntegration.m 99.200% <100.000%> (ø)
...rces/Sentry/SentryCrashIntegrationSessionHandler.m 100.000% <ø> (ø)
Sources/Sentry/SentryCrashReportSink.m 90.909% <ø> (ø)
Sources/Sentry/SentryDependencyContainer.m 89.361% <100.000%> (-0.046%) ⬇️
... and 10 more

... and 8 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 8203ac0...889f5e7. Read the comment docs.

Copy link
Contributor

github-actions bot commented Sep 2, 2025

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1217.06 ms 1242.94 ms 25.88 ms
Size 23.75 KiB 962.91 KiB 939.15 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
2a07609 1207.79 ms 1233.77 ms 25.98 ms
2481950 1221.04 ms 1248.98 ms 27.94 ms
3ffd0e5 1228.04 ms 1253.04 ms 25.00 ms
62f8d2a 1226.24 ms 1249.92 ms 23.67 ms
8f2120f 1235.92 ms 1249.67 ms 13.75 ms
f97a070 1218.88 ms 1253.12 ms 34.24 ms
9389467 1218.62 ms 1244.86 ms 26.24 ms
e18d392 1228.69 ms 1244.43 ms 15.73 ms
db9572a 1212.61 ms 1237.73 ms 25.13 ms
52f3b6e 1219.57 ms 1239.70 ms 20.13 ms

App size

Revision Plain With Sentry Diff
2a07609 23.75 KiB 912.78 KiB 889.03 KiB
2481950 23.74 KiB 872.74 KiB 849.00 KiB
3ffd0e5 23.75 KiB 947.55 KiB 923.80 KiB
62f8d2a 23.75 KiB 926.80 KiB 903.05 KiB
8f2120f 23.75 KiB 941.78 KiB 918.03 KiB
f97a070 23.75 KiB 858.68 KiB 834.93 KiB
9389467 23.75 KiB 866.51 KiB 842.76 KiB
e18d392 23.75 KiB 866.68 KiB 842.93 KiB
db9572a 23.75 KiB 858.65 KiB 834.90 KiB
52f3b6e 23.75 KiB 920.54 KiB 896.79 KiB

Previous results on branch: itay/cocoa-551-convert-sentrycrashwrapper-to-swift

Startup times

Revision Plain With Sentry Diff
d7566e2 1205.94 ms 1236.04 ms 30.10 ms
4290f04 1217.52 ms 1236.62 ms 19.10 ms
df16a98 1208.71 ms 1219.46 ms 10.75 ms

App size

Revision Plain With Sentry Diff
d7566e2 23.75 KiB 941.63 KiB 917.88 KiB
4290f04 23.75 KiB 936.73 KiB 912.98 KiB
df16a98 23.75 KiB 941.67 KiB 917.92 KiB

@itaybre itaybre marked this pull request as ready for review September 3, 2025 18:35

private func setScreenDimensions(_ deviceData: inout [String: Any]) {
// The UIWindowScene is unavailable on visionOS
#if (os(iOS) || os(tvOS) || (swift(>=5.9) && os(visionOS))) && !SENTRY_NO_UIKIT && !(swift(>=5.9) && os(visionOS))
Copy link
Contributor

Choose a reason for hiding this comment

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

You did the vision OS check twice here, I think you can remove them both and just say iOS || tvOS

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.

I'm a bit afraid that we introduce new bugs with this refactoring cause the test coverage for the SentryCrashWrapper is quite limited.

return 0
}

@objc public func enrichScope(_ scope: Scope) {
Copy link
Member

Choose a reason for hiding this comment

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

h: If I'm not mistaken, the test coverage for all the scope-enriching logic is very limited. I don't feel comfortable changing this without adding tests first for the existing implementation and then using these tests to avoid regressions. Or how can we be sure we don't introduce any new bugs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is an integration test here: SentryCrashIntegrationTests
Not the same, but I will add individual UT

Copy link
Member

Choose a reason for hiding this comment

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

SentryCrashIntegrationTests covers some parts of it yes, but I think not all.

cursor[bot]

This comment was marked as outdated.

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.

4 participants