[XCTestExpectation] Adding expectationForNotification #85

kylesyoon · 2016-03-27T20:36:10Z

What's in this pull request?

This pull request adds the XCTestCase convenience constructor for XCTestExpectation expectationForNotification:object:handler:.

It also includes the XCNotificationExpectationHandler typealias file as well as tests to check expectationForNotification:object:handler: behavior.

https://bugs.swift.org/browse/SR-907
However, this pull request does not include the convenience constructor expectationForPredicate:evaluatedWithObject:handler: described in the ticket, as NSPredicate initializers have yet to be implemented.

Why merge this pull request?

This will provide a full Swift implementation of expectationForNotification:object:handler: that allows asynchronous testing for NSNotification.

jacobvanorder · 2016-03-28T18:16:33Z

Tests/Functional/Asynchronous/Expectations/main.swift

+// CHECK: .*/Tests/Functional/Asynchronous/Expectations/main.swift:125: error: ExpectationsTestCase.test_observeNotificationWithIncorrectObject_fails : Asynchronous wait failed - Exceeded timeout of 0.1 seconds, with unfulfilled expectations: Expect notification 'notificationWithIncorrectObjectTest' from dummyObject
+// CHECK: Test Case 'ExpectationsTestCase.test_observeNotificationWithIncorrectObject_fails' failed \(\d+\.\d+ seconds\).
+    func test_observeNotificationWithIncorrectObject_fails() {
+        let notificationNamme = "notificationWithIncorrectObjectTest"


“notificationName”

😂 You got me
🙏 Namaste

modocache · 2016-03-28T18:45:34Z

This is awesome, thanks @yoonapps!! 💯 I have a few nitpicks, but other than those this looks great to me!

modocache · 2016-03-28T18:47:17Z

Sources/XCTest/XCTestCase.swift

+            objectDescription = "\(object)"
+        } else {
+            objectDescription = "any object"
+        }


nit-pick: Perhaps use a ternary operator here?

let objectDescription = object == nil ? "any object" : "\(object)"

I don't have a strong opinion either way, but a ternary operator would only take one line, whereas this approach uses six.

I was thinking about the ternary operator but if I don't unwrap the optional, then the object description comes with Optional(description). This led to using the if statement. I can change it to the ternary operator as long as it's cool to do "\(object!)" (force unwrap).

Or is it better to print in the description that the argument is an optional? Maybe it's something to consider since the description is following the Obj C style.

Oh, good point! Perhaps this, then?

let objectDescription = object == nil ? "any object" : "\(object!)"

Or we could leave this as-is. Again, I don't have a strong opinion here. :)

modocache · 2016-03-28T18:58:03Z

@yoonapps This is fantastic! 😍 Could you squash these three commits into one?

@mike-ferris-apple Could you ask @swift-ci to please test?

kylesyoon · 2016-03-28T19:01:38Z

Awesome, yes I will address these reviews and squash the commits this evening! Thanks for the reviews.

modocache · 2016-03-28T19:04:20Z

Sources/XCTest/XCTestCase.swift

+    ///   allows a nil notification name for its criteria of observing
+    ///   notifications, the Objective-C implementation of this function 
+    ///   requires a notification name.
+    public func expectationForNotification(notificationName: String, object: AnyObject?, handler: XCNotificationExpectationHandler?) -> XCTestExpectation {


Super minor nit-pick: Technically the Apple XCTest signature for this method is:

public func expectationForNotification(notificationName: String, object objectToObserve: AnyObject?, handler: XCNotificationExpectationHandler?) -> XCTestExpectation

I don't think the parameter name is important, but I'd personally prefer to be 100% consistent. 😛

mike-ferris · 2016-03-28T19:19:51Z

@swift-ci please test

kylesyoon · 2016-03-28T20:51:12Z

@modocache addressed issues

modocache · 2016-03-28T21:30:47Z

Tests/Functional/Asynchronous/Notifications/Expectations/main.swift

+XCTMain([testCase(NotificationExpectationsTestCase.allTests)])
+
+// CHECK: Executed 5 tests, with 2 failures \(0 unexpected\) in \d+\.\d+ \(\d+\.\d+\) seconds
+// CHECK: Total executed 5 tests, with 2 failures \(0 unexpected\) in \d+\.\d+ \(\d+\.\d+\) seconds


nit-pick: Add a new line at the end of this file so Git won't complain 😛

kylesyoon · 2016-03-29T00:25:02Z

@modocache ~~done~~ ✔️ hmm actually not

kylesyoon · 2016-03-29T00:33:23Z

Ok well Github is not showing the extra line... but there is one 😕

modocache · 2016-03-29T02:55:00Z

Yup, it's there! If no new line is present at the bottom of a file GitHub shows a symbol that looks like 🚫.

Sorry @mike-ferris-apple, but could you ask @swift-ci to please test again? I think the updates to the pull request cancelled the previous CI job.

If CI passes on Linux (OS X is still broken, see #77), then I'll merge. 😄

mike-ferris · 2016-03-29T14:27:47Z

@swift-ci please test

kylesyoon · 2016-03-29T15:55:01Z

😖
@modocache The SwiftXCTestFunctionalTests build is passing for me locally. Although, I've seen the test fail once, and it was because the test expected // CHECK: in the new handler test main.swift to be the // CHECK: in the existing handler test main.swift. But I ran the test again and it was fine. Do you think this could be a file path issue?

// RUN: %{swiftc} %s -o %{built_tests_dir}/Handler
// RUN: %{built_tests_dir}/Handler > %t || true
// RUN: %{xctest_checker} %t %s

modocache · 2016-03-29T21:38:13Z

@yoonapps Nice catch!! It most certainly is a file path issue, one I've seen before--it's pretty nasty, but we need to use a unique file name for each test. You should be fine if you "namespace" your test file name, like so:

// RUN: %{swiftc} %s -o %{built_tests_dir}/Aynschronous-Notifications-Handler
// RUN: %{built_tests_dir}/Aynschronous-Notifications-Handler > %t || true
// RUN: %{xctest_checker} %t %s

Sorry about that. I'll create a JIRA issue so we can track fixing this.

kylesyoon · 2016-03-29T22:27:19Z

@modocache Ok! I updated the paths for both the Notification Expectations and Handler main.swift files. Please work! 😭

modocache · 2016-03-29T22:29:16Z

@mike-ferris-apple Mind asking @swift-ci to please test again? 😅

I wonder when CI will be opened up to all committers... 💭

kylesyoon · 2016-03-29T22:29:42Z

Sorry!

modocache · 2016-03-29T22:32:03Z

Haha, don't be! I'm actually fine with merging this without testing again, but better safe than sorry.

- Prints the same description as Obj C - Uses the default notification center to observe for notifications - Optional handler returns true or false of whether the expectation was fulfilled - Tests check comparing the notification and object arguments for the expectation and notification - Tests check returning true or false in handler

parkera · 2016-03-30T00:40:56Z

@swift-ci please test

modocache · 2016-03-30T03:52:06Z

Awesome, thanks @yoonapps! Congratulations on completing your first starter task! 💐

kylesyoon · 2016-03-30T03:57:56Z

AWWWW YEEEEE
@modocache Thank you so much for all your help! Can't wait to do more. 🔥

jacobvanorder reviewed Mar 28, 2016
View reviewed changes

modocache reviewed Mar 28, 2016
View reviewed changes

kylesyoon force-pushed the master branch 2 times, most recently from c70c5f4 to adae1b2 Compare March 28, 2016 20:39

modocache reviewed Mar 28, 2016
View reviewed changes

kylesyoon force-pushed the master branch from adae1b2 to cdc5b01 Compare March 29, 2016 00:24

kylesyoon force-pushed the master branch from cdc5b01 to 42c2101 Compare March 29, 2016 00:29

kylesyoon force-pushed the master branch from 42c2101 to de1ed00 Compare March 29, 2016 22:24

kylesyoon force-pushed the master branch from de1ed00 to c2a9258 Compare March 29, 2016 22:33

modocache merged commit db3643a into apple:master Mar 30, 2016

swift-ci mentioned this pull request Mar 27, 2016

[SR-907] Add convenience XCTestExpectation constructors for NSNotification #402

Closed

modocache mannequin mentioned this pull request May 9, 2022

[SR-1110] Add convenience XCTestExpectation constructors for NSPredicate #396

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[XCTestExpectation] Adding expectationForNotification #85

[XCTestExpectation] Adding expectationForNotification #85

kylesyoon commented Mar 27, 2016

jacobvanorder Mar 28, 2016

jacobvanorder Mar 28, 2016

kylesyoon Mar 28, 2016

modocache commented Mar 28, 2016

modocache Mar 28, 2016

kylesyoon Mar 28, 2016

kylesyoon Mar 28, 2016

modocache Mar 28, 2016

modocache commented Mar 28, 2016

kylesyoon commented Mar 28, 2016

modocache Mar 28, 2016

mike-ferris commented Mar 28, 2016

kylesyoon commented Mar 28, 2016

modocache Mar 28, 2016

kylesyoon commented Mar 29, 2016

kylesyoon commented Mar 29, 2016

modocache commented Mar 29, 2016

mike-ferris commented Mar 29, 2016

kylesyoon commented Mar 29, 2016

modocache commented Mar 29, 2016

kylesyoon commented Mar 29, 2016

modocache commented Mar 29, 2016

kylesyoon commented Mar 29, 2016

modocache commented Mar 29, 2016

parkera commented Mar 30, 2016

modocache commented Mar 30, 2016

kylesyoon commented Mar 30, 2016

[XCTestExpectation] Adding expectationForNotification #85

[XCTestExpectation] Adding expectationForNotification #85

Conversation

kylesyoon commented Mar 27, 2016

What's in this pull request?

Why merge this pull request?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modocache commented Mar 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modocache commented Mar 28, 2016

kylesyoon commented Mar 28, 2016

Choose a reason for hiding this comment

mike-ferris commented Mar 28, 2016

kylesyoon commented Mar 28, 2016

Choose a reason for hiding this comment

kylesyoon commented Mar 29, 2016

kylesyoon commented Mar 29, 2016

modocache commented Mar 29, 2016

mike-ferris commented Mar 29, 2016

kylesyoon commented Mar 29, 2016

modocache commented Mar 29, 2016

kylesyoon commented Mar 29, 2016

modocache commented Mar 29, 2016

kylesyoon commented Mar 29, 2016

modocache commented Mar 29, 2016

parkera commented Mar 30, 2016

modocache commented Mar 30, 2016

kylesyoon commented Mar 30, 2016