test: remove length invocation testing from SentryInvalidJSONString#8453
test: remove length invocation testing from SentryInvalidJSONString#8453NinjaLikesCheez wants to merge 3 commits into
Conversation
NinjaLikesCheez
added
the
ready-to-merge
|
|
||
| func testSerializationFailsWithFirstValidAndThenInvalidJSONObject() { | ||
| let json = [ SentryInvalidJSONString(lengthInvocationsToBeInvalid: 1)] | ||
| let json = [ SentryInvalidJSONString()] |
There was a problem hiding this comment.
m: I think this quietly drops coverage of a crash path we care about. Once SentryInvalidJSONString() is invalid from the first length call, this test only hits the isValidJSONObject guard and never reaches the throw-during-serialization branch. That branch is the interesting one: the valid-then-invalid string models data that passes a shallow check and then blows up deep in the encoder — which is exactly the launch crash from #4280 that #4337 added this guard for.
There was a problem hiding this comment.
Ah, I'm not sure how I missed that - in this case I will revert the changes to this class and add the new *OS 27 stack trace to the filtering logic. Thank you for catching this!
Why: Apple changes the call stack every now and then and we're playing cat and mouse - couldn't find a use of the invocation checks in a test where it made sense to keep it. This brings us closer to *OS 27 testing parity (but not there - testFramesOrder still fails)
d9ab245 to
c14877b
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c14877b. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| @[ @"__CFStringEncodeByteStream", @"+[_NSJSONReader validForJSON:depth:allowFragments:]" ]; | ||
|
|
||
| NSString *callStackSymbol = NSThread.callStackSymbols[1]; | ||
|
|
There was a problem hiding this comment.
Bug: NSLog statements remain in SentryInvalidJSONString.m despite the commit message indicating they would be removed, causing unnecessary console output during tests.
Severity: MEDIUM
Suggested Fix
Remove the NSLog statements at lines 49 and 54 in Tests/SentryTests/TestUtils/SentryInvalidJSONString.m to align the code with the stated intent of the commit and to clean up test console output.
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: Tests/SentryTests/TestUtils/SentryInvalidJSONString.m#L49
Potential issue: The pull request intended to remove debug logging, as stated in the
commit message "test: remove debug logging". However, `NSLog` statements on lines 49 and
54 of `SentryInvalidJSONString.m` were not removed. These statements will execute
repeatedly during test runs that use `SentryInvalidJSONString` for JSON
encoding/decoding, creating excessive console noise and making test output more
difficult to debug.
Also affects:
Tests/SentryTests/TestUtils/SentryInvalidJSONString.m:54
Did we get this right? 👍 / 👎 to inform future reviews.
2 participants
📜 Description
💡 Motivation and Context
Why: Apple changes the call stack every now and then and we're playing cat and mouse - couldn't find a use of the invocation checks in a test where it made sense to keep it.
This brings us closer to *OS 27 testing parity (but not there -
testFramesOrder()still fails as there's a new async mechanism being used to start XCTest (I think))💚 How did you test it?
📝 Checklist
You have to check all boxes before merging:
#skip-changelog
Closes #8454