-
Notifications
You must be signed in to change notification settings - Fork 17
Fix decoding of MessagePack data, and other fixes to CI #443
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
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
v3 no longer works.
To get rid of a GitHub warning about using `set-output`.
websocket-driver 0.8 changed the format in which it emits binary data [1]: > Emit binary message as a string with Encoding::BINARY instead of an array > Add the option :binary_data_format to force the previous behaviour [1] https://github.com/faye/websocket-driver-ruby/blob/main/CHANGELOG.md#080--2025-05-25
93ecf9b to
77cb55c
Compare
This was changed in Realtime a while ago [1], and we've already had to update other SDKs' test suites to handle it. Also, remove an assertion of an error message, as in 72120de. [1] https://ably-real-time.slack.com/archives/CURL4U2FP/p1733420816469159
I guess they must have changed it on the backend to make the statusCode match the code?
77cb55c to
9db3a61
Compare
This is a partial fix for #444 (nature of error not considered when deciding whether to use a fallback host for realtime connections). Here, I implement just enough of RTN17f to make sure that we don't use a fallback host in response to a token error. This fixes an intermittent failure in the test case on connection_failures_spec.rb:1361, which tests that we correctly handle token errors when resuming a connection per RTN15c5. This test was intermittently failing because it would incorrectly try and use a fallback host after receiving the token error, causing the resume attempt to sometimes fail with "Unable to resume connection from another site". I don't have loads of time on to spend on this issue right now, which is why I'm applying the smallest possible fix. We need to return to #444 and properly consider how to identify the types of errors mentioned in RTN17f.
bf791dd to
dbd47b8
Compare
ttypic
approved these changes
Jun 12, 2025
Contributor
ttypic
left a comment
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.
LGTM
sacOO7
approved these changes
Jun 12, 2025
Contributor
sacOO7
left a comment
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.
lgtm
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes:
websocket-drivergem (we should do a release for this because it may be affecting customers)See commit messages for more details.