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.

Sign up for GitHub

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

Jump to bottom

Rsc7b #464

Merged
merged 6 commits into from
Aug 25, 2016
Merged

Rsc7b #464

merged 6 commits into from
Aug 25, 2016

Conversation

EvgenyKarkan
Copy link
Contributor

No description provided.

mattheworiordan
mattheworiordan reviewed Aug 16, 2016
View reviewed changes
Spec/RestClient.swift
expect(error).to(beNil())
let headerLibVersion = testHTTPExecutor.requests.first!.allHTTPHeaderFields?["X-Ably-Lib"]
let ablyBundleLibVersion = ARTDefault.libraryVersion()
expect(headerLibVersion).to(equal(ablyBundleLibVersion))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whilst I understand that you don't necessarily want the the test to change for every time a version changes, it does feel like this test needs to be a bit more verbose i.e. can it not at least check that the header matches ios-0.8.* for example? It just means the test confirms what we expect as opposed to simply referencing another part of the internals of the lib.

@mattheworiordan
Copy link
Member

Thansk, minor comments otherwise good, 👍

@EvgenyKarkan
Copy link
Contributor Author

Thanks for feedback, I'll try to improve it.

@EvgenyKarkan
Copy link
Contributor Author

Hi @mattheworiordan, I've added commit, please find time to review it.

@mattheworiordan
Copy link
Member

Thanks, 👍

@EvgenyKarkan @mattheworiordan
Rtn2g (ably#465)
9e23acd 
* RSC7b

* RTN2g

* Minor cleanups.

* Lib param expected value as explicitly string pattern.

* Bump lib bundle version, minor fix to expect its value in test.
@mattheworiordan
Copy link
Member

Can this PR please be rebased onto master as I cannot merge automatically?

@EvgenyKarkan
Copy link
Contributor Author

Morning @mattheworiordan , no more conflicts here.

@mattheworiordan
Copy link
Member

Thanks @EvgenyKarkan

@mattheworiordan mattheworiordan merged commit d3e4916 into ably:master Aug 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@EvgenyKarkan @mattheworiordan