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

Author: stmontgomery

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
             }
 

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 <rdar://problem/49123061>
@@ -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

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