XCTestObservation and XCTestObservationCenter #69

briancroom · 2016-03-10T22:01:23Z

As requested by @modocache, here is the start of some work on this missing feature for early review. I'm waiting for the next snapshot to rebase on top of the latest master because I don't have an environment with the new Swift 3 stdlib and corelibs-foundation stuff at this point.

For the XCTestObservation protocol, I've only included the methods related to XCTestCase at this point, not the bundle or test suite ones. If and when we decide to introduce XCTestSuite we can introduce the relevant events at that point.

Note that I haven't touched XCTestRun yet which could be added to provide a useful summary (failure count, test case success) in testCaseDidFinish(). I do remember that @mike-ferris-apple had mentioned avoiding it for some reason, and I was hoping to get an understanding of why that was before starting down that path. I did add XCTestCase's name property though, which allows differentiating between instances in the observation callbacks.

Method documentation will also still be needed before this is mergeable! Looking for feedback first though 😀

czechboy0 · 2016-03-10T22:04:48Z

Sources/XCTest/XCTestObservation.swift

+
+public class XCTestObservationCenter {
+    private static var center = XCTestObservationCenter()
+    private var observers = [XCTestObservation]()


Would it help to have Set<XCTestObservation> here instead, so that below you can avoid looking up the index of the observer to be removed?

Unfortunately not, because XCTestObservation doesn't conform to Hashable. I briefly considered NSMutableSet but I'm not convinced that it's worth losing the strong typing.

Hmm since we know it's a class protocol, I'm surprised we can't force the set to use the pointer value as the hash value (basically defining XCTestObservation's equality as ===).

Yeah, that would make a lot of sense. AFAICT there's no way to do that without making XCTestObservation itself extend Hashable though.

modocache · 2016-03-11T04:22:46Z

Awesome, thanks @briancroom!! It's so exciting to see one of the final hurdles to API parity with Apple XCTest getting tackled. 👍

If and when we decide to introduce XCTestSuite we can introduce the relevant events at that point.

This sounds like a reasonable approach. Also, thanks for linking to @mike-ferris-apple's comment about avoiding XCTestRun. That sounds strange to me, though--to write a cross-platform test observer, I'd expect XCTest users would want to write the exact same code--which means accessing the test results via testCase.testRun. I'll let him comment further, but I think this is something we should resolve sooner rather than later.

modocache · 2016-03-11T04:25:12Z

Sources/XCTest/XCTestCase.swift

@@ -92,6 +99,8 @@ extension XCTestCase {

                XCTPrint("Test Case '\(fullName)' started.")

+                observationCenter.testCaseWillStart(testCase)


Here's a crazy idea: could we define an XCTestObserver that we use internally to call XCTPrint()? That is, could we use our own observation infrastructure to print the default test output?

I'm not sure off the top of my head whether Apple XCTest does this, but I think it'd be wild! 😎

As it turns out, Apple XCTest does exactly that (well, not exactly, because the logging uses the legacy XCTestObserver protocol, but close enough. I learned all this while working through Quick/Quick#486)

I would like to make that refactor, yes. In fact, one reason why I wrote the observer in the functional test to keep track of state in properties instead of just printing was to facilitate this, since XCTestObservationCenter doesn't provide any guarantees of the order in which observers are called.

mike-ferris · 2016-03-11T23:26:12Z

I don't recall exactly the thinking behind my comment about avoiding XCTestRun... It may have been motivated primarily by wanting to keep things simple at first. I think @modocache makes a good point that for some of what folks would be doing in an observer, they may need access and if that looks different, it's not portable.

briancroom · 2016-03-14T07:16:42Z

I've rebased this and done some cleanups now:

Switch to using Set to track observers by introducing a wrapper type (thanks @czechboy0 for getting me thinking about how I could make that work!)
Add documentation to public symbols
Add empty default implementations to XCTestObservation methods to line up with their @optional status in Objective-C

Note: Unfortunately I still haven't managed to get a functional Swift 3 dev environment up and running on my machine, so there could be build errors related to that.

In the interest of incremental development and getting more focused code review, I'm thinking it makes the most sense to defer XCTestRun and refactoring the test logging code to separate pull requests.

czechboy0 · 2016-03-14T11:38:35Z

Sources/XCTest/ObjectWrapper.swift

+//
+
+/// A `Hashable` representation of an object and its ObjectIdentifier. This is
+/// useful because Swift classes aren't implicitly hashable based on identity.


Great way of putting it - I wonder whether there'd be value in opening a thread about changing that on Swift evolution. What do you think?

It does seem worth discussing. I was a bit disappointed that I had to jump through such hoops to make Set work out in this situation. I'm not sure how soon I'd have time to do a proper write-up though, so please feel free to open a thread if you feel so inclined!

modocache · 2016-03-15T18:00:03Z

I'm thinking it makes the most sense to defer XCTestRun and refactoring the test logging code to separate pull requests.

Makes sense to me! XCTestRun without XCTestObservation would add an API without any real consumers, after all.

Please remove the "[WIP]", rebase, and ping this thread once you're ready. I also emailed you a Swift 3 toolchain I built a few days ago--maybe that'll work...?

briancroom · 2016-03-16T03:01:50Z

Done!

Thanks for the toolchain help @modocache, it worked great and allowed me to make a minor adjustment that's needed for this to build in the Swift 3 world.

modocache · 2016-03-16T05:17:26Z

Sources/XCTest/XCTestCase.swift

+    /// The name of the test case, consisting of its class name and the method name it will run
+    public let name: String
+
+    public required init(methodName: String) {


Apple XCTest's XCTestCase defines two initializers: -[XCTestCase initWithInvocation:] and -[XCTestCase testCaseWithSelector]. I suppose neither of those could be made available in an environment without the Objective-C runtime. But I feel like we shouldn't provide public API beyond what Apple XCTest defines--that's the stuff evolution proposals are for. (I feel the empty initializer is fine because Apple XCTest also technically allows you to call -[[XCTestCase alloc] init]).

Is there a reason this API needs to be public? It seems we could have methodName be a private attribute that can be set after the XCTestCase instance is initialized. That would avoid the issue of providing an additional public API.

Thanks for pointing this out. When I first saw your comment I was pretty sure I had simply made that constructor public by mistake. Attempting to change it landed me in the middle of an intense battle with the compiler, which ended with me conceding we will have to live with name having a public setter for now (I left comments in the new code pointing this out.) Sadly non-public fields seem to be off-limits for now 😞

I'm reasonably content with the latest version of this, though. What do you think?

(As an aside, while trying to understand the bigger picture here, I learned that it's effectively impossible to provide an overridden constructor on Swift XCTestCase subclasses under Apple XCTest right now, because the framework calls init(invocation:) when building the default test suite, which can't be overridden because of NSInvocation's unavailability.)

modocache · 2016-03-16T05:28:21Z

I had two comments. But boy, I love your tests for this feature! 💯

modocache · 2016-03-16T23:20:43Z

Sources/XCTest/XCTestCase.swift

+    /// The name of the test case, consisting of its class name and the method name it will run.
+    /// - Note: FIXME: This property should be readonly, but currently has to be publicly settable due to a
+    ///         toolchain bug on Linux. To ensure compatibility of tests between
+    ///         swift-corelibs-xctest and Apple XCTest, this property should not be modified.


Oh wow, interesting... It would be great to reference a JIRA issue here. ☝️

modocache · 2016-03-16T23:22:33Z

Awesome work! I'd be happy to merge this if @mike-ferris-apple could ask @swift-ci to please test.

parkera · 2016-03-16T23:36:24Z

@swift-ci please test

XCTestObservation and XCTestObservationCenter

modocache · 2016-03-17T20:31:17Z

Fantastic! API parity with Apple XCTest is so close I can taste it. Thanks, @briancroom and @parkera! 👍

czechboy0 reviewed Mar 10, 2016
View reviewed changes

modocache reviewed Mar 11, 2016
View reviewed changes

briancroom force-pushed the xctestobservation branch 2 times, most recently from abd125a to ebbeb27 Compare March 14, 2016 07:09

czechboy0 reviewed Mar 14, 2016
View reviewed changes

briancroom force-pushed the xctestobservation branch from ebbeb27 to f804cfd Compare March 16, 2016 02:58

briancroom changed the title ~~[WIP] XCTestObservation and XCTestObservationCenter~~ XCTestObservation and XCTestObservationCenter Mar 16, 2016

modocache reviewed Mar 16, 2016
View reviewed changes

briancroom added 2 commits March 16, 2016 22:47

Add name as a property on XCTestCase

500b964

Initial pieces of XCTestObservation and XCTestObservationCenter

2638459

briancroom force-pushed the xctestobservation branch from f804cfd to 2638459 Compare March 16, 2016 21:48

modocache reviewed Mar 16, 2016
View reviewed changes

modocache added a commit that referenced this pull request Mar 17, 2016

Merge pull request #69 from briancroom/xctestobservation

5dce136

XCTestObservation and XCTestObservationCenter

modocache merged commit 5dce136 into swiftlang:master Mar 17, 2016

briancroom deleted the xctestobservation branch March 22, 2016 21:52

modocache mentioned this pull request Mar 30, 2016

[XCTestObservation] Add XCTestSuite announcements #84

Merged

		@@ -92,6 +99,8 @@ extension XCTestCase {

		XCTPrint("Test Case '\(fullName)' started.")

		observationCenter.testCaseWillStart(testCase)

XCTestObservation and XCTestObservationCenter #69

XCTestObservation and XCTestObservationCenter #69

Uh oh!

Conversation

briancroom commented Mar 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

modocache commented Mar 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mike-ferris commented Mar 11, 2016

Uh oh!

briancroom commented Mar 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

modocache commented Mar 15, 2016

Uh oh!

briancroom commented Mar 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

modocache commented Mar 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

modocache commented Mar 16, 2016

Uh oh!

parkera commented Mar 16, 2016

Uh oh!

modocache commented Mar 17, 2016

Uh oh!

Uh oh!