-
Notifications
You must be signed in to change notification settings - Fork 25
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
CI: update CocoaPod to v1.0.1 #467
Conversation
LGTM |
Just for the record. This build failed because of:
Nothing related with this PR. Can be merged. |
Worrying that it failed though, but I agree, can be merged. Will wait for @tcard to confirm |
The errors are either the know issue with presence leave data, or something like this: Then it seems that we have some crashes in waitUntil(done) {
channel.attach { error
if error != nil {
fail("error not nil")
done(); return
}
done()
}
}
// if error wasn't nil, we really should stop the test execution here
channel.doSomethingAssumingItsAttached() And the iOS test runner doesn't even let us see the So I would just make sure that the crashing tests pass individually, which I'd bet they do. Arguably it's also not critical right now to make the tests more resilient to crashes. |
Well @tcard I agree we don't need to make them more resilient to crashes, but I do think we need a stable test suite so that the work being done by Prophonix is on a solid base. Therefore if I see a PR which fails I will know that is most likely due to a real failure. @ricardopereira I am happy to merge this, but please raise any issues necessary to describe the CI problems so that we can get these resolved. PS. Presence issues should be fixed on Sandbox now, but I am testing using the Ruby lib to be sure. |
A stable test suite is critical at this point. For example with #444: it has a core change which affects the complete client. When I run the test suite and I see a couple of errors where it's difficult to understand if it's an Ably service error or a code mistake or even a race condition then I'm not comfortable to say it's ready to be merged. I will open an issue with all the frustrations I'm facing and what I do to override them. Could be important to discuss and decide what should be done. |
Thanks |
Running failed tests individually:
|
All of these are about a known issue with presence LEAVE in the backend. |
Sorry, except for the "Invalid clientId for credentials" ones. That might have to do with 0ed5bb9. |
@ricardopereira Anything blocking this? |
@tcard No. Can be merged. |
No description provided.