feat(test): add mock clock API #18
Conversation
|
@Stebalien PTAL |
|
Next step will be to use this mock clock in all the test here that currently do |
|
Yes, let's land this first please. |
|
@marten-seemann @Stebalien It seems running the tests many times ( |
|
Even removing the overwrite check, running two tests in a row (is it really sequential?) still fails locally: So it seems I'm clearly missing something of how multiple runs of same test share context that I'll need to look closer into. |
|
Did current tests pass with |
|
I think anything that is run in the same If this is confirmed then the expectation of #17 (comment) is clearly unmet and we should re-discuss in the issue how is the mock clock going to be used. |
|
Yeah, we're definitely sharing the sweeper across tests. Blocking on #17 (comment). |
|
|
Having race issues with the swapping, will try some more this approach (with more care of stopping goroutines first) and attempt the alternative if I can't make progress here. |
schomatis
force-pushed
the
schomatis/feat/add-mock-clock
branch
from
f04ec7a
to
9725a73
Compare
schomatis
force-pushed
the
schomatis/feat/add-mock-clock
branch
from
9725a73
to
f34d616
Compare
|
Added a stop signal when swapping sweepers. |
There was a problem hiding this comment.
This probably works, but being able to "stop" adds complexity.
You encountered a race because the globalSweeper is by-value. If we make it by-reference, we should be able to replace it without interfering with the previous sweeper (which will die on its own).
| // SetClock puts a global clock in place for testing purposes only. | ||
| // It should be called only once and before using any of the Meter or | ||
| // MeterRegistry APIs of this package and should be restored with | ||
| // RestoreClock after the test. | ||
| func SetClock(clock clock.Clock) { | ||
| globalSweeper.stop() | ||
| globalSweeper = sweeper{} | ||
| globalSweeper.clock = clock | ||
| } | ||
|
|
||
| func RestoreClock() { | ||
| globalSweeper.stop() | ||
| globalSweeper = sweeper{} | ||
| } |
There was a problem hiding this comment.
These are testing functions and don't need to be exported.
There was a problem hiding this comment.
The idea would be to use this in libp2p/go-libp2p-core#267 so I think we still want to export this, or something similar to it.
| mockClock.Add(40 * time.Millisecond) | ||
| } | ||
| actual := m.Snapshot() | ||
| checkApproxEq(t, actual.Rate, 25000, 100) |
There was a problem hiding this comment.
I think I've said this before, but there's no need for approximations. We're using a mock clock, so timing is precise, and so should be the measurement.
|
I'm getting the impression that this PR is moving in the wrong direction. |
Co-authored-by: Steven Allen <steven@stebalien.com>
That makes sense. |
@marten-seemann Your call. This PR is already passing and I'm aligned with Steven's input, so I'm not sure which wrong direction has this taken. The error margin comment is valid but it's not central to the mock clock implementation (but how we leverage it later), so I prioritized fixing the data race which was the blocker here. |
Went with a more specialized
UseMockClockAPI to clearly signal this is for tests and not something you normally mess with but can revert to the more generalSetClockif preferred.Closes #17.