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

Add a Clock interface for testing #18

Merged
merged 1 commit into from
May 23, 2017
Merged

Add a Clock interface for testing #18

merged 1 commit into from
May 23, 2017

Conversation

thockin
Copy link
Contributor

@thockin thockin commented May 21, 2017

This adds an interface that is used internally to allow a caller to
intercept calls to time.Now() and time.Sleep(), and do better testing.

Fixes #17

@howbazaar this is about as simple as it gets. Unfortunately the factory functions are a bit tedious. Could be changed to "WithDeps" (or something like that) and have a Deps struct, but for now that is going to hold exactly 1 field. This code is simple enough that it probably won't accumulate more. Your call.

This adds an interface that is used internally to allow a caller to
intercept calls to time.Now() and time.Sleep(), and do better testing.
Copy link
Contributor

@howbazaar howbazaar left a comment

Choose a reason for hiding this comment

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

Ideally we'd have the NewBucket function take a config struct, but we probably don't want to change the signature of that right now.

I would so write this package differently, but given what we have, and given minimal changes to the existing public functions, I think this is fine.

@@ -33,12 +34,37 @@ type Bucket struct {
availTick int64
}

// Clock is used to inject testable fakes.
type Clock interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... this is the first time that I have looked at this code in detail.

The Clock interface that Juju uses elsewhere is a subset of this one:
https://github.com/juju/utils/blob/master/clock/clock.go

So, instead of Sleep, we generally use After. Mostly because we have selects with cancellation channels.

@howbazaar
Copy link
Contributor

$$merge$$

@wojtek-t
Copy link

@howbazaar - when can we expect this PR to be merged?

@axw axw merged commit 5b9ff86 into juju:master May 23, 2017
@axw
Copy link
Contributor

axw commented May 23, 2017

@wojtek-t sorry for the delay. Thanks @thockin.

@wojtek-t
Copy link

@axw - thanks a lot for very quick response. And I definitely wouldn't consider "24 hours" as a delay :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants