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

auth: latest spec additions #45

Merged
merged 21 commits into from
Nov 18, 2015
Merged

auth: latest spec additions #45

merged 21 commits into from
Nov 18, 2015

Conversation

rjeczalik
Copy link
Contributor

Hey,

This CL:

  • fixes build on master (client sends empty clientId due to missing omitempty tag in TokenRequest struct definition)
  • fixes channel name escaping
  • reworks test helpers
  • adds proper ClientID handling for the Auth
  • adds proper AuthURL handling for the POST method
  • test coverage

/cc @mattheworiordan @lmars @kouno

Renames testutil to ablytest and moves all helper types from
helpers_test.go file.
Add 1s margin for times received from Ably servers.

The TokenAuth test sometimes fails with:

--- FAIL: TestAuth_TokenAuth (4.31s)
        auth_test.go:156: want t=2015-10-29 14:58:58.275 +0100 CET to be
within [2015-10-29 14:58:58.560058554 +0100 CET, 2015-10-29
14:58:58.984524906 +0100 CET] time span

Especially on flaky internet connection.
- reduce size of proto.ProtocolMessage from 264B back to 200B
- add ABLY_LOGLEVEL env for testing
- drop Protocol field
- rename UseBinaryProtocol to NoBinaryProtocol (default is to use
  binary)
- minor change: store ClientOptions only once, add log helper to all
  types
@mattheworiordan
Copy link
Member

This looks great @rjeczalik 👍

@rjeczalik
Copy link
Contributor Author

@mattheworiordan @lmars @kouno Ok to merge?

@kouno
Copy link
Contributor

kouno commented Nov 16, 2015

Globally, looks good. 👍

@lmars
Copy link
Member

lmars commented Nov 16, 2015

LGTM

@mattheworiordan
Copy link
Member

I've added some comments @rjeczalik but this looks great

@rjeczalik rjeczalik force-pushed the devel branch 2 times, most recently from 81e987e to c421cbd Compare November 18, 2015 01:27
@rjeczalik
Copy link
Contributor Author

Hey @mattheworiordan @kouno, I've addressed all. Is there's something more to do?

mattheworiordan added a commit that referenced this pull request Nov 18, 2015
auth: latest spec additions
@mattheworiordan mattheworiordan merged commit b35e729 into ably:master Nov 18, 2015
@mattheworiordan
Copy link
Member

Thanks @rjeczalik

@rjeczalik rjeczalik deleted the devel branch November 18, 2015 21:55
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.

4 participants