Be more careful about threading and notifications

[johnmckerrell]

Make sure that Nimble doesn't try to access notification properties from the main
thread as they may have been created on another thread and get upset
(most likely when Core Data is involved).
Fixes #973

• Does this have tests?
• Does this have documentation?
• Does this break the public API (Requires major version bump)?
• Is this a new feature (Requires minor version bump)?

[younata]

I'd like to get this in the next release. Approved running the tests here.

[younata]

Thanks for your contribution!

Holding onto a stringified version of the notification, but still holding onto the notification itself for later comparison with the predicate feels like it's not the right solution here, as tests would still fail upon comparison time. I'd love it if we could do all of the comparisons necessary in the thread that actually handles and receives the notification. There's not really a good way to do this with the current signature of the postNotifications matcher. If predicate was instead [Predicate<Notification>] instead of Predicate<[Notification]>, then, when a notification is received, you could pop off a predicate, compare it, and store the result of that.

I'm curious if anyone else @Quick/core has ideas here?

[younata]

This isn't doing what you think it's doing, and it's resulting in a test that's passing, but not asserting what you're intending it to assert. (The code is still doing the right thing, but it doesn't actually have a test guarding it).

This isn't doing any waiting whatsoever for the notification to actually be posted before the predicate of the matcher runs. Because this fires off an asynchronous block on a background thread, with no waiting for that block to finish, the predicate runs, sees that no notifications have been posted, and passes.

To fix this, I would use an OperationQueue, and use the AddOperations(_:waitUntilFinished:) method. This actually runs the operations and waits for them to finish before continuing. You could also use DispatchQueue.asyncAndWait(execute:) with an appropriate DispatchWorkItem, but that API isn't available in the versions of iOS/macOS/tvOS Nimble is targeting.

This also shouldn't use eventually as that results in the notification being posted many times if the initial predicate doesn't match.

Suggested change

	func testPassesWhenNotificationIsPostedFromADifferentThreadAndToEventuallyNotCalled() {
	let n1 = Notification(name: Notification.Name("Foo"), object: nil)
	expect {
	DispatchQueue.global(qos: .userInitiated).async {
	let backgroundThreadObject = BackgroundThreadObject()
	let n2 = Notification(name: Notification.Name(n1.name.rawValue + "a"), object: backgroundThreadObject)
	self.notificationCenter.post(n2)
	}
	}.toEventuallyNot(postNotifications(equal([n1]), from: notificationCenter))
	}
	func testPassesWhenNotificationIsPostedFromADifferentThreadAndToNotCalled() {
	let n1 = Notification(name: Notification.Name("Foo"), object: nil)
	expect {
	OperationQueue().addOperations([BlockOperation {
	let backgroundThreadObject = BackgroundThreadObject()
	let n2 = Notification(name: Notification.Name(n1.name.rawValue + "a"), object: backgroundThreadObject)
	self.notificationCenter.post(n2)
	}], waitUntilFinished: true)
	}.toNot(postNotifications(equal([n1]), from: notificationCenter))
	}

[younata]

This needs some more work and thought on it, and I don't want to gate v10 on it. It's definitely valuable work, but it can also wait until v11.

[johnmckerrell]

Thanks for the feedback (wow, 13 days ago), I certainly don't want to dump a PR and run away but it will probably be a little while longer before I can make any further changes FYI.

[younata]

Bumping this to the next (major?) release in favor of getting async/await support out the door sooner.

[younata]

Hi John! After a bunch of noodling on this, I forked your fork and made a PR that includes these changes + some feedback: #1051

This will get merged shortly. Thanks for being patient!