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

rsa10k cache server time offset #95

Merged
merged 6 commits into from
Oct 18, 2018
Merged

Conversation

gernest
Copy link
Contributor

@gernest gernest commented Oct 3, 2018

No description provided.

@gernest gernest changed the title [WIP] rsa10k cache server time offset rsa10k cache server time offset Oct 4, 2018
@gernest gernest requested a review from ORBAT October 4, 2018 10:40
ably/auth.go Outdated
//Timestamp returns the timestamp to be used in authorization request.
func (a *Auth) Timestamp(query bool) (time.Time, error) {
var now time.Time
if a.Now != nil {
Copy link
Member

Choose a reason for hiding this comment

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

I'm trying to understand this. It looks to me like you've declared Auth:Now(). So what does this test do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@paddybyers Now is a helper in case the user wants to supply the time function. The reason behind this is to allow me to write better test coverage of the caching feature. This function allows us to set default values in case Now is not supplied. Please see the tests, I think I wouldn't have been able to make them that way without this.

I didn't want to use real clock, also I was avoiding client.Time() call because that is testsed somewhere else so not relevant here. The most important bit is caching and calculating time based on cached duration.

Copy link
Member

Choose a reason for hiding this comment

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

Now is a helper in case the user wants to supply the time function

Isn't there a way to add that without making it part of the public API?

Copy link
Member

@paddybyers paddybyers left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@gernest gernest merged commit 0ef09ca into ably:integration-spec-1.1 Oct 18, 2018
@gernest gernest deleted the rsa10k branch October 18, 2018 08:35
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.

2 participants