jonboulle / clockwork

a fake clock for golang
Apache License 2.0
625 stars 57 forks source link

Move lock logic to fakeClock. Reduce Ticker and Timer code complexity. #52

Closed DPJacques closed 1 year ago

DPJacques commented 1 year ago

Tested: All tests pass, no race conditions from go -race.

DPJacques commented 1 year ago

I think this is going to fail until #51 is merged.

I recommend delaying this review until #51 is merged and I can re-run integration.

Update: It seems this is angry at older versions of Go. I'll get this sorted when #51 is merged.

sagikazarmark commented 1 year ago

I don't mind dropping Go versions from the matrix if that's necessary to keep up with changes. If it's easily avoidable (eg. using build tags), good, if not, supporting the last 2-3 Go versions should generally be enough.

Just make sure it's reflected in the go.mod file as well.

DPJacques commented 1 year ago

Ok, this should be ready to go for go version 1.15 and up.

I used squash to add the changes to go.mod, please let me know if that is the preferred way to do it. I noticed the top line of the commits were the same in both cases, which was somewhat confusing. Maybe you have a way to squash them all on your end?

Let me know if you need anything from me to keep the commit history tidy.

I don't know why it shows the integration tests for 1.16 were cancelled, maybe because the earlier ones failed? Either way, I see 1.15, 1.17, and 1.18 succeeded.

sagikazarmark commented 1 year ago

I don't know why it shows the integration tests for 1.16 were cancelled, maybe because the earlier ones failed? Either way, I see 1.15, 1.17, and 1.18 succeeded.

I updated the CI config on master. Can you please rebase your PR, so CI gets triggered for the supported Go versions only?

And yes, they got cancelled because failing fast is enabled by default on GitHub Actions. Fixing that in the next patch.

DPJacques commented 1 year ago

Done. Thanks for updating the CI settings!