Skip to content
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

RTN17b #385

Merged
merged 12 commits into from
May 4, 2016
Merged

RTN17b #385

merged 12 commits into from
May 4, 2016

Conversation

ricardopereira
Copy link
Contributor

No description provided.

@ricardopereira ricardopereira force-pushed the rtn17b branch 2 times, most recently from a6aeb04 to 7cd2470 Compare April 14, 2016 07:34
@ricardopereira ricardopereira force-pushed the rtn17b branch 2 times, most recently from 4084f7f to 41f5f93 Compare April 22, 2016 12:15
@tcard
Copy link
Contributor

tcard commented Apr 22, 2016

PTAL.

I had to switch from SwiftWebSocket to PocketSocket to be able to inspect which kind of error the websocket connection is failing with. SwiftWebSocket uses a Swift enum for this, which can't be used from Objective-C.

Originally I tried to add some Swift to our Objective-C codebase to do the bridging, but Xcode gave me a extremely hard time with this, so I gave up. Forking SwiftWebSocket to add Objective-C support is another possibility, but that would've required fighting against Swift's way of doing things from inside a Swift project, so I decided to just use a Objective-C library.

@ricardopereira
Copy link
Contributor Author

LGTM but it's not passing.

@tcard
Copy link
Contributor

tcard commented Apr 26, 2016

@ricardopereira Not yet. The switch to PocketSocket unveiled a few things that are wrong that were working basically by chance. We need some of the changes in ably/docs#112 for this, will do that first.

@tcard tcard force-pushed the rtn17b branch 2 times, most recently from 708a69f to c5845c7 Compare April 27, 2016 15:21
@mattheworiordan
Copy link
Member

What's the status on this?

@tcard
Copy link
Contributor

tcard commented Apr 28, 2016

@mattheworiordan I'm about to try to reproduce why the Travis build stalls with this.

@tcard tcard force-pushed the rtn17b branch 5 times, most recently from f06dd4b to f1baf62 Compare April 29, 2016 19:26
@tcard tcard mentioned this pull request May 2, 2016
tcard added 3 commits May 2, 2016 15:05
I had to switch from SwiftWebSocket to PocketSocket to be able to
inspect which kind of error the websocket connection is failing with.
SwiftWebSocket uses a Swift enum for this, which can't be used from
Objective-C.
@tcard tcard force-pushed the rtn17b branch 2 times, most recently from 735fd24 to 6cda49e Compare May 3, 2016 10:55
@tcard
Copy link
Contributor

tcard commented May 3, 2016

Sitrep: this might be the worst case of Works On My Machine™ I've encountered so far. The whole test suite works on my machine. I've already implemented #387, #388, #389 and #390 on top of this and they also work. I've implemented MessagePack encoding on top of this: also works. So the only thing missing now is pretty much to have this stop crashing on Travis.

Since there doesn't seem to be a way of looking into stdout from Travis, I've tried to get some insight into what's going on by capturing every single line of logging, sending it in a HTTP request to a server, and writing them there. But I can't spot anything wrong from the logs; it just seems to stall at some random point.

I'll try to add some more external monitoring. Any idea would be appreciated.

@mattheworiordan
Copy link
Member

Is this since PocketRocket change?

@ricardopereira
Copy link
Contributor Author

@tcard Change the .travis.xml and run the test suite with standard xcodebuild and see what happens on the raw log.

@ricardopereira
Copy link
Contributor Author

Something like: xcodebuild -workspace './Ably.xcworkspace' -scheme 'Ably' -destination 'platform=iOS Simulator,OS=latest,name=iPhone 5s' build test

@tcard
Copy link
Contributor

tcard commented May 4, 2016

@mattheworiordan Yes, although it was still happening with SocketRocket. I need one or the other to implement this spec line, as explained above.

Now it works. There's only one test failing, unrelated to this PR (I'll fix it separately). I don't know what changed, but it's passed consistently a few times now. Maybe PocketSocket was broken, and since the switch to SocketRocket it was just a coincidence with some other condition? Anyway, it's so annoying that it's so hard to get better diagnostics.

Please confirm I can merge this.

@mattheworiordan
Copy link
Member

Please confirm I can merge this.

👍

@tcard tcard merged commit 3a0048a into master May 4, 2016
@tcard tcard deleted the rtn17b branch May 4, 2016 11:00
tcard added a commit that referenced this pull request May 12, 2016
I've tested the ARTOSReachability "live" manually (by switching on/off
the real Internet connection on my machine) and it works as expected,
but it needs #385 to properly handle transport errors.
tcard pushed a commit that referenced this pull request May 16, 2016
* Test suite: removed Custom case from NetworkAnswer

- to be used with both TestProxyHTTPExecutor and TestProxyTransport

* Test suite: replaceMethod

* TestProxyTransport can now mock network answers

* RTN17b

* RTN17b: pending

* Replace SwiftWebSocket -> PocketRocket.

* Fix RTN17b.

I had to switch from SwiftWebSocket to PocketSocket to be able to
inspect which kind of error the websocket connection is failing with.
SwiftWebSocket uses a Swift enum for this, which can't be used from
Objective-C.

* Use upstream PocketSocket.

* Small fix in test.

* Switch back to SocketRocket.

PocketSocket seems to be kind of buggy.

* Queue messages when CONNECTED but renewing token.

* Logging: always log memory address of current ARTRealtime instance.
tcard added a commit that referenced this pull request Aug 26, 2016
I've tested the ARTOSReachability "live" manually (by switching on/off
the real Internet connection on my machine) and it works as expected,
but it needs #385 to properly handle transport errors.
tcard added a commit that referenced this pull request Aug 26, 2016
I've tested the ARTOSReachability "live" manually (by switching on/off
the real Internet connection on my machine) and it works as expected,
but it needs #385 to properly handle transport errors.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants