-
Notifications
You must be signed in to change notification settings - Fork 26
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
Incremental backoff and jitter #1520
Conversation
d5930a6
to
467b893
Compare
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.
The implementation LGTM, although I don't know much objective-c so it's hard to say for sure.
A couple of things to mention:
- There are some newly skipped tests here which shouldn't be the case - is there a particular reason why these tests can't be fixed?
- It might be worth considering using a seed for random number generation in the tests here to make retry intervals deterministic (this wasn't possible in ably-js because
Math.random()
doesn't take a seed argument).
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.
Will review the content of the PR next, but re the skipped tests, I think ideally we should investigate and fix this here, and not leave it for #1526 and #1527. The newly-skipped tests are tests that were passing before, and have only started to fail after we added these new tests. Is there something that makes you think that the problem lies with the existing tests, and not the new ones that we've added?
I guess most of all other failing/flaky tests were reported like that, but I agree we should stop this practice and try to fix it in place.
|
Are the tests that you've marked as skipped failing consistently after adding the new tests? |
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 need to dive into this properly and understand the tests, but here are some initial thoughts from looking over the code.
I just tried (locally) un-skipping the tests mentioned in #1526 and #1527, and I didn't see them failing. But what I did see was some other failures:
|
64e585c
to
409f100
Compare
409f100
to
64e585c
Compare
a516d9d
to
ea0d5f1
Compare
1d72363
to
7665b51
Compare
64e585c
to
259c9c4
Compare
TODO explain why
Co-authored-by: Lawrence Forooghian <lawrence@forooghian.com>
This is no longer being used as of 6fdf573.
259c9c4
to
d6c94a9
Compare
Closes #1431
There is an unresolved conversation in the reference implementation for channel - ably/ably-js#1008 (comment) I've decided to take
state
check from Owen's implementation.