test: prevent crashing on nil or index out of bounds #2423
Conversation
armcknight
requested review from
philipphofmann and
brustolin
as code owners
There was a problem hiding this comment.
Thanks, @armcknight for that great refactoring 🙏😀. You could refactor this to use XCTUnwrap. But it's also fine not doing that. I changed the base to 8.0.0 as this is our default branch now until we make 8.0.0 GA to avoid merging back and forth from the master branch.
If you need this for #2228 and you want to do a hotfix for #2228 we could also merge it to both 8.0.0 and master, but I would like to avoid that if possible.
| guard let tracer = transactionSpan as? SentryTracer else { | ||
| XCTFail("Expected a tracer") | ||
| return | ||
| } |
There was a problem hiding this comment.
m: You could use XCTUnwrap for all of these instead. let tracer = try XCTUnwrap(transactionSpan). For more info see https://www.hackingwithswift.com/example-code/testing/how-to-check-and-unwrap-optionals-in-tests-using-xctunwrap.
There was a problem hiding this comment.
Nice, I've seen that used in the tests but forgot about it, and haven't used it myself before. I'll clean things up to use it instead!
There was a problem hiding this comment.
Yeah, that makes things much nicer. The only place I couldn't use them was within the view controller lifecycle callbacks, because they would need to be declared as throws in swift which would require i think an inout NSError in the ObjC interface? I was just wary of changing that SDK code solely for testing purposes.
| func testUILifeCycle_ViewDidAppear() { | ||
| assertUILifeCycle(finishStatus: SentrySpanStatus.ok) { sut, viewController, tracker, callbackExpectation, transactionSpan in | ||
| func testUILifeCycle_ViewDidAppear() throws { | ||
| try assertUILifeCycle(finishStatus: SentrySpanStatus.ok) { sut, viewController, tracker, callbackExpectation, transactionSpan in |
There was a problem hiding this comment.
assertUILifeCycle had some XCTUnwraps in it so had to be marked as throws, which bubbled up to a few other test cases.
| @@ -209,63 +203,57 @@ class SentryUIViewControllerPerformanceTrackerTests: XCTestCase { | |||
| self.advanceTime(bySeconds: 1) | |||
| callbackExpectation.fulfill() | |||
| } | |||
| assertSpanDuration(span: lastSpan, expectedDuration: 1) | |||
| try assertSpanDuration(span: lastSpan, expectedDuration: 1) | |||
There was a problem hiding this comment.
Same here, there are some XCTUnwraps in assertSpanDuration forcing it to be throws
While testing after making changes for #2228, the test suite would crash in various places due to changes in behavior changing e.g. the amount of things in an array. Change such logic to allow graceful degradation into simply failing the test case instead of crashing the test run.
#skip-changelog