Consolidate AfterFunc and Sleep in tests #3
Comments
rabbbit
added a commit
that referenced
this issue
Oct 4, 2020
We currently have two different implementations. Currently: - they're in the same package - one of them is currently not tested - one has a bug related to slack (see #27, #23) - one has a broken constructor taking options affecting the other type We plan to add at least 3 more implementations: - one that addresses #22 - one that re-uses ratelimitfx atomic style - one that compares us against x/time/rate Even if we decide to eventually keep a single implementation, they'll be there for a while. And I imagine it'll get noisy. (A crazy idea would be to return different implementation based on some user params. But that's totally TBD). This diff tries to organize us a bit my moving the separate implementations to separate packages. Followups: - will try to refactor the tests into a shared test package & re-use them. This way we can write tests once and make sure new implementations are working as expected. - will try to make sure the implementations take equivalent configuration - will have to modify the root ratelimit package to have stronger type guarantees, and not just aliasing methods - that would require a separate /types package though (I think). Accidentally this diff is also changing signature of AfterFunc in internal/clock package. This is so that it implements it's own Clock interface. Looking at #3 though, I think we can simply drop AfterFunc from the interface in followup diffs, and re-use Clock from the root ratelimit package. (We currently have two Clock interfaces defined, we can merge them into a single one using a /types package trick) in a follow up.
rabbbit
added a commit
that referenced
this issue
Oct 19, 2020
Resolves #3 Was looking at the open issues and #3 seemed to make sense - it was also something that I saw in #37 so I thought I'd take a stab. The reasoning is that less code + smaller interface is a good thing. During the diff it turned out that: - our internal/clock has no tests - our internal/clock has at least two bugs: -- one that I'm fixing here, related to not executing two timers if they're scheduled at the "now" boundry -- one regarding adding time but not having a timer set for that specific time (not a problem for us, so not touching it) Looking at the internal/clock, we see: ``` // Forked from github.com/andres-erbsen/clock to isolate a missing nap. ``` There's one extra nap in AfterFunc that I'm removing in this diff, and so I'm hoping that by removing AfterFunc we'll eventually be able to move off the fork. The tests are a bit of a pain, I'll put up another PR for a test runner that will make this a bit easier.
rabbbit
added a commit
that referenced
this issue
Nov 1, 2020
Resolves #3 Was looking at the open issues and #3 seemed to make sense - it was also something that I saw in #37 so I thought I'd take a stab. The reasoning is that less code + smaller interface is a good thing. During the diff it turned out that: - our internal/clock has no tests - our internal/clock has at least two bugs: -- one that I'm fixing here, related to not executing two timers if they're scheduled at the "now" boundry -- one regarding adding time but not having a timer set for that specific time (not a problem for us, so not touching it) Looking at the internal/clock, we see: ``` // Forked from github.com/andres-erbsen/clock to isolate a missing nap. ``` There's one extra nap in AfterFunc that I'm removing in this diff, and so I'm hoping that by removing AfterFunc we'll eventually be able to move off the fork.
rabbbit
added a commit
that referenced
this issue
Nov 1, 2020
Resolves #3 Was looking at the open issues and #3 seemed to make sense - it was also something that I saw in #37 so I thought I'd take a stab. The reasoning is that less code + smaller interface is a good thing. During the diff it turned out that: - our internal/clock has no tests - our internal/clock has at least two bugs: -- one that I'm fixing here, related to not executing two timers if they're scheduled at the "now" boundry -- one regarding adding time but not having a timer set for that specific time (not a problem for us, so not touching it) Looking at the internal/clock, we see: ``` // Forked from github.com/andres-erbsen/clock to isolate a missing nap. ``` There's one extra nap in AfterFunc that I'm removing in this diff, and so I'm hoping that by removing AfterFunc we'll eventually be able to move off the fork.
rabbbit
added a commit
that referenced
this issue
Nov 1, 2020
Resolves #3 Was looking at the open issues and #3 seemed to make sense - it was also something that I saw in #37 so I thought I'd take a stab. The reasoning is that less code + smaller interface is a good thing. During the diff it turned out that: - our internal/clock has no tests - our internal/clock has at least two bugs: -- one that I'm fixing here, related to not executing two timers if they're scheduled at the "now" boundry -- one regarding adding time but not having a timer set for that specific time (not a problem for us, so not touching it) Looking at the internal/clock, we see: ``` // Forked from github.com/andres-erbsen/clock to isolate a missing nap. ``` There's one extra nap in AfterFunc that I'm removing in this diff, and so I'm hoping that by removing AfterFunc we'll eventually be able to move off the fork.
rabbbit
added a commit
that referenced
this issue
Nov 1, 2020
Resolves #3 Was looking at the open issues and #3 seemed to make sense - it was also something that I saw in #37 so I thought I'd take a stab. The reasoning is that less code + smaller interface is a good thing. During the diff it turned out that: - our internal/clock has no tests - our internal/clock has at least two bugs: -- one that I'm fixing here, related to not executing two timers if they're scheduled at the "now" boundry -- one regarding adding time but not having a timer set for that specific time (not a problem for us, so not touching it) Looking at the internal/clock, we see: ``` // Forked from github.com/andres-erbsen/clock to isolate a missing nap. ``` There's one extra nap in AfterFunc that I'm removing in this diff, and so I'm hoping that by removing AfterFunc we'll eventually be able to move off the fork.
rabbbit
added a commit
that referenced
this issue
Nov 10, 2020
Resolves #3 Was looking at the open issues and #3 seemed to make sense - it was also something that I saw in #37 so I thought I'd take a stab. The reasoning is that less code + smaller interface is a good thing. During the diff it turned out that: - our internal/clock has no tests - our internal/clock has at least two bugs: -- one that I'm fixing here, related to not executing two timers if they're scheduled at the "now" boundry -- one regarding adding time but not having a timer set for that specific time (not a problem for us, so not touching it) Looking at the internal/clock, we see: ``` // Forked from github.com/andres-erbsen/clock to isolate a missing nap. ``` There's one extra nap in AfterFunc that I'm removing in this diff, and so I'm hoping that by removing AfterFunc we'll eventually be able to move off the fork.
rabbbit
added a commit
that referenced
this issue
Nov 10, 2020
…39) * Get rid of internal/clock #1 - Consolidate AfterFunc and Sleep in tests Resolves #3 Was looking at the open issues and #3 seemed to make sense - it was also something that I saw in #37 so I thought I'd take a stab. The reasoning is that less code + smaller interface is a good thing. During the diff it turned out that: - our internal/clock has no tests - our internal/clock has at least two bugs: -- one that I'm fixing here, related to not executing two timers if they're scheduled at the "now" boundry -- one regarding adding time but not having a timer set for that specific time (not a problem for us, so not touching it) Looking at the internal/clock, we see: ``` // Forked from github.com/andres-erbsen/clock to isolate a missing nap. ``` There's one extra nap in AfterFunc that I'm removing in this diff, and so I'm hoping that by removing AfterFunc we'll eventually be able to move off the fork.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
It should be sufficient to use Sleep in a goroutine and we should not need a separate AfterFunc.
The text was updated successfully, but these errors were encountered: