[SR-460] Provide XCTestCase a class to allow overriding setUp() and tearDown()

[briancroom]

What is it?

This PR renames the XCTestCase protocol to XCTestCaseType and introduces a new XCTestCase class in its place which individual test cases may derive from. A relevant test case is also included (see below).

Motivation

Providing XCTestCase as just a protocol rather than a class is a great concept, however it presents an insurmountable barrier to achieving source-level compatibility with Darwin XCTest with respect to defining setUp() and tearDown() methods in test cases, because the override keyword is required when inheriting (Darwin XCTest) but is prohibited when just conforming to the protocol (Corelibs XCTest). With this patch, it becomes possible to write a test case utilizing setUp() and tearDown() which builds with both frameworks. Here is the example from SR-460:

import XCTest

class Test: XCTestCase {
    var allTests: [(String, () -> Void)] { return [("testExample", testExample)] }

    override func setUp() {
        super.setUp()
    }

    override func tearDown() {
        super.tearDown()
    }

    func testExample() {
        XCTAssertEqual(1, 1)
    }
}

Drawbacks

There is one significant caveat to this change: test cases deriving from the new XCTestCase class must still explicitly declare conformance to the XCTestCaseType protocol before they can be passed to XCTMain, which makes this a breaking change for current users of Corelibs XCTest. (The XCTestCase base class can't conform to XCTestCaseType without losing compile-time checking that individual test cases provide an allTests property for their own tests.) I see two reasonable approaches to this for cross-platform tests:

1. Conditionally redeclare XCTestCaseType on Apple platforms, and conform to the protocol in-line for each test case. This is essentially what swiftpm is currently doing (see here and here).
2. Add XCTestCaseType conformance in a separate source file (perhaps the main.swift entry point which contains the call to XCTMain). I can easily imagine running a script as a part of the build process which dynamically generates a source file adding this conformance to each test case class. Such a script could even be made to generate the allTests property implementation automatically as well.

Rejected Alternatives

I considered a couple of variants before settling on the current contents of this patch. Note that neither of these include the XCTestCaseType protocol:

• Convert the XCTestCase protocol into a class which conforms to XCTestCaseProvider. In this version, the base XCTestCase class included an empty allTests definition. This was rejected both because it required individual test cases to mark their allTests definitions as override, and because there would be no compile-time checking that an overridden allTests was present at all.
• Convert the XCTestCase protocol into a class which does not conform to XCTestCaseProvider. In this implementation, XCTMain remained unchanged, and accepted an array of XCTestCase classes. This fared poorly because it required a runtime check that individual test cases conform to XCTestCaseProvider, which felt like a missed opportunity to leverage the type system.

I look forward to getting feedback and iterating on this!
@mike-ferris-apple @eschaton @modocache

[modocache]

Oh man, thank you for this proposal. The other day I was trying to implement XCTestObservation in swift-corelibs-xctest, and realized that it doesn't really work unless we have XCTestCase instances, and those have XCTestRun attributes. I think this direction would open up a lot of possibilities.

A few questions:

1. In Darwin XCTest, a new test case instance is instantiated for each test run. How would you feel about doing so here as well?
2. Is there an equivalent to NS_REQUIRES_SUPER in Swift? It would be nice to remind users to call super.setUp() and super.tearDown().
3. It is a shame test cases must inferit/conform to two things: XCTestCase, XCTestCaseType is a lot of typing. Upon first glance your second "rejected alternative", of checking at runtime, seems like it has potential... I'll need to stew on this more.

I'm also very interested in what the core team thinks here.

[mike-ferris]

I have been intending to make a similar change as well.

If we have XCTestCase as a base class, I am wondering what advantage we really get having the XCTestCase protocol (XCTestCaseType in your proposal), though, and even what benefit we’re really getting from XCTestCaseProvider. I had more or less planned to remove both the protocols in favor of implementing an XCTestCase base class.

For me, these changes were motivated by wanting to implement XCTestObservation (as Brian G. talks about below.)

On Jan 14, 2016, at 11:49 AM, Brian Gesiak notifications@github.com wrote:

Oh man, thank you for this proposal. The other day I was trying to implement XCTestObservation in swift-corelibs-xctest, and realized that it doesn't really work unless we have XCTestCase instances, and those have XCTestRun attributes. I think this direction would open up a lot of possibilities.

In my change for this I had been avoiding introducing XCTestRun.
A few questions:

In Darwin XCTest, a new test case instance is instantiated for each test run. How would you feel about doing so here as well?

For XCTestObservation to be useful, we will need to do this. This will also most likely require that XCTMain change to take XCTestCase classes instead of instances. (And that will require some coordination of changes in the two main projects that are clients of XCTest… corelibs Foundation and the package manager).

Is there an equivalent to NS_REQUIRES_SUPER in Swift? It would be nice to remind users to call super.setUp() and super.tearDown().
It is a shame test cases must inferit/conform to two things: XCTestCase, XCTestCaseType is a lot of typing. Upon first glance your second "rejected alternative", of checking at runtime, seems like it has potential... I'll need to stew on this more.
I'm also very interested in what the core team thinks here.

—
Reply to this email directly or view it on GitHub #40 (comment).

Mike

[briancroom]

Cool. I had been trying to preserve the protocol to avoid locking things down too much, but it sounds like things are headed towards a place that requires a tighter interface. I've now pushed some modified commits which do away with the protocols as @mike-ferris-apple suggested. The original branch with the XCTestCaseType protocol is still available here for reference.

I have started prototyping some changes that would allow a new XCTestCase instance to be used for each test, but none of that is included in this PR.

[mike-ferris]

When we take this pull request, we're going to need existing clients to update code, right? (At least they'll be adding the "override" on their allTests, and on setUp() and tearDown() if they have them.)

We're going to have to coordinate with the foundation and package manager projects to get patches for their tests to go in at the same time these changes go.

When we change the execution semantics to an instance per test, we will also need to make another source breaking change. I think it might be good to do that at the same time so we will hopefully not have to do another coordinated patch of foundation and package manager at that time.

I would recommend we change the signature of XCTMain() to be:

func XCTMain(testCaseClasses: [XCTestCase.Type])

In addition, I found it useful to change allTests into a dictionary when I was prototyping the change to the execution model as well:

public var allTests: [String: () -> Void]

How do you feel about making these changes preemptively as well to minimize the number of changes we're having to coordinate with the two major clients?

[mike-ferris]

One additional detail about allTests as a dictionary... this removes ordering from allTests. To replace that, I was sorting the tests by name. This makes it less practical to order tests specifically, but actually that's arguably a bad thing to do, especially if/when a test harness starts allowing executing subsets of test cases. Allowing ordering dependencies between test cases work against having suites that can be decomposed cleanly.

[parkera]

We can react quickly in Foundation once you make this change, so don't worry about that part (cc @phausler). @ddunbar probably should just have a heads up as well for the package manager.

[modocache]

I found it useful to change allTests into a dictionary

This forces all tests to have unique names. An interesting aspect of the current tuple-based API is that tests need not be uniquely named within a test case. I actually consider this to be a feature; XCTest/Xcode integration implicitly requires test selectors to be unique, which causes subtle problems when dynamically generating test methods. See: specta/specta#83 (comment)

I'm not against requiring each test method name to be unique within a test case, provided there's a benefit to doing so. What's the upside here?

[phausler]

The one problem with this is that the tests won't be portable to Darwin. Because there will be an override that isn't overriding anything.

I find it quite nice that I can copy/paste most tests across and run them against the Foundation that ships with Mac OS X.

[mike-ferris]

Arg! Good point. Note that part of the motivation here was to solve the opposite problem with setUp() and tearDown().

Not sure what to do here. If we keep that in the TestProvider protocol then all tests need to do the double inheritance (and actually that probably creates its own problems with the portability of tests).

One possibility would be to add allTests to XCTestCase in the overlay so it’s an override in Xcode as well. I am not thrilled with that and would want to talk it through more here and with the rest of the Xcode XCTest folks as well…

Any other options?

Mike

On Jan 19, 2016, at 10:25 AM, Philippe Hausler notifications@github.com wrote:

The one problem with this is that the tests won't be portable to Darwin. Because there will be an override that isn't overriding anything.

I find it quite nice that I can copy/paste most tests across and run them against the Foundation that ships with Mac OS X.

—
Reply to this email directly or view it on GitHub #40 (comment).

[parkera]

Can setUp and tearDown come from inheritance and allTests come from protocol conformance?

[parkera]

We would probably then require an empty protocol of the same name on Darwin to avoid having clients #ifdef. That's not really that great either.

[mike-ferris]

Perhaps there’s a better way to do this, but in order to prototype the execution model I wound up needing to instantiate an instance just to get the test list. Then as the tests are being run an instance has to be created to actually run each test case. The problem was that you need to use the closure from allTests for the instance that is going to run each test, so the initial instance gathered up the names and then passed each one into the instance that was going to run an individual test and it would look the closure up in allTests for that instance to get a block bound to the right self. Having the dictionary allowed that lookup not to be linear.

It may be that there’s a cleverer way than I could think of to do this better. It sucks to have to create N+1 instances as well… If we can’t come up with a different approach, we could keep allTests as an array if we were willing to search it linearly, but that winds up being n-squared on the number of tests since we have to search once for each test.

Mike

On Jan 19, 2016, at 10:19 AM, Brian Gesiak notifications@github.com wrote:

I found it useful to change allTests into a dictionary

This forces all tests to have unique names. An interesting aspect of the current tuple-based API is that tests need not be uniquely named within a test case. I actually consider this to be a feature; XCTest/Xcode integration implicitly requires test selectors to be unique, which causes subtle problems when dynamically generating test methods. See: specta/specta#83 (comment) specta/specta#83 (comment)
I'm not against requiring each test method name to be unique within a test case, provided there's a benefit to doing so. What's the upside here?

—
Reply to this email directly or view it on GitHub #40 (comment).

[mike-ferris]

Yeah, if we’re doing that, I’d rather add allTests to XCTestCase in Xcode… At least then people wouldn’t need to have two things in their inheritance list every time.

Mike

On Jan 19, 2016, at 10:38 AM, Tony Parker notifications@github.com wrote:

We would probably then require an empty protocol of the same name on Darwin to avoid having clients #ifdef. That's not really that great either.

—
Reply to this email directly or view it on GitHub #40 (comment).

[modocache]

in order to prototype the execution model I wound up needing to instantiate an instance just to get the test list

I believe this is why, in Apple XCTest, +[XCTestCase testInvocations] is a class method. That way, you don't need to instantiate a test case in order to gather its tests. Apple XCTest then uses the -[XCTestCase initWithInvocation:] initializer to instantiate the test case to run the test. Could we change allTests to a static var or func?

The problem was that you need to use the closure from allTests for the instance that is going to run each test, so the initial instance gathered up the names and then passed each one into the instance that was going to run an individual test and it would look the closure up in allTests for that instance to get a block bound to the right self. Having the dictionary allowed that lookup not to be linear.

In my experience, the most time-expensive aspect of Apple XCTest was XPC communication between the XCTest driver, Xcode, and the XCTest host on the iOS/tvOS/watchOS device/simulator. My gut is to not worry too much about linear lookups until they demonstrate themselves to be a performance bottleneck--but that's just my two cents! 💸

We would probably then require an empty protocol of the same name on Darwin to avoid having clients #ifdef. That's not really that great either.

Uh-oh, somebody better tell Joar. 😉

I'm hesitant to advocate adding this API to Apple XCTest. Perhaps we could migrate away from +[XCTestCase testInvocations] to something that we could use here, but even then, I imagine any API churn on Apple XCTest will be an extremely hard sell.

[mike-ferris]

On Jan 19, 2016, at 11:02 AM, Brian Gesiak notifications@github.com wrote:

in order to prototype the execution model I wound up needing to instantiate an instance just to get the test list

I believe this is why, in Apple XCTest, +[XCTestCase testInvocations] is a class method. That way, you don't need to instantiate a test case in order to gather its tests. Apple XCTest then uses the -[XCTestCase initWithInvocation:] initializer to instantiate the test case to run the test. Could we change allTests to a static var or func?

NSInvocation has the -setTarget: method, though. In Swift, if you implement allTests as a class property it returns unbound test closures which changes the signature. And, not only that, but getting the typing right proved to be problematic… You would want testClosure(self: XCTestCase.Type) but when you take a closure for a method on MyTestCaseSubclass you get a closure of type testClosure(self: MyTestCaseSubclass.Type) which is not compatible with the first signature.

Someone with more generic Swift-fu than I might be able to come up with a way around this…

The problem was that you need to use the closure from allTests for the instance that is going to run each test, so the initial instance gathered up the names and then passed each one into the instance that was going to run an individual test and it would look the closure up in allTests for that instance to get a block bound to the right self. Having the dictionary allowed that lookup not to be linear.

In my experience, the most time-expensive aspect of Apple XCTest was XPC communication between the XCTest driver, Xcode, and the XCTest host on the iOS/tvOS/watchOS device/simulator. My gut is to not worry too much about linear lookups until they demonstrate themselves to be a performance bottleneck--but that's just my two cents!

We would probably then require an empty protocol of the same name on Darwin to avoid having clients #ifdef. That's not really that great either.

Uh-oh, somebody better tell Joar https://twitter.com/joar_at_work.

I'm hesitant to advocate adding this API to Apple XCTest. Perhaps we could migrate away from +[XCTestCase testInvocations] to something that we could use here, but even then, I imagine any API churn on Apple XCTest will be an extremely hard sell.

I wouldn’t mind migrating Xcode’s XCTest toward something more swift-y than +testInvocations if we could figure out a good approach. Probably something more block-y. And potentially this could be staged in as a new primitive that is implemented in terms of +testInvocations to avoid disruption, but the first thing would be figuring out what we do want.

Mike

[briancroom]

Thanks for the discussion around this guys. I completely agree that we should aim to consolidate the breaking changes required for these two features.

When I was hacking on the instance-per-test concept before, I ran into precisely the same issues that @mike-ferris-apple describes. I still have a couple of other ideas to work around the generics-related issues when making allTests a class variable - probably involving a thunk/type eraser of some sort when calling into XCTMain. I'll update once (if?) I figure out a way to make it syntactically palatable.

[briancroom]

Sorry for the slow turnaround on this.

After much wrangling with the type system, I have ended up with two approaches, either of which I would be happy to see merged. I've pushed my preferred option to this PR's branch, and the alternative one to another branch. Both branches include additional test cases covering the newly-introduced behaviors.

To recap, the requirements I was aiming to meet are:

• Source code compatibility by allow test cases to override setUp() and tearDown(), including super calls, with same syntax as Xcode's XCTest
• Source code compatibility by ensuring that the allTests property need not be marked as override
• Behavioral compatibility by executing each test method with a new instance of its test case object

One set of changes is shared by the two branches. The two source code compatibility requirements demand that XCTestCase be a class which defines setUp() and tearDown(), but not allTests. However since the test runner requires access to all three of these, I have defined an XCTestCaseType protocol which unifies them. This does require test cases to declare an extra protocol conformance, however I know of no way around that. I additionally dropped XCTestCaseProvider as I failed to see any continued benefit to its existence as an independent protocol.

XCTMain's signature then becomes: @noreturn public func XCTMain(testCases: [XCTestCaseType.Type])

The two branches vary in their handling of the allTests property.

---

This current branch achieves the goal of turning allTests into a static property. It solves the generics-related challenges that @mike-ferris-apple described so clearly above by providing a generic adapter function which performs the type erasure necessary to keep allTests non-generic. Test cases' allTests declarations thus take the following form:

static var allTests: [(String, XCTestCaseType throws -> Void)] {
    return [
        ("test_passes", test(test_passes)),
    ]
}

In my opinion the only drawback to this approach is that it requires the use of this new test function when declaring allTests, however I feel that this is fairly minor, considering that this is in a block of code which is specific to Corelibs XCTest users anyway. (I also still think that we should investigate the feasibility of providing a build-time tool that would automatically generate a file adding allTests and XCTestCaseType conformances, but that's a separate issue.)

---

The other branch keeps allTests as an instance property, and is very similar to what I assume @mike-ferris-apple prototype (mentioned above) must have looked like. It suffers from the problem described above of requiring N+1 instantiations of the test case, and needing re-execute allTests on each test case instance and looking up tests by name within the returned collection. This branch keeps allTests as an array (with the associated extra lookup cost), however it would be trivial to change it to be a dictionary as previously described.

---

I'm looking forward to getting feedback on these branches. Note that in either case, documentation updates (particularly in the README) would still be required before any merge would be completed.

[mike-ferris]

I wanted to at least reply as it may be a few days before I can look at these in detail.

Conceptually the notion of saying test(testMethod) in the allTests seems like a small price to pay. I’ll try to shop this idea around with some folks here who may not be following this discussion as closely including the foundation and package manager folks.

Thanks Brian!

Mike

On Jan 22, 2016, at 7:43 AM, Brian Croom notifications@github.com wrote:

Sorry for the slow turnaround on this.

After much wrangling with the type system, I have ended up with two approaches, either of which I would be happy to see merged. I've pushed my preferred option to this PR's branch, and the alternative one to another branch https://github.com/briancroom/swift-corelibs-xctest/tree/new_instance_for_each_test. Both branches include additional test cases covering the newly-introduced behaviors.

To recap, the requirements I was aiming to meet are:

Source code compatibility by allow test cases to override setUp() and tearDown(), including super calls, with same syntax as Xcode's XCTest
Source code compatibility by ensuring that the allTests property need not be marked as override
Behavioral compatibility by executing each test method with a new instance of its test case object
One set of changes is shared by the two branches. The two source code compatibility requirements demand that XCTestCase be a class which defines setUp() and tearDown(), but not allTests. However since the test runner requires access to all three of these, I have defined an XCTestCaseType protocol which unifies them. This does require test cases to declare an extra protocol conformance, however I know of no way around that. I additionally dropped XCTestCaseProvider as I failed to see any continued benefit to its existence as an independent protocol.

XCTMain's signature then becomes: @NoReturn public func XCTMain(testCases: [XCTestCaseType.Type])

The two branches vary in their handling of the allTests property.

This current branch achieves the goal of turning allTests into a static property. It solves the generics-related challenges that @mike-ferris-apple https://github.com/mike-ferris-apple described so clearly above by providing a generic adapter function which performs the type erasure necessary to keep allTests non-generic. Test cases' allTests declarations thus take the following form:

static var allTests: [(String, XCTestCaseType throws -> Void)] {
return [
("test_passes", test(test_passes)),
]
}
In my opinion the only drawback to this approach is that it requires the use of this new test function when declaring allTests, however I feel that this is fairly minor, considering that this is in a block of code which is specific to Corelibs XCTest users anyway. (I also still think that we should investigate the feasibility of providing a build-time tool that would automatically generate a file adding allTests and XCTestCaseType conformances, but that's a separate issue.)

The other branch https://github.com/briancroom/swift-corelibs-xctest/tree/new_instance_for_each_test keeps allTests as an instance property, and is very similar to what I assume @mike-ferris-apple https://github.com/mike-ferris-apple prototype (mentioned above) must have looked like. It suffers from the problem described above of requiring N+1 instantiations of the test case, and needing re-execute allTests on each test case instance and looking up tests by name within the returned collection. This branch keeps allTests as an array (with the associated extra lookup cost), however it would be trivial to change it to be a dictionary as previously described.

I'm looking forward to getting feedback on these branches. Note that in either case, documentation updates (particularly in the README) would still be required before any merge would be completed.

—
Reply to this email directly or view it on GitHub #40 (comment).

[briancroom]

Thanks for the note, Mike.

When you do get a chance to take a closer look, please consider whether these changes are compatible with preparing for the XCTestObservation work you mentioned in your first reply. I wasn't explicitly targeting that because I didn't fully understand the requirements you had in mind. But, for example, might it be necessary to make XCTestCaseType a class protocol?

[modocache]

In the grand scheme of things, I feel this is fairly painless for end-users. 👍

If they're already writing cross-platform tests (like the swift-corelibs-foundation and swift-package-manager projects are), then they have a conditionally-compiled file named that's used to call XCTMain(). This pull request simply requires those projects to add extensions like these in that file, one for each of their test cases.

Once we can use runtime reflection to automatically discover tests, we can simplify this quite a bit--we won't need to jump through hoops to ensure we don't override allTests. In the meantime, this seems like the best way to proceed.

[parkera]

@czechboy0 interesting idea; I hadn't thought of using SwiftPM to find the test cases instead of building it into metadata in the runtime.

[parkera]

@mike-ferris-apple This seems reasonable to me. CC @phausler

[phausler]

the idea seems pretty solid as an approach. What would we need to do for swift-corelibs-foundation? How can one disable tests with this approach? or exclude them from certain platforms? So this means we can get rid of the main.swift in our test project and the allTests perhaps? If we can do that a big 👍

[briancroom]

@mike-ferris-apple I just opened swiftlang/swift-package-manager#158 and swiftlang/swift-corelibs-foundation#270 with the required changes for those projects. This should allow us to move forward with merging these changes in even before @czechboy0's (exciting!) work is ready to go.

[czechboy0]

@briancroom 👍 Only a small change will be required in the new test manifest generator once this gets merged.

[mike-ferris]

OK. I checked with @mxcl and @parkera and got the go ahead. Assuming all three of these patches can be cleanly merged in the morning, I will do it then.

[mike-ferris]

@swift-ci please test

[briancroom]

I just rebased them all now, so hopefully they will still be good to go then.

[mike-ferris]

OK. In that case, let's go now. All three have been merged.

[briancroom]

Awesome! Thanks for doing the honors @mike-ferris-apple 🎉

I appreciate everyone that's been involved in the discussions around this over the past weeks and thanks for your patience through all the iterations 😄