Asynchronous testing API [AsyncXCTest 6/6]

[modocache]

What's in this pull request?

An attempt to add asynchronous testing support, mirroring the Objective-C XCTest API. This is done primarily via two methods:

• XCTestCase.expectationWithDescription()
• XCTestCase.waitForExpectationsWithTimeout()

The tests in the pull request pass on Darwin, but not on Linux. Still, I've encountered a few issues that I'd like to get feedback on.

What's worth discussing about this pull request?

The methods added are identical to their Objective-C SDK overlay counterparts, with the following exceptions:

1. If expectations are created but not waited for, Objective-C XCTest generates a test failure. In order to display a failure on the exact line of the last expectation that was created, Objective-C XCTest symbolicates the call stack. It does so using a private framework called CoreSymbolication.framework. We don't have this at our disposal for swift-corelibs-xctest, so instead this pull request has the method take file and line as parameters, defaulting to __FILE__ and __LINE__. By doing so, we provide the same failure highlighting functionality while also maintaining a (nearly) identical API.
2. If waitForExpectationsWithTimeout() fails, Objective-C XCTest generates a test failure on the exact line that method was called, using the same technique from (1). For the same reason, this method also takes file and line parameters in this pull request.
3. waitForExpectationsWithTimeout() takes a handler callback, which in Objective-C XCTest is passed an instance of NSError. swift-corelibs-xctest doesn't depend upon swift-corelibs-foundation, and so does not have access to NSError. Instead, it defines a shim, XCTError.

I'd like to discuss the following:

(A) Without using something like CoreSymbolication.framework, we cannot emit failures for the correct file or line while maintaining an identical API. This pull request is a sort of middle ground; the use of the API looks identical:

// Objective-C XCTest
let expectation = self.expectationWithDescription("foo")
expectation.fulfill()
self.waitForExpectationsWithTimeout(10, handler: nil)
self.waitForExpectationsWithTimeout(10) { error in
    // ...
}

// swift-corelibs-xctest
let expectation = self.expectationWithDescription("bar")
expectation.fulfill()
self.waitForExpectationsWithTimeout(10, handler: nil)
self.waitForExpectationsWithTimeout(10) { error in
    // ...
}

However, they're not totally identical. The following is only possible in swift-corelibs-xctest:

// swift-corelibs-xctest
let expectation = self.expectationWithDescription("baz", file: "/path/to/file", line: 10)
expectation.fulfill()
self.waitForExpectationsWithTimeout(10, file: "/path/to/file", line: 20, handler: nil)

Is this an acceptable solution?

(B) The code in this pull request has a dummy implementation to wait for expectations to be fulfilled. For a real implementation, I think we'll need to add a dependency to swift-corelibs-xctest, and I think that should either be swift-corelibs-libdispatch or swift-corelibs-foundation.

I think swift-corelibs-foundation would be ideal: it includes both:

• NSRunLoop, which we can use to implement the waiting logic, and...
• ...NSError, which is used by the callback to waitForExpectationsWithTimeout().

However, swift-corelibs-foundation uses swift-corelibs-xctest for its own unit test suite. So while the dependency isn't exactly cyclical (Foundation's tests depend on XCTest, which depends on Foundation--no cycle), it's still a little murky as to whether it's a good idea.

Should swift-corelibs-xctest depend on swift-corelibs-foundation? Should it depend on swift-corelibs-libdispatch? Or should it depend on neither--and if so, how can we implement asynchronous testing?

/cc @briancroom @jeffh

[modocache]

Without using something like CoreSymbolication.framework, we cannot emit failures for the correct file or line while maintaining an identical API.

Symbolication is a great solution for getting the file and line numbers in Objective-C, where default parameters aren't available. However in very large codebases, I've seen symbolication take over 90 seconds to complete. I believe the private API +[XCTestCase _enableSymbolication] was added to allow users to opt-out of symbolication--but then failure reporting doesn't work very well.

In a Swift-only API, however, I think file and line parameters are better. They work well in any sized codebase, and like XCTAssert, users will probably never actually use the parameters.

[jeffh]

Capturing the last (or all) locations of un-awaited expectations seems like a good alternative solution without having to desymbolicate. 👍

Several observations on this implementation vs Objective-C XCTest (for which Nimble's is a similar to):

1. This implementation has a leeway of max(min(1/10th of the timeout interval, 60 * NSEC_PER_SEC), NSEC_PER_MSEC). The Objective-C XCTest sets this to 0 since it uses timer sources directly over dispatch_after. Along with using the global high priority queue, this decreases the likelihood of the timer not triggering at the desired timeout because of (dispatch) run loop congestion.
2. Objective-C XCTest doesn't actually poll for fulfillment of expectations. fulfill() triggers "poll-check" - that is the machinery to potentially stop the timer if all expectations are met.
3. fulfill() multiple times for the same expectation is an API Violation in Objective-C XCTest.
4. There's no attempted detection and recovery / abort when the main thread is blocked. Nimble attempts to continue unsafely by forcing the runloop to stop (but failing that test). Objective-C XCTest restarts the testing process. I'm not sure what's the recommended strategy here.

I think NSRunLoop is used to block instead of dispatch_semaphore_wait because it can still process traditional run loop operations, but I'm not familiar to how closely tied Objective-C CoreFoundation's CFRunLoop sources and libdispatch sources/queues are. So that's just an educated guess.

Just my 2 cents.

[modocache]

Thanks for the comments, @jeffh! 👏

Capturing the last (or all) locations of un-awaited expectations seems like a good alternative solution without having to desymbolicate.

This is actually what Objective-C XCTest does as well! Running the following will report a single failure on the line where the last expectation is created:

func test_expectationsUnawaited() {
    let _ = self.expectationWithDescription("foo")
    let _ = self.expectationWithDescription("bar") // Failure reported here in Objective-C XCTest
}

3. fulfill() multiple times for the same expectation is an API Violation in Objective-C XCTest.

Nice catch--I'll update the code in this RFC. 👍

Thanks for bringing up (1), (2), and (4)--they'll be important to keep in mind once we've decided which dependencies to take on, and are ready to focus on implementation.

[briancroom]

I agree with @jeffh that using the file/line params seems a very appropriate (and Swifty) way to capture the callsite info needed for failure reporting.

Pulling in Corelibs Foundation for NSRunLoop and NSError seems preferable to me as well for driving the asynchronous flows necessary for this. @phausler do you have any concerns about Foundation being testing by a library which itself uses Foundation?

[parkera]

We have that circular dependency for testing Foundation with XCTest on Darwin as well, and it's never bothered me. I figure if we break NSRunLoop or NSError, then the tests are still likely to fail. It may be harder for us to figure out why, but that's the cost of being a low level library.

[parkera]

@mike-ferris-apple should still comment on the actual substance of this RFC though.

[phausler]

@briancroom I am of the opinion that testing something with itself only makes sense when the project is very mature. NSRunLoop is very new to swift-corelibs-foundation and I'm certain that we have at least one or two bugs still lurking around there. If we can help it I'd vote in favor of avoiding a circular graph if at all possible.

Perhaps a conditional inclusion for upper level frameworks/apps that depend on Foundation might be interesting.

But as @parkera mentioned @mike-ferris-apple should probably be the point of inquiry on that.

[mike-ferris]

Just a quick reply saying that I see this and will take a look when I get a chance. May be a couple days at this point, though.

[mike-ferris]

On point (A) in the original pull request, I think this seems like a reasonable solution. Writing source compatible tests is still very much possible, and mostly this will be transparent to folks.

Point (B) says that there's a dummy implementation, from what I am seeing it looks like the current code would simply not compile on Linux? @briancroom commented that using Foundation to get access to NSError and NSRunLoop might be preferable and I would tend to agree. But I also agree with @phausler that establishing a dependency of Foundation (and in particular on NSRunLoop) is worrying given the current state of maturity of the corelibs Foundation.

I have my doubts about how practically useful this API will be as long as the wait call is always guaranteed to wait the full timeout. I know from experience that when we have had bugs in Xcode's implementation that yielded the same behavior it winds up making tests run unacceptably slow.

[modocache]

Point (B) says that there's a dummy implementation, from what I am seeing it looks like the current code would simply not compile on Linux?

Yup! To be clear: the current implementation only works on OS X, and is meant purely as a placeholder. My plan was to add a cross-platform implementation (that also satisfied the expectations before the timeout when possible), just as soon as we reached a consensus on whether to depend upon swift-corelibs-libdispatch or swift-corelibs-foundation.

It sounds like while there are worries about the maturity of swift-corelibs-foundation, no one is outright opposed to the idea of swift-corelibs-xctest depending upon swift-corelibs-foundation. I agree with this sentiment.

As a next step, I'd like to amend this pull request with an implementation that depends upon swift-corelibs-foundation. Integrating swift-corelibs-foundation into this project and the build script may end up being a good chunk of work, so if anyone's 100% opposed to this idea, let me know ASAP, and save me a weekend of free time! 😉

[mike-ferris]

Let's separate the question slightly...

@parkera and @phausler, I'd love to here your thoughts.

1. Would there be any objection to generally establishing a dependency on Foundation? For example, what if it was for access to NSError?

2. is the NSRunLoop question specifically... This is the probably more interesting one in terms of the newness of that code.

@modocache I would suggest that if we're OK with #1 in general, then this is worth pursuing, and that perhaps we can structure the waiting implementation internally to possibly allow for alternate implementations to be tried out... I am not terribly sure what a non-libdispach/non-Foundation implementation of this would look like, but we could at least keep that door open.

[parkera]

No objection from me on using Foundation (with the caveat that we know a big renaming effort is under way so you may have to update soon).

The run loop implementation in corelibs Foundation is actually pretty close to mature. It's 95% the same as our other platforms and the Linux-specific stuff looks very reasonable to me.

[modocache]

@parkera @phausler Here I've implemented asynchronous tests in XCTest using SwiftFoundation's NSRunLoop. Unfortunately, this doesn't exactly work--unlike when I use Objective-C Foundation, the SwiftFoundation runloop here does not run until the specified date.

In order to get this to work as it does with Objective-C Foundation, I had to make the following change to SwiftFoundation:

diff --git a/Foundation/NSRunLoop.swift b/Foundation/NSRunLoop.swift
index 4434429..6288242 100644
--- a/Foundation/NSRunLoop.swift
+++ b/Foundation/NSRunLoop.swift
@@ -95,10 +95,6 @@ extension NSRunLoop {
             return false
         }
         let modeArg = mode._cfObject
-        if _CFRunLoopFinished(_cfRunLoop, modeArg) {
-            return false
-        }
-
         let limitTime = limitDate.timeIntervalSinceReferenceDate
         let ti = limitTime - CFAbsoluteTimeGetCurrent()
         CFRunLoopRunInMode(modeArg, ti, true)

I examined the implementation of _CFRunLoopFinished to see what was amiss, but couldn't figure it out. Any ideas?

[modocache]

Looks like the above works perfectly fine on Linux. Perhaps swift-corelibs-xctest should depend upon Objective-C Foundation when being built for OS X, instead of SwiftFoundation?

[modocache]

I updated the commit to use SwiftFoundation. The project builds on OS X, and its tests pass if I make the change to NSRunLoop I describe in a comment above.

I use the same technique swift-corelibs-foundation does to specify a dependency. swift-corelibs-xctest now assumes ../swift-corelibs-foundation/Foundation.xcodeproj exists. I'll add a note to the README to make sure contributors know what's necessary to build the project now.

For Linux, I plan on massaging the build script and lit.cfg sometime soon in order to get things working.

[modocache]

OK! I pushed up a few changes to build_script.py and the tests' lit.cfg. These changes are dependent upon #46, so I'd love to see that PR merged soon.

This now works perfectly on Linux. It works fine on OS X, aside from that weird NSRunLoop behavior -- @parkera, @phausler, any help here would be appreciated. Should we depend on Objective-C Foundation on OS X, instead of SwiftFoundation?

@mike-ferris-apple, you'll be happy to hear that XCTestCase.waitForExpectationsWithTimeout() only waits as long as the expectations are unfulfilled, or when the timeout is reached. So tests for expectations that are fulfilled immediately pass immediately.

There are some changes to how this project is built: see my changes to the README for details. These changes will cause swift/utils/build-script --xctest to break. I'll send a corresponding pull request to apple/swift before we merge these changes.

fulfill() multiple times for the same expectation is an API Violation in Objective-C XCTest.

@jeffh, I still haven't added addressed this comment, but I will! 😝

[modocache]

By the way, @briancroom: an interesting aspect of the new dependency on Foundation is that we can use NSTimeInterval for time measurements instead of Double. ⌛ 👍

[modocache]

Here's how the build script will need to change once these changes are merged: modocache/swift@a17c05b. That commit allows me to build and test swift-corelibs-xctest from OS X and Linux.

[parkera]

@modocache I think it's fine in this case to use the Swift version of Foundation as a dependency on OS X. My thinking is this: the reason that swift-corelibs-foundation and swift-corelibs-xctest run on OS X at all is as a convenience for us to develop them in an environment that most of us are already familiar with (OS X, Xcode, etc). The deployment target of these two libraries is currently just Linux though (you'll notice that they are not present in the packages created for OS X).

Plus, swift-corelibs-foundation already uses swift-corelibs-xctest as its test harness on OS X instead of the system one. So we have a kind of precedent there.

[modocache]

Plus, swift-corelibs-foundation already uses swift-corelibs-xctest as its test harness on OS X instead of the system one.

It does?! Wow, this is news to me. I had assumed that, like swift-package-manager, it conditionally used either Objective-C XCTest or swift-corelibs-xctest, based on the host platform. It's very cool that it uses swift-corelibs-xctest on all platforms! 💯

I think it's fine in this case to use the Swift version of Foundation as a dependency on OS X.

Awesome, great to hear. Any ideas on the behavior discrepancy around NSRunLoop I describe in #43 (comment)?

[parkera]

If you can, let's open a PR on swift-corelibs-foundation to discuss the required change for NSRunLoop.

[modocache]

@parkera Sure thing: swiftlang/swift-corelibs-foundation#258 Thanks!

[briancroom]

Nice documentation on these!

[briancroom]

Thanks for all the persistent work on this @modocache! It was really cool to see it all working on both platforms when I checked it out today. I've gone through and left a bunch of comments, some nits, some larger discussion points, but none of which I would consider to be merge blockers on this.

[mike-ferris]

Lots of activity here, trying to make sense of it all.

@modocache , on your checklist above, 1, 2 and 3 look good to me. Since there's been a bit of discussion about what parameters should be required in the pm build script after this change, it would probably be a good idea to get buy in from @mxcl or @ddunbar for that PR.

@briancroom had a bunch of interested comments/suggestions above. I agree with him that most changes from his suggestions could be addressed as later refinements if you want (though see below for one possible exception).

On the meat of the PR, I made a couple comments on individual parts of the commit. Overall this looks good! Thanks for your persistence in putting this all together! The one thing that seems possibly worth addressing is the business about keeping the expectation failures out of band with the regular failure stream.

Brian, once any final tweaks are made and once the pm guys sign off, would you like to do the actual merging here since it will need several steps to be staged? If testing is needed in the middle of the sequence, ping me and I can kick it off.

[modocache]

Thanks for the excellent reviews, @briancroom and @mike-ferris-apple! 💯 I believe I've addressed all the comments.

@mike-ferris-apple, as you suggested, I'll wait for @mxcl or @ddunbar to review swiftlang/swift-package-manager#176 and swiftlang/swift-package-manager#177. If those look good, I'll merge everything in the order I specified (except for the SwiftPM pull requests, I don't have commit access for those).

As for CI: @mike-ferris-apple, could you "@swift-ci please test" the following three pull requests?

• [build_script.py] Add --foundation-build-dir parameter [AsyncXCTest 1/6] #62
• Preemptively add XCTest.xcworkspace [AsyncXCTest 2/6] #63
• [bootstrap] Preemptively add argument for Foundation [AsyncXCTest 3/6] swift-package-manager#176

[modocache]

@mike-ferris-apple Everything's in place for this to be merged. Could you ask @swift-ci to please test? If the tests go well on Linux I'll merge. (I assume the CI still doesn't work for OS X, I'll test that locally prior to merging.)

[mike-ferris]

@swift-ci please test

[modocache]

I'll double check this works on OS X, then merge this within the next few hours.

[modocache]

We did it!! 🎉

Thanks for all the awesome reviews and the amazing troubleshooting everyone. I'll send an email to the corelibs-dev mailing list to let people know about the new Foundation dependency.

[briancroom]

Yeaaaah! So exciting to see this go in.

[mike-ferris]

Great! Thanks for your perseverance on this one!