From dcaa306853d735afe6563355742df9d202ca445f Mon Sep 17 00:00:00 2001 From: Stuart Montgomery Date: Tue, 24 Nov 2020 11:14:58 -0600 Subject: [PATCH] Over-fulfilling XCTestExpectation records test failure even when `assertForOverFulfill` 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 --- .../Asynchronous/XCTestExpectation.swift | 19 +++----- .../Asynchronous/Expectations/main.swift | 43 +++++++++++++++++-- .../Functional/Asynchronous/Misuse/main.swift | 28 ++---------- 3 files changed, 49 insertions(+), 41 deletions(-) diff --git a/Sources/XCTest/Public/Asynchronous/XCTestExpectation.swift b/Sources/XCTest/Public/Asynchronous/XCTestExpectation.swift index afe435617..3d355fa84 100644 --- a/Sources/XCTest/Public/Asynchronous/XCTestExpectation.swift +++ b/Sources/XCTest/Public/Asynchronous/XCTestExpectation.swift @@ -259,19 +259,12 @@ open class XCTestExpectation { // expectations have completed. Similarly, this should cause an // error as well. - if queue_isFulfilled { - // FIXME: No regression tests exist for this feature. We may break it - // without ever realizing (similar to `continueAfterFailure`). - if _assertForOverFulfill { - fatalError("API violation - multiple calls made to fulfill() for \(queue_expectationDescription)") - } - - if let testCase = XCTCurrentTestCase { - testCase.recordFailure( - description: "API violation - multiple calls made to XCTestExpectation.fulfill() for \(queue_expectationDescription).", - at: sourceLocation, - expected: false) - } + if queue_isFulfilled, _assertForOverFulfill, let testCase = XCTCurrentTestCase { + testCase.recordFailure( + description: "API violation - multiple calls made to XCTestExpectation.fulfill() for \(queue_expectationDescription).", + at: sourceLocation, + expected: false) + return nil } diff --git a/Tests/Functional/Asynchronous/Expectations/main.swift b/Tests/Functional/Asynchronous/Expectations/main.swift index bbf274243..521a4c9d3 100644 --- a/Tests/Functional/Asynchronous/Expectations/main.swift +++ b/Tests/Functional/Asynchronous/Expectations/main.swift @@ -338,6 +338,39 @@ class ExpectationsTestCase: XCTestCase { waitForExpectations(timeout: 0.2) } + // PRAGMA MARK: - assertForOverFulfill + +// CHECK: Test Case 'ExpectationsTestCase.test_assertForOverfulfill_disabled' started at \d+-\d+-\d+ \d+:\d+:\d+\.\d+ +// CHECK: Test Case 'ExpectationsTestCase.test_assertForOverfulfill_disabled' passed \(\d+\.\d+ seconds\) + func test_assertForOverfulfill_disabled() { + let foo = XCTestExpectation(description: "foo") + XCTAssertFalse(foo.assertForOverFulfill, "assertForOverFulfill should be disabled by default") + foo.fulfill() + foo.fulfill() + } + +// CHECK: Test Case 'ExpectationsTestCase.test_assertForOverfulfill_failure' started at \d+-\d+-\d+ \d+:\d+:\d+\.\d+ +// CHECK: .*[/\\]Tests[/\\]Functional[/\\]Asynchronous[/\\]Expectations[/\\]main.swift:[[@LINE+7]]: error: ExpectationsTestCase.test_assertForOverfulfill_failure : API violation - multiple calls made to XCTestExpectation.fulfill\(\) for rob. +// CHECK: .*[/\\]Tests[/\\]Functional[/\\]Asynchronous[/\\]Expectations[/\\]main.swift:[[@LINE+16]]: error: ExpectationsTestCase.test_assertForOverfulfill_failure : API violation - multiple calls made to XCTestExpectation.fulfill\(\) for rob. +// CHECK: Test Case 'ExpectationsTestCase.test_assertForOverfulfill_failure' failed \(\d+\.\d+ seconds\) + func test_assertForOverfulfill_failure() { + let expectation = self.expectation(description: "rob") + expectation.assertForOverFulfill = true + expectation.fulfill() + expectation.fulfill() + // FIXME: The behavior here is subtly different from Objective-C XCTest. + // Objective-C XCTest would stop executing the test on the line + // above, and so would not report a failure for this line below. + // In total, it would highlight one line as a failure in this + // test. + // + // swift-corelibs-xctest continues to execute the test, and so + // highlights both the lines above and below as failures. + // This should be fixed such that the behavior is identical. + expectation.fulfill() + self.waitForExpectations(timeout: 0.1) + } + // PRAGMA MARK: - Interrupted Waiters // Disabled due to non-deterministic ordering of XCTWaiterDelegate callbacks, see [SR-10034] and @@ -497,6 +530,10 @@ class ExpectationsTestCase: XCTestCase { ("test_countedConditionPassBeforeWaiting", test_countedConditionPassBeforeWaiting), ("test_countedConditionFail", test_countedConditionFail), + // assertForOverFulfill + ("test_assertForOverfulfill_disabled", test_assertForOverfulfill_disabled), + ("test_assertForOverfulfill_failure", test_assertForOverfulfill_failure), + // Interrupted Waiters // ("test_outerWaiterTimesOut_InnerWaitersAreInterrupted", test_outerWaiterTimesOut_InnerWaitersAreInterrupted), ("test_outerWaiterCompletes_InnerWaiterTimesOut", test_outerWaiterCompletes_InnerWaiterTimesOut), @@ -513,11 +550,11 @@ class ExpectationsTestCase: XCTestCase { }() } // CHECK: Test Suite 'ExpectationsTestCase' failed at \d+-\d+-\d+ \d+:\d+:\d+\.\d+ -// CHECK: \t Executed 30 tests, with 14 failures \(0 unexpected\) in \d+\.\d+ \(\d+\.\d+\) seconds +// CHECK: \t Executed 32 tests, with 16 failures \(2 unexpected\) in \d+\.\d+ \(\d+\.\d+\) seconds XCTMain([testCase(ExpectationsTestCase.allTests)]) // CHECK: Test Suite '.*\.xctest' failed at \d+-\d+-\d+ \d+:\d+:\d+\.\d+ -// CHECK: \t Executed 30 tests, with 14 failures \(0 unexpected\) in \d+\.\d+ \(\d+\.\d+\) seconds +// CHECK: \t Executed 32 tests, with 16 failures \(2 unexpected\) in \d+\.\d+ \(\d+\.\d+\) seconds // CHECK: Test Suite 'All tests' failed at \d+-\d+-\d+ \d+:\d+:\d+\.\d+ -// CHECK: \t Executed 30 tests, with 14 failures \(0 unexpected\) in \d+\.\d+ \(\d+\.\d+\) seconds +// CHECK: \t Executed 32 tests, with 16 failures \(2 unexpected\) in \d+\.\d+ \(\d+\.\d+\) seconds diff --git a/Tests/Functional/Asynchronous/Misuse/main.swift b/Tests/Functional/Asynchronous/Misuse/main.swift index fb482d57c..6e978a0a4 100644 --- a/Tests/Functional/Asynchronous/Misuse/main.swift +++ b/Tests/Functional/Asynchronous/Misuse/main.swift @@ -28,41 +28,19 @@ class MisuseTestCase: XCTestCase { self.waitForExpectations(timeout: 0.1) } -// CHECK: Test Case 'MisuseTestCase.test_whenExpectationIsFulfilledMultipleTimes_fails' started at \d+-\d+-\d+ \d+:\d+:\d+\.\d+ -// CHECK: .*[/\\]Tests[/\\]Functional[/\\]Asynchronous[/\\]Misuse[/\\]main.swift:[[@LINE+6]]: error: MisuseTestCase.test_whenExpectationIsFulfilledMultipleTimes_fails : API violation - multiple calls made to XCTestExpectation.fulfill\(\) for rob. -// CHECK: .*[/\\]Tests[/\\]Functional[/\\]Asynchronous[/\\]Misuse[/\\]main.swift:[[@LINE+15]]: error: MisuseTestCase.test_whenExpectationIsFulfilledMultipleTimes_fails : API violation - multiple calls made to XCTestExpectation.fulfill\(\) for rob. -// CHECK: Test Case 'MisuseTestCase.test_whenExpectationIsFulfilledMultipleTimes_fails' failed \(\d+\.\d+ seconds\) - func test_whenExpectationIsFulfilledMultipleTimes_fails() { - let expectation = self.expectation(description: "rob") - expectation.fulfill() - expectation.fulfill() - // FIXME: The behavior here is subtly different from Objective-C XCTest. - // Objective-C XCTest would stop executing the test on the line - // above, and so would not report a failure for this line below. - // In total, it would highlight one line as a failure in this - // test. - // - // swift-corelibs-xctest continues to execute the test, and so - // highlights both the lines above and below as failures. - // This should be fixed such that the behavior is identical. - expectation.fulfill() - self.waitForExpectations(timeout: 0.1) - } - static var allTests = { return [ ("test_whenExpectationsAreMade_butNotWaitedFor_fails", test_whenExpectationsAreMade_butNotWaitedFor_fails), ("test_whenNoExpectationsAreMade_butTheyAreWaitedFor_fails", test_whenNoExpectationsAreMade_butTheyAreWaitedFor_fails), - ("test_whenExpectationIsFulfilledMultipleTimes_fails", test_whenExpectationIsFulfilledMultipleTimes_fails), ] }() } // CHECK: Test Suite 'MisuseTestCase' failed at \d+-\d+-\d+ \d+:\d+:\d+\.\d+ -// CHECK: \t Executed 3 tests, with 4 failures \(4 unexpected\) in \d+\.\d+ \(\d+\.\d+\) seconds +// CHECK: \t Executed 2 tests, with 2 failures \(2 unexpected\) in \d+\.\d+ \(\d+\.\d+\) seconds XCTMain([testCase(MisuseTestCase.allTests)]) // CHECK: Test Suite '.*\.xctest' failed at \d+-\d+-\d+ \d+:\d+:\d+\.\d+ -// CHECK: \t Executed 3 tests, with 4 failures \(4 unexpected\) in \d+\.\d+ \(\d+\.\d+\) seconds +// CHECK: \t Executed 2 tests, with 2 failures \(2 unexpected\) in \d+\.\d+ \(\d+\.\d+\) seconds // CHECK: Test Suite 'All tests' failed at \d+-\d+-\d+ \d+:\d+:\d+\.\d+ -// CHECK: \t Executed 3 tests, with 4 failures \(4 unexpected\) in \d+\.\d+ \(\d+\.\d+\) seconds +// CHECK: \t Executed 2 tests, with 2 failures \(2 unexpected\) in \d+\.\d+ \(\d+\.\d+\) seconds