Add NewTicker to the Clock Interface

[moisesvega]

Changes in this diff:

• Move Clock interface to zapcore and re-exported from the top-level zap package
• Add NewTicker method as part of Clock Interface

With this changes Clock interface will be available inside of zapcore and the NewTicker method will allow us to control time.Ticker when it is necessary.

This is a stacked PR to addressed the comments on #782 . Next PR will be doing a refactor of WriteSyncer.

[codecov]

Codecov Report

Merging #948 (0839a6b) into master (3748251) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #948   +/-   ##
=======================================
  Coverage   98.02%   98.02%           
=======================================
  Files          44       44           
  Lines        1974     1975    +1     
=======================================
+ Hits         1935     1936    +1     
  Misses         30       30           
  Partials        9        9           

Impacted Files	Coverage Δ
clock.go	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3748251...0839a6b. Read the comment docs.

[abhinav]

Behaviorally, I would assume that a constantClock will never advance the ticker since time never moves.

[abhinav]

This works but we can shorten this by embedding clock.Mock and only overriding NewTicker:

type controlledClock struct{
  *clock.Mock
}

func newControlledClock() *controlledClock

func (*controlledClock) NewTicker(...) *time.Ticker

[abhinav]

This is a goroutine leak. If we use go.uber.org/goleak here (which we do in #782).
We need to create a second quit channel that this selects on.

for {
  select {
  case <-quit:
    ticker.Stop()
    return
    
  case <-ticker.C:
    // ...
  }
}

This might also have to use a second channel to indicate that the goroutine has finished to wait for it to exit.

done := make(chan struct{})
go func() {
    defer close(done)
    // ...
}()
defer func() { <-done }() // wait for end

[abhinav]

This shouldn't be necessary. My guess is that it's to wait for the ticker to start up.
We can probably get rid of this by starting the ticker right above the go func(), or as an argument to go func.

go func(ticker *time.Ticker) {
  // ...
}(ctrlMock.NewTicker(..))

[abhinav]

code style: can be just time.Microsecond instead of 1 * time.Microsecond.

[abhinav]

Instead of using sync/atomic, we should use go.uber.org/atomic here.

var n atomic.Int32

// ...

<-ticker.C
n.Inc()

// ...

assert.Equal(t, 42, n.Load())

See also https://github.com/uber-go/guide/blob/master/style.md#use-gouberorgatomic

[abhinav]

If we embed clock.MockClock inside ctrlMock as suggested above, this .mockClock can be dropped.

[abhinav]

For assert.Equal, the arguments are (expected, got), not (got, expected).

[abhinav]

For channels that'll only ever fire once, close the channel instead of posting to it. (See also this.)

[abhinav]

Separately, please also see this comment.

We need to wait for the goroutine to actually exit once we tell it to stop before the test finishes.

Consider incorporating goleak with a TestMain in zapcore to detect issues like this.

[abhinav]

RE: lint failures: you need to run go mod tidy if this is a new dependency.

[mh-park]

LGTM!