Skip to content

Conversation

noahsmartin
Copy link
Contributor

Fixes #5921

#skip-changelog

cursor[bot]

This comment was marked as outdated.

Copy link

codecov bot commented Aug 13, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@af7a86c). Learn more about missing BASE report.
⚠️ Report is 23 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##             main     #5943   +/-   ##
========================================
  Coverage        ?   86.749%           
========================================
  Files           ?       424           
  Lines           ?     36724           
  Branches        ?     17367           
========================================
  Hits            ?     31858           
  Misses          ?      4820           
  Partials        ?        46           
Files with missing lines Coverage Δ
Sources/Sentry/SentryCrashWrapper.m 91.129% <ø> (ø)
Sources/Sentry/SentryDependencyContainer.m 88.425% <100.000%> (ø)
Sources/Sentry/SentryExtraContextProvider.m 84.615% <ø> (ø)
Sources/Sentry/SentrySDKInternal.m 86.885% <ø> (ø)
...rces/Swift/Core/Helper/SentryUIDeviceWrapper.swift 100.000% <100.000%> (ø)

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 af7a86c...8333e1d. Read the comment docs.

Copy link

Seer failed to run.

@noahsmartin noahsmartin force-pushed the sentryUIDeviceWrapperSwift branch from 01e7f08 to 94f1e5d Compare August 14, 2025 01:14
Copy link
Contributor

github-actions bot commented Aug 14, 2025

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1230.41 ms 1256.12 ms 25.71 ms
Size 23.75 KiB 928.88 KiB 905.13 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
1fe932f 1231.92 ms 1253.44 ms 21.52 ms
073562b 1232.63 ms 1259.88 ms 27.24 ms
cda95fc 1231.42 ms 1247.18 ms 15.77 ms
8fd192f 1202.10 ms 1220.19 ms 18.09 ms
51b7dd3 1235.06 ms 1258.21 ms 23.15 ms
ebc3a34 1236.24 ms 1261.02 ms 24.78 ms
fd5961e 1210.59 ms 1235.57 ms 24.98 ms
42a95d5 1206.00 ms 1224.26 ms 18.26 ms
ae7be93 1236.24 ms 1258.18 ms 21.94 ms
884b224 1221.11 ms 1255.88 ms 34.77 ms

App size

Revision Plain With Sentry Diff
1fe932f 23.75 KiB 913.63 KiB 889.88 KiB
073562b 23.75 KiB 927.06 KiB 903.31 KiB
cda95fc 23.75 KiB 912.77 KiB 889.02 KiB
8fd192f 23.74 KiB 872.75 KiB 849.01 KiB
51b7dd3 23.75 KiB 913.26 KiB 889.52 KiB
ebc3a34 23.75 KiB 919.91 KiB 896.16 KiB
fd5961e 23.74 KiB 874.07 KiB 850.32 KiB
42a95d5 23.75 KiB 906.08 KiB 882.33 KiB
ae7be93 23.75 KiB 879.24 KiB 855.49 KiB
884b224 23.75 KiB 879.55 KiB 855.80 KiB

Previous results on branch: sentryUIDeviceWrapperSwift

Startup times

Revision Plain With Sentry Diff
4da8e49 1209.78 ms 1234.24 ms 24.47 ms
3da0264 1195.74 ms 1215.08 ms 19.34 ms
7dd2b6f 1209.50 ms 1228.82 ms 19.32 ms
44b1293 1208.92 ms 1263.34 ms 54.42 ms

App size

Revision Plain With Sentry Diff
4da8e49 23.75 KiB 921.01 KiB 897.26 KiB
3da0264 23.75 KiB 922.10 KiB 898.35 KiB
7dd2b6f 23.75 KiB 920.80 KiB 897.05 KiB
44b1293 23.75 KiB 922.10 KiB 898.35 KiB

@noahsmartin noahsmartin force-pushed the sentryUIDeviceWrapperSwift branch from 94f1e5d to 0b0a346 Compare August 14, 2025 04:58
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 would really prefer adding some tests when changing this. So basically some tests validating the old functionality. They need to be green and stay green once the class is converted to Swift. I know it's just a wrapper class, but it's so easy to introduce major bugs with such refactorings.

@noahsmartin
Copy link
Contributor Author

Updated and added 2 new tests for this

@noahsmartin noahsmartin force-pushed the sentryUIDeviceWrapperSwift branch 2 times, most recently from 277fd08 to 60860ee Compare August 15, 2025 13:32
Copy link
Member

@philprime philprime left a comment

Choose a reason for hiding this comment

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

LGTM, but it might be worth to add the additional two test cases to get the code coverage of SentryUIDeviceWrapper.swift to 100%

@noahsmartin noahsmartin force-pushed the sentryUIDeviceWrapperSwift branch from 60860ee to 8333e1d Compare August 28, 2025 03:30
@noahsmartin noahsmartin merged commit 83d27f6 into main Aug 28, 2025
165 of 169 checks passed
@noahsmartin noahsmartin deleted the sentryUIDeviceWrapperSwift branch August 28, 2025 15:15
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.

Convert SentryUIDeviceWrapper.h to Swift
3 participants