-
Notifications
You must be signed in to change notification settings - Fork 738
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ability to pause/resume websocket without dumping subscriptions #1335
Changes from 5 commits
c57099e
bb67716
d4fee17
4eb1f2a
10c3524
5f424b4
ab34ab9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -3,6 +3,7 @@ import Apollo | |||||||||
import ApolloTestSupport | ||||||||||
@testable import ApolloWebSocket | ||||||||||
import StarWarsAPI | ||||||||||
import Starscream | ||||||||||
|
||||||||||
class StarWarsSubscriptionTests: XCTestCase { | ||||||||||
let SERVER: String = "ws://localhost:8080/websocket" | ||||||||||
designatednerd marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
|
@@ -12,6 +13,8 @@ class StarWarsSubscriptionTests: XCTestCase { | |||||||||
var webSocketTransport: WebSocketTransport! | ||||||||||
|
||||||||||
var connectionStartedExpectation: XCTestExpectation? | ||||||||||
var disconnectedExpectation: XCTestExpectation? | ||||||||||
var reconnectedExpectation: XCTestExpectation? | ||||||||||
|
||||||||||
override func setUp() { | ||||||||||
super.setUp() | ||||||||||
|
@@ -408,11 +411,85 @@ class StarWarsSubscriptionTests: XCTestCase { | |||||||||
|
||||||||||
waitForExpectations(timeout: 10, handler: nil) | ||||||||||
} | ||||||||||
|
||||||||||
func testPausingAndResumingWebSocketConnection() { | ||||||||||
let subscription = ReviewAddedSubscription() | ||||||||||
let reviewMutation = CreateAwesomeReviewMutation() | ||||||||||
|
||||||||||
// Send the mutations via a separate transport so they can still be sent when the websocket is disconnected | ||||||||||
let httpTransport = HTTPNetworkTransport(url: URL(string: "http://localhost:8080/graphql")!) | ||||||||||
let httpClient = ApolloClient(networkTransport: httpTransport) | ||||||||||
|
||||||||||
func sendReview() { | ||||||||||
let reviewSentExpectation = self.expectation(description: "review sent") | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
httpClient.perform(mutation: reviewMutation) { mutationResult in | ||||||||||
switch mutationResult { | ||||||||||
case .success: | ||||||||||
break | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm how about already here it can fulfill the expectation? Or I'm missing something. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The expectation should be fulfilled and the wait should be ended no matter which case comes in, so that's why it's below the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think XCTFail runs before I guess. Hmm ... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep that's by design - we want that failure to occur before the wait ends. |
||||||||||
case .failure(let error): | ||||||||||
XCTFail("Unexpected error sending review: \(error)") | ||||||||||
} | ||||||||||
|
||||||||||
reviewSentExpectation.fulfill() | ||||||||||
} | ||||||||||
self.wait(for: [reviewSentExpectation], timeout: 10) | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
} | ||||||||||
|
||||||||||
let subscriptionExpectation = self.expectation(description: "Received review") | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
// This should get hit twice - once before we pause the web socket and once after. | ||||||||||
subscriptionExpectation.expectedFulfillmentCount = 2 | ||||||||||
let sub = self.client.subscribe(subscription: subscription) { subscriptionResult in | ||||||||||
designatednerd marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
switch subscriptionResult { | ||||||||||
case .success(let graphQLResult): | ||||||||||
XCTAssertEqual(graphQLResult.data?.reviewAdded?.episode, .jedi) | ||||||||||
subscriptionExpectation.fulfill() | ||||||||||
case .failure(let error): | ||||||||||
if let wsError = error as? Starscream.WSError { | ||||||||||
// This is an expected error on disconnection, ignore it. | ||||||||||
XCTAssertEqual(wsError.code, 1000) | ||||||||||
} else { | ||||||||||
XCTFail("Unexpected error receiving subscription: \(error)") | ||||||||||
subscriptionExpectation.fulfill() | ||||||||||
} | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
self.waitForSubscriptionsToStart() | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
sendReview() | ||||||||||
|
||||||||||
self.disconnectedExpectation = self.expectation(description: "Web socket disconnected") | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
webSocketTransport.pauseWebSocketConnection() | ||||||||||
self.wait(for: [self.disconnectedExpectation!], timeout: 10) | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Do we need to specify exact expectation here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes since that's the only one we want to wait for |
||||||||||
|
||||||||||
// This should not go through since the socket is paused | ||||||||||
sendReview() | ||||||||||
|
||||||||||
self.reconnectedExpectation = self.expectation(description: "Web socket reconnected") | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
webSocketTransport.resumeWebSocketConnection() | ||||||||||
self.wait(for: [self.reconnectedExpectation!], timeout: 10) | ||||||||||
self.waitForSubscriptionsToStart() | ||||||||||
Comment on lines
+469
to
+470
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
|
||||||||||
// Now that we've reconnected, this should go through to the same subscription. | ||||||||||
sendReview() | ||||||||||
|
||||||||||
self.wait(for: [subscriptionExpectation], timeout: 10) | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
|
||||||||||
// Cancel subscription so it doesn't keep receiving from other tests. | ||||||||||
sub.cancel() | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
extension StarWarsSubscriptionTests: WebSocketTransportDelegate { | ||||||||||
|
||||||||||
func webSocketTransportDidConnect(_ webSocketTransport: WebSocketTransport) { | ||||||||||
self.connectionStartedExpectation?.fulfill() | ||||||||||
} | ||||||||||
|
||||||||||
func webSocketTransportDidReconnect(_ webSocketTransport: WebSocketTransport) { | ||||||||||
self.reconnectedExpectation?.fulfill() | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I don't know if there is a style guide, but I usually omit There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is not currently. Personally, I find There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK yep, being consistent is the key here, I agree! |
||||||||||
} | ||||||||||
|
||||||||||
func webSocketTransport(_ webSocketTransport: WebSocketTransport, didDisconnectWithError error: Error?) { | ||||||||||
self.disconnectedExpectation?.fulfill() | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
} | ||||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think just
reconnect
is fine? Aligning it with Starscream. But on other hand addingauto
can be more explicit. :P