Add a test runner

[rabbbit]

Something that came up in #39 - there's a fair bit of duplication
in the tests. A runner make current setup a bit easier/shorter,
but would make #39 much smaller.

There's no functional changes.

[cinchurge]

a test function being able to modify the state of the runner directly feels a little odd. would it be a better idea to pass in a runner interface instead? exposing the waitgroup directly allows for crazy things like calling wg.Wait inside the test function which can cause a deadlock. not that we expect people to do that, but it's better to reduce the API surface to avoid accidental problems

[rabbbit]

Hm.

Agree that having a smaller surface is a good thing, but not sure if this won't over-complicate things here. Like,

• there are literally two tests (one of which tests nothing, I think)
• we're unlikely to have more tests
• if we cause a deadlock than it's on us ;)

Maybe I should have called the runner just args - would have been closer to my goal of just reducing the noise.

If I pass in the interface I also have to take away access to r.count, r.doneCh (test below) and I'll need to pass in a function as an argument to run (since TestDelayedRateLimiter) is doing some custom work below.

Anyway, can take a stab, why not.

[rabbbit]

Okay, so if we passed in the interface we kinda need more methods:

type runner interface {
	// startTaking tries to Take() on passed in limiters in a loop/goroutine.
	startTaking(rls ...ratelimit.Limiter)
	// assertCountAt asserts the limiters have Taken() a number of times at the given time.
	// It's a thin wrapper around afterFunc to reduce boilerplate code.
	assertCountAt(d time.Duration, count int)
	// getClock returns the test clock.
	getClock() ratelimit.Clock
	// afterFunc executes a func at a given time.
	afterFunc(d time.Duration, fn func())
}

It's a bit verbose ... but I'm starting to like it - the actual test code looks quite clear.

[rabbbit]

I'm not sure whether we'll need afterFunc once we fix TestDelayedRateLimiter

But for now it should stay here.

[cinchurge]

I like the idea, but we should probably do some encapsulation of the runner to reduce potential problems

[cinchurge]

do we need to add this to the waitgroup?

[rabbbit]

So, technically, I think we should, but it's not needed right now - clock.afterFunc does time.sleep(1*millisecond) so we get it for free.

Given that it works now, and the old code was doing this, I'd prefer to keep it as is - just to minimize changes in this PR.

#39 actually adds a separate wg for starting jobs (it's out of date now, I'll update it after this PR) - I'd prefer to iterate & update it in follow ups.

But yeah, more wgs will be needed if we don't want to rely on sleeps (and to deprecate the internal clock fork), good observation :)

[cinchurge]

lgtm!