[SR-12575] Over-fulfilling XCTestExpectation records test failure even when assertForOverFulfill is disabled
#320
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ertForOverFulfill` is disabled This fixes an issue where calling `XCTestExpectation.fulfill()` more times than expected will still record a test failure, even if its `assertForOverFulfill` property is set to `false` (the default). In this Corelibs version of XCTest, `assertForOverFulfill` is only guarding a `fatalError`, but we still unconditionally record a test failure which causes this symptom. This means the behavior is different than in the Xcode copy of XCTest, where if `assertForOverFulfill` is false, neither an NSAssert exception nor a test failure occurs. Swift doesn't support exceptions, so I believe the unconditional test failure was added here in the past in an attempt to emulate that behavior. The fix here removes the `fatalError()` entirely, and leaves the test failure-recording logic but makes it conditional on `assertForOverFulfill` being true. The behavior will still differ from Xcode's copy of XCTest, but I think it's justifiable and closer to what users expect. The use of NSAssert in the (ObjC) Xcode copy of XCTest was largely an attempt to get better diagnostics when an over-fulfill occurred by capturing the backtrace of that fulfillment. This is especially important with XCTestExpectations, since they are meant for testing async conditions, and an over-fulfillment could occur after their test has completed, so without a backtrace or some better state tracking the test failure could be misattributed to a subsequent test once it becomes "current". Corelibs XCTest doesn't suffer from this diagnostics problem as much since it already captures `file:line:` in `fulfill()`. Moreover, there have been requests to change that behavior in ObjC XCTest and use a "plain" test failure everywhere instead of NSAssert. So this PR gets us closer to that goal, starting here in Corelibs, by removing the fatalError and only leaving the existing test failure-recording behavior. To fully realize it and ensure these test failures are attributed to the correct test, we'll need to make further internal state tracking enhancements (63874139), but for now this seems like a good incremental step, considering that Corelibs already performed this test failure recording. I've also added a new test and modified an existing one. rdar://problem/62202297
|
@swift-ci please test |
stmontgomery
changed the title
assertForOverFulfill is disabledassertForOverFulfill is disabled
briancroom
approved these changes
Dec 2, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes an issue where calling
XCTestExpectation.fulfill()more times than expected will stillrecord a test failure, even if its
assertForOverFulfillproperty is set tofalse(the default).In this Corelibs version of XCTest,
assertForOverFulfillis only guarding afatalError, but westill unconditionally record a test failure which causes this symptom. This means the behavior is
different than in the Xcode copy of XCTest, where if
assertForOverFulfillis false, neither anNSAssert exception nor a test failure occurs. Swift doesn't support exceptions, so I believe the
unconditional test failure was added here in the past in an attempt to emulate that behavior.
The fix here removes the
fatalError()entirely, and leaves the test failure-recording logic butmakes it conditional on
assertForOverFulfillbeing true. The behavior will still differ from Xcode'scopy of XCTest, but I think it's justifiable and closer to what users expect. The use of NSAssert in
the (ObjC) Xcode copy of XCTest was largely an attempt to get better diagnostics when an over-fulfill
occurred by capturing the backtrace of that fulfillment. This is especially important with
XCTestExpectations, since they are meant for testing async conditions, and an over-fulfillment could
occur after their test has completed, so without a backtrace or some better state tracking the test
failure could be misattributed to a subsequent test once it becomes "current". Corelibs XCTest doesn't
suffer from this diagnostics problem as much since it already captures
file:line:infulfill().Moreover, there have been requests to change that behavior in ObjC XCTest and use a "plain" test failure
everywhere instead of NSAssert.
So this PR gets us closer to that goal, starting here in Corelibs, by removing the fatalError and only
leaving the existing test failure-recording behavior. To fully realize it and ensure these test
failures are attributed to the correct test, we'll need to make further internal state tracking
enhancements (63874139), but for now this seems like a good incremental step, considering that Corelibs
already performed this test failure recording. I've also added a new test and modified an existing one.
rdar://problem/62202297