Use XCTestRun to execute tests and report results #86

modocache · 2016-03-28T06:39:52Z

What's in this pull request?

A major goal for swift-corelibs-xctest is API parity with Apple XCTest. This adds the largest missing API in swift-corelibs-xctest: XCTestRun.

In Apple XCTest, XCTestRun is responsible for keeping track of the result of a test run. It's an integral part of how Apple XCTest works. swift-corelibs-xctest, on the other hand, used a global array of XCTRun structs to keep track of how many tests passed/failed.

While it may have been possible to tack on XCTestRun to the swift-corelibs-xctest mechanism for failure reporting, this commit instead fully integrates it. As a result, the changes are widespread: gone is XCTFailureHandler, XCTRun, and other internal structures. In their place, welcome the Apple XCTest public APIs: the XCTest abstract class, XCTestRun, and its subclasses XCTestCaseRun and XCTestSuiteRun.

In conjunction with the new XCTestSuite-related observation methods from #84, test reporting is now done exclusively through XCTestObservation. As a result, test output is now nearly identical to Apple XCTest.

Why merge this pull request?

Our goal for Swift 3 is API parity, and this pull request implements a ton of the Apple XCTest API.
This pull request implements test output that is nearly identical to Apple XCTest, including announcements about test suites starting and stopping.

What downsides are there to merging this pull request?

This is a massive overhaul of swift-corelibs-xctest internals. It's hard to review such a large set of changes. I am sorry about this, and merging [XCTestObservation] Add XCTestSuite announcements #84 first should slim this pull request down a bit--but even then, this is huge. In my defense, XCTestRun is a major part of the Apple XCTest API; introducing it to swift-corelibs-xctest requires a lot of changes.
I recommend looking at the changes to Tests/Functional first. Note that these changes report test suites, as well as start and stop times for each test suite/case.
A lot of the existing Swift code is much more "idiomatic" than the code here, which introduces two "abstract" base classes: XCTest and XCTestRun. Like it or not, this is the Apple XCTest API, which has been built for Objective-C since day one. We'll leave it to the third-party testing frameworks to build APIs more suitable to Swift.

modocache · 2016-03-28T19:25:56Z

@mike-ferris-apple @briancroom Would love to hear your feedback on this one! 👋

mike-ferris · 2016-03-29T15:46:23Z

I looked at the prior one to add suites to the observation protocol. It may be a few days before I can look at this one in any detail.

mike-ferris · 2016-04-03T15:28:04Z

Sources/XCTest/XCAbstractTest.swift

+
+    // FIXME: This initializer is required due to a Swift compiler bug on Linux.
+    //        It should be removed once the bug is fixed.
+    public init() {}


Should reference the specific bug here...

Oh, good point. I have a feeling this is subtly different from SR-1129, but I'll double check.

mike-ferris · 2016-04-03T15:45:57Z

I've looked through this. This actually seems to clean up a lot of stuff in addition to getting more API parity. I like that the printing is now handled by an observer, and, in general things seem to be better encapsulated and separated.

I don't have the bandwidth to truly assess for functional equivalence with Xcode's XCTest behavior, but the API all seems good and I saw no obvious issues in the implementation.

modocache · 2016-04-03T16:32:29Z

@mike-ferris-apple Excellent! Could you try kicking off the tests?

@briancroom said he might have time to review this soon so I'll wait for that before considering merging.

ddunbar · 2016-04-03T16:57:19Z

@swift-ci please test

briancroom · 2016-04-03T19:08:01Z

Sources/XCTest/XCAbstractTest.swift

@@ -26,6 +26,37 @@ public class XCTest {
        fatalError("Must be overridden by subclasses.")
    }

+    /// The `XCTestRun` subclass that will be instantiated when the test is run
+    /// to hold the test's results. Must be overridden by subclasses.
+    public var testRunClass: AnyClass {


Starting in the version of XCTest.framework that ships with Xcode 7.3, this property has a type of AnyClass?. If we want to strictly follow Apple XCTest here, we should use that. Alternatively, I'm wondering if it would be reasonable to adjust this to have a type of XCTestRun.Type (or XCTestRun.Type?) which would be considerably Swiftier while (IMO) not breaking the intention of the original API.

Nice catch. Updated to use AnyClass? here. Submitted https://bugs.swift.org/browse/SR-1164 (with radar) to track changing the type in the SDK overlay.

briancroom · 2016-04-03T19:35:48Z

Wow, this is really great @modocache! My biggest complaint is that you've left so little API parity work for others (i.e. me) to have fun with 😝. In all seriousness though, I'm happy to see this, and it would have been very difficult to break it into more PRs without the smaller steps being pretty meaningless on their own.

I've left a handful of minor comments and questions for you!

briancroom · 2016-04-04T01:54:40Z

Sources/XCTest/XCTestCase.swift

@@ -39,8 +44,75 @@ public class XCTestCase: XCTest {
    ///   https://bugs.swift.org/browse/SR-1129 for details.
    public var _name: String

-    public required override init() {
-        _name = "\(self.dynamicType).<unknown>"
+    public override var testRunClass: AnyClass {


XCTestCase is missing an override for testCaseCount (AFAICT it should just always return 1)

Wow, nice catch!! Thanks.

modocache · 2016-04-06T05:09:09Z

Haha, thanks @briancroom. Although, even aside from NSInvocation-related methods, performance testing, and XCUITest, there are still some API discrepancies. I was looking through the Swift headers for Apple XCTest and found several public API we don't provide or have discrepancies with:

var XCT_GENERICS_AVAILABLE: Int32 { get }
var XCT_NULLABLE_AVAILABLE: Int32 { get }
var XCT_UI_TESTING_AVAILABLE: Int32 { get }

let XCTestErrorDomain: String
enum XCTestErrorCode : Int {
  init?(rawValue rawValue: Int)
  var rawValue: Int { get }
  case timeoutWhileWaiting
  case failureWhileWaiting
}

class XCTestCase : XCTest {
  init(selector selector: Selector)
}

extension XCTestCase {
  func keyValueObservingExpectation(for objectToObserve: AnyObject, keyPath keyPath: String, expectedValue expectedValue: AnyObject?) -> XCTestExpectation
  func keyValueObservingExpectation(for objectToObserve: AnyObject, keyPath keyPath: String, handler handler: XCKeyValueObservingExpectationHandler? = nil) -> XCTestExpectation
  func expectation(for predicate: NSPredicate, evaluatedWith object: AnyObject, handler handler: XCPredicateExpectationHandler? = nil) -> XCTestExpectation
}
typealias XCKeyValueObservingExpectationHandler = (AnyObject, [NSObject : AnyObject]) -> Bool
typealias XCPredicateExpectationHandler = () -> Bool

class XCTestSuite : XCTest {
  class func defaultTestSuite() -> Self
  convenience init(forBundlePath bundlePath: String)
  convenience init(forTestCaseWithName name: String)
  convenience init(forTestCaseClass testCaseClass: AnyClass)
}

I'll create an issue to track these once this is merged. Speaking of which, I think I've addressed all feedback so far--what do you think, @mike-ferris-apple?

modocache · 2016-04-06T19:31:23Z

I rebased this onto @briancroom's changes from #88. @mike-ferris-apple, care to kick off CI for this? I'd like to merge today in order to unblock @harlanhaskins in #91.

harlanhaskins · 2016-04-06T19:33:41Z

Thanks, @modocache!

mike-ferris · 2016-04-06T19:37:56Z

@swift-ci please test

modocache · 2016-04-06T20:06:48Z

Looks like a linker failure when building for SwiftPM. Will take a look tonight.

harlanhaskins · 2016-04-06T22:10:26Z

@modocache If it's alright with you, I'd like to fix and merge #92 today.

modocache · 2016-04-06T22:53:10Z

@harlanhaskins Sure thing!

modocache · 2016-04-06T23:20:32Z

Fixed the build failure, but let's hold off on merging this until after @harlanhaskins is done with #92.

harlanhaskins · 2016-04-06T23:26:24Z

Well, we're now waiting for conflicts to resolve, so if you want to merge this wholesale now, I can update my PR.

harlanhaskins · 2016-04-06T23:28:23Z

Also, @modocache looking at the tests, there seem to be a lot of places where the line numbers are the only thing that change. It might be worthwhile to update them to use [[@LINE+<offset>]] instead.

modocache · 2016-04-06T23:32:30Z

@harlanhaskins We aren't using FileCheck, and our checker doesn't support offsets yet. Support is added in #68 but haven't landed due to some incredible merge conflicts. Will fix soon!

@mike-ferris-apple One last Swift CI test to merge?

harlanhaskins · 2016-04-06T23:33:11Z

Oops, didn't notice it wasn't FileCheck; my apologies!

mike-ferris · 2016-04-06T23:37:19Z

@swift-ci please test

A major goal for swift-corelibs-xctest is API parity with Apple XCTest. This adds the largest missing API in swift-corelibs-xctest: `XCTestRun`. In Apple XCTest, `XCTestRun` is responsible for keeping track of the result of a test run. It's an integral part of how Apple XCTest works. swift-corelibs-xctest, on the other hand, used a global array of `XCTRun` structs to keep track of how many tests passed/failed. While it may have been possible to tack on `XCTestRun` to the swift-corelibs-xctest mechanism for failure reporting, this commit instead fully integrates it. As a result, the changes are widespread: gone is `XCTFailureHandler`, `XCTRun`, and other internal structures. In their place, welcome the Apple XCTest public APIs: the `XCTest` abstract class, `XCTestRun`, and its subclasses `XCTestCaseRun` and `XCTestSuiteRun`. In conjunction with the new `XCTestSuite`-related observation methods from swiftlang#84, test reporting is now done exclusively through `XCTestObservation`. As a result, test output is now nearly identical to Apple XCTest.

modocache · 2016-04-07T02:38:31Z

The test passed before. Just rebased onto #91. @mike-ferris-apple care to run CI again?

mike-ferris · 2016-04-07T14:03:56Z

@swift-ci please test

modocache · 2016-04-07T17:38:39Z

Woohoo! 🎉

So, so close to 100% API parity (modulo performance tests, UI tests, and NSInvocation-related stuff)!

briancroom · 2016-04-07T18:00:12Z

Excellent! Thanks for your effort on this @modocache! 💯

modocache · 2016-04-29T20:43:19Z

My biggest complaint is that you've left so little API parity work for others (i.e. me) to have fun with 😝

@briancroom SR-1355 is calling out to you... 😉

briancroom · 2016-04-29T22:45:18Z

@modocache I've already started digging in to see all the failures it can throw when you misuse the API 😆

modocache mentioned this pull request Apr 2, 2016

[XCTestObservation] Add XCTestSuite announcements #84

Merged

modocache force-pushed the api-parity branch 5 times, most recently from 2331f63 to 651fd40 Compare April 3, 2016 04:26

mike-ferris reviewed Apr 3, 2016
View reviewed changes

briancroom reviewed Apr 3, 2016
View reviewed changes

briancroom reviewed Apr 4, 2016
View reviewed changes

modocache force-pushed the api-parity branch from 651fd40 to 096fed0 Compare April 6, 2016 02:11

briancroom mentioned this pull request Apr 6, 2016

[SE-0046] Implemented consistent function labels #91

Closed

modocache force-pushed the api-parity branch from 1653f99 to 8df94f5 Compare April 6, 2016 19:30

modocache force-pushed the api-parity branch from 8df94f5 to 6493cbb Compare April 6, 2016 23:19

modocache force-pushed the api-parity branch from 6493cbb to ce992f5 Compare April 7, 2016 02:37

modocache merged commit 9ff1724 into swiftlang:master Apr 7, 2016

modocache deleted the api-parity branch April 7, 2016 17:37

openradar-mirror mentioned this pull request Jun 24, 2016

25568830: XCTestCase testRunClass should be of type XCTestRun.Type in Swift lionheart/openradar-mirror#15120

Open

modocache mannequin mentioned this pull request Apr 9, 2016

[SR-906] Allow XCTestCase.continueAfterFailure to be set #403

Open

modocache mannequin mentioned this pull request May 9, 2022

[SR-1164] XCTestCase testRunClass should be of type XCTestRun.Type in Swift #393

Closed

briancroom mentioned this pull request May 9, 2022

[SR-1145] XCTestSuite doc comments describe behavior exclusive to Apple XCTest #394

Closed

Use XCTestRun to execute tests and report results #86

Use XCTestRun to execute tests and report results #86

Uh oh!

Conversation

modocache commented Mar 28, 2016

What's in this pull request?

Why merge this pull request?

What downsides are there to merging this pull request?

Uh oh!

modocache commented Mar 28, 2016

Uh oh!

mike-ferris commented Mar 29, 2016

Uh oh!

mike-ferris Apr 3, 2016

Choose a reason for hiding this comment

Uh oh!

modocache Apr 3, 2016

Choose a reason for hiding this comment

Uh oh!

mike-ferris commented Apr 3, 2016

Uh oh!

modocache commented Apr 3, 2016

Uh oh!

ddunbar commented Apr 3, 2016

Uh oh!

briancroom Apr 3, 2016

Choose a reason for hiding this comment

Uh oh!

modocache Apr 6, 2016

Choose a reason for hiding this comment

Uh oh!

briancroom commented Apr 3, 2016

Uh oh!

briancroom Apr 4, 2016

Choose a reason for hiding this comment

Uh oh!

modocache Apr 6, 2016

Choose a reason for hiding this comment

Uh oh!

modocache commented Apr 6, 2016

Uh oh!

modocache commented Apr 6, 2016

Uh oh!

harlanhaskins commented Apr 6, 2016

Uh oh!

mike-ferris commented Apr 6, 2016

Uh oh!

modocache commented Apr 6, 2016

Uh oh!

harlanhaskins commented Apr 6, 2016

Uh oh!

modocache commented Apr 6, 2016

Uh oh!

modocache commented Apr 6, 2016

Uh oh!

harlanhaskins commented Apr 6, 2016

Uh oh!

harlanhaskins commented Apr 6, 2016

Uh oh!

modocache commented Apr 6, 2016

Uh oh!

harlanhaskins commented Apr 6, 2016

Uh oh!

mike-ferris commented Apr 6, 2016

Uh oh!

modocache commented Apr 7, 2016

Uh oh!

mike-ferris commented Apr 7, 2016

Uh oh!

modocache commented Apr 7, 2016

Uh oh!

briancroom commented Apr 7, 2016

Uh oh!

modocache commented Apr 29, 2016

Uh oh!

briancroom commented Apr 29, 2016

Uh oh!

Uh oh!