-
-
Notifications
You must be signed in to change notification settings - Fork 371
fix: Nullability in SentrySwizzleWrapperHelper #6970
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
base: main
Are you sure you want to change the base?
Conversation
bca2c2a to
b2eb06e
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6970 +/- ##
=============================================
- Coverage 85.076% 84.492% -0.584%
=============================================
Files 453 453
Lines 27674 27664 -10
Branches 12166 12162 -4
=============================================
- Hits 23544 23374 -170
- Misses 4087 4240 +153
- Partials 43 50 +7
... and 50 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 32f2329 | 1224.36 ms | 1257.33 ms | 32.98 ms |
| df94923 | 1217.35 ms | 1247.44 ms | 30.09 ms |
| e3ebff3 | 1223.47 ms | 1249.27 ms | 25.80 ms |
| dba2de8 | 1231.45 ms | 1251.88 ms | 20.43 ms |
| ee1f2ca | 1227.47 ms | 1251.58 ms | 24.12 ms |
| fc6557e | 1226.40 ms | 1249.88 ms | 23.48 ms |
| 7cc23cf | 1203.15 ms | 1232.11 ms | 28.96 ms |
| cda95fc | 1231.42 ms | 1247.18 ms | 15.77 ms |
| 535ebd9 | 1194.59 ms | 1219.84 ms | 25.26 ms |
| 26f7b17 | 1218.47 ms | 1253.82 ms | 35.35 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 32f2329 | 23.75 KiB | 1.01 MiB | 1016.02 KiB |
| df94923 | 24.15 KiB | 1.01 MiB | 1014.90 KiB |
| e3ebff3 | 23.75 KiB | 878.48 KiB | 854.73 KiB |
| dba2de8 | 23.75 KiB | 969.25 KiB | 945.50 KiB |
| ee1f2ca | 24.15 KiB | 1.01 MiB | 1015.24 KiB |
| fc6557e | 23.75 KiB | 866.68 KiB | 842.93 KiB |
| 7cc23cf | 23.75 KiB | 913.62 KiB | 889.87 KiB |
| cda95fc | 23.75 KiB | 912.77 KiB | 889.02 KiB |
| 535ebd9 | 23.75 KiB | 1008.67 KiB | 984.92 KiB |
| 26f7b17 | 23.75 KiB | 960.93 KiB | 937.19 KiB |
Previous results on branch: fixSwizzlingNullability
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 3669afe | 1197.04 ms | 1233.71 ms | 36.67 ms |
| ced24ac | 1209.42 ms | 1241.28 ms | 31.86 ms |
| 05e7507 | 1219.73 ms | 1252.80 ms | 33.07 ms |
| 20fbddf | 1214.44 ms | 1255.20 ms | 40.76 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 3669afe | 24.14 KiB | 1.02 MiB | 1016.92 KiB |
| ced24ac | 24.14 KiB | 1.02 MiB | 1016.91 KiB |
| 05e7507 | 24.14 KiB | 1.02 MiB | 1016.91 KiB |
| 20fbddf | 24.14 KiB | 1.02 MiB | 1016.90 KiB |
philipphofmann
left a comment
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.
LGTM, @noahsmartin, I think it would be great to add a test preventing this crash from happening again.
dba1012 to
08617b6
Compare
| Button(action: captureTransactionAction) { | ||
| Text("Capture Transaction") | ||
| } | ||
| Button("UIApplication sendEvent") { |
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.
l:
| Button("UIApplication sendEvent") { | |
| Button("UIApplication sendEmptyEvent") { |
| Text("Capture Transaction") | ||
| } | ||
| Button("UIApplication sendEvent") { | ||
| UIApplication.shared.sendAction(#selector(UIResponder.resignFirstResponder), to: nil, from: nil, for: nil) |
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.
m: I think it makes sense to add a comment that this is required for a UITest, because otherwise it could be confusing what the purpose of this.
|
|
||
| final class UIApplicationSwizzleTests: XCTestCase { | ||
|
|
||
| func testSendEvent() { |
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.
m: I'm unsure if we actually run these UI tests in CI. Worth double checking.
|
|
||
| final class UIApplicationSwizzleTests: XCTestCase { | ||
|
|
||
| func testSendEvent() { |
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.
m: I think it's worth adding a comment to explain the purpose of this test. Basically it's a regression test for a crash we had and maybe we can add the link to the issue.
08617b6 to
9526bb3
Compare
| app.safelyLaunch() | ||
|
|
||
| app.buttons["UIApplication sendEmptyEvent"].tap() | ||
| fatalError() |
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: Unconditional fatalError() in testSendEvent prevents test from validating fix.
Severity: HIGH | Confidence: High
🔍 Detailed Analysis
The testSendEvent method in UIApplicationSwizzleTests.swift contains an unconditional fatalError() at line 15. This causes the test to crash every time it is executed, preventing it from verifying the intended fix for issue #6966. The test lacks any assertions to confirm the app's behavior after tapping the button, making it non-functional and unable to validate the fix.
💡 Suggested Fix
Remove the fatalError() and add an assertion (e.g., XCTAssertNoThrow or XCTAssertTrue that the app is still running) after the button tap to verify the app does not crash.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: Samples/iOS-SwiftUI/iOS-SwiftUI-UITests/UIApplicationSwizzleTests.swift#L15
Potential issue: The `testSendEvent` method in `UIApplicationSwizzleTests.swift`
contains an unconditional `fatalError()` at line 15. This causes the test to crash every
time it is executed, preventing it from verifying the intended fix for issue #6966. The
test lacks any assertions to confirm the app's behavior after tapping the button, making
it non-functional and unable to validate the fix.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 5493352
| app.safelyLaunch() | ||
|
|
||
| app.buttons["UIApplication sendEmptyEvent"].tap() | ||
| fatalError() |
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: Test contains fatalError() that will always crash
The test testSendEvent contains a fatalError() call at the end that will unconditionally crash when the test runs. This appears to be accidentally committed debugging code. The test is intended to verify that the app doesn't crash when sendAction:to:from:forEvent: is called with nil parameters, but the fatalError() will cause the test to fail regardless of whether the actual fix works.
fixes: #6966