RTL8 #219
RTL8 #219
Conversation
| channel1.publish(nil, data: "message") | ||
| channel1.publish(nil, data: "message") | ||
|
|
||
| expect(Test.counter).toEventually(equal(3), timeout: testTimeout) |
There was a problem hiding this comment.
I really don't like the readability of the test. Why not simply set up one channel, subscribe, publish and check that message is received. Then unsubscribe and check that no message is received? This is overly complex and I don't really follow why one channel unsubscribes whilst the other one doesn't.
There was a problem hiding this comment.
Yes, you right. I will simplify.
|
|
||
| // If `unsubscribe` fails then the test suite will raise "Done closure's was called multiple times." | ||
| waitUntil(timeout: testTimeout) { done in | ||
| delay(5.0) { done() } |
There was a problem hiding this comment.
Could you please make this delay much smaller and move it to the callback of the second channel.publish? The test suite is already way too slow as it is...
|
LGTM |
| waitUntil(timeout: testTimeout) { done in | ||
| channel.publish(nil, data: "message") { errorInfo in | ||
| expect(errorInfo).to(beNil()) | ||
| // If `unsubscribe` fails then the test suite will raise "Done closure's was called multiple times." |
There was a problem hiding this comment.
This test is unnecessarily complex because of this. Why are we testing publish on line 628 at all. Surely we just add a listener, and the unsubscribe the listener. We have other tests to test that the listener registration works, so if we don't test something that this test is not about, you can instead raise an error in the listener block because we never expect it to be called.
No description