-
-
Notifications
You must be signed in to change notification settings - Fork 372
Make SentrySDKSettings only visible to Swift #6927
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Missing equality implementation for SentrySdkInfo breaks tests
The SentrySdkInfo+Equality.m file that provided custom isEqual: and hash implementations for SentrySdkInfo was removed, but tests still rely on object equality comparisons via XCTAssertEqual. Since SentrySdkInfo inherits from NSObject, it will now use default reference equality instead of value equality. Unlike SentrySDKSettings which received a Swift Equatable extension, no replacement was added for SentrySdkInfo. This will cause all test assertions comparing SentrySdkInfo instances to fail, as two objects with identical content will not be considered equal.
Tests/SentryTests/Protocol/SentrySdkInfoTests.swift#L510-L519
sentry-cocoa/Tests/SentryTests/Protocol/SentrySdkInfoTests.swift
Lines 510 to 519 in a23a43d
| private func assertEmptySdkInfo(actual: SentrySdkInfo) { | |
| XCTAssertEqual(SentrySdkInfo( | |
| name: "", | |
| version: "", | |
| integrations: [], | |
| features: [], | |
| packages: [], | |
| settings: SentrySDKSettings(dict: [:]) | |
| ), actual) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6927 +/- ##
=============================================
+ Coverage 85.029% 86.044% +1.014%
=============================================
Files 453 453
Lines 27635 37613 +9978
Branches 12144 17468 +5324
=============================================
+ Hits 23498 32364 +8866
- Misses 4093 5202 +1109
- Partials 44 47 +3
... and 411 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 0ee162c | 1226.90 ms | 1261.72 ms | 34.83 ms |
| 7f26f16 | 1220.62 ms | 1255.04 ms | 34.42 ms |
| 5290541 | 1216.38 ms | 1249.56 ms | 33.18 ms |
| 319fb1e | 1219.48 ms | 1242.69 ms | 23.21 ms |
| 0f410ad | 1193.34 ms | 1255.49 ms | 62.15 ms |
| b6f8eb3 | 1217.94 ms | 1246.57 ms | 28.63 ms |
| e8f9a1d | 1229.02 ms | 1264.17 ms | 35.15 ms |
| 37238de | 1236.00 ms | 1267.80 ms | 31.80 ms |
| 0ecea77 | 1191.47 ms | 1216.12 ms | 24.65 ms |
| 1655116 | 1220.72 ms | 1260.58 ms | 39.85 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 0ee162c | 23.75 KiB | 933.33 KiB | 909.58 KiB |
| 7f26f16 | 23.75 KiB | 1.02 MiB | 1016.66 KiB |
| 5290541 | 24.14 KiB | 1.01 MiB | 1015.38 KiB |
| 319fb1e | 23.75 KiB | 1019.18 KiB | 995.43 KiB |
| 0f410ad | 24.14 KiB | 1.01 MiB | 1014.82 KiB |
| b6f8eb3 | 23.75 KiB | 988.70 KiB | 964.95 KiB |
| e8f9a1d | 23.75 KiB | 969.78 KiB | 946.04 KiB |
| 37238de | 23.75 KiB | 963.18 KiB | 939.43 KiB |
| 0ecea77 | 24.15 KiB | 1.01 MiB | 1014.90 KiB |
| 1655116 | 23.75 KiB | 1023.84 KiB | 1000.10 KiB |
Previous results on branch: sdkSettingsSwiftOnly
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| b14f987 | 1221.27 ms | 1249.65 ms | 28.38 ms |
| 0a23836 | 1223.60 ms | 1249.43 ms | 25.82 ms |
| 4a6894e | 1227.54 ms | 1266.37 ms | 38.83 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| b14f987 | 24.14 KiB | 1.01 MiB | 1013.08 KiB |
| 0a23836 | 24.14 KiB | 1.01 MiB | 1013.08 KiB |
| 4a6894e | 24.14 KiB | 1.01 MiB | 1013.68 KiB |
a23a43d to
c39a858
Compare
49ff5d5 to
6232a4a
Compare
6232a4a to
8a325b0
Compare
I noticed the objc compatibility here was only used from tests, so I cleaned up the tests and removed the
@objcfrom SentrySDKSettings.#skip-changelog
Closes #6928