Repository: etcd-io/etcd
File: tests/robustness/client/watch.go (Go)

Code before:
			defer c.Close()

			watchFailed := watchUntilRevision(ctx, lg, c, memberMaxRevisionChan, cfg)
			if watchFailed && !everFailed {
				everFailed = true
			}
			mux.Lock()
			reports[i] = c.Report()
			mux.Unlock()
		}(i, c)
	}
	wg.Add(1)
	go func() {
		defer wg.Done()
		maxRevision := <-maxRevisionChan
		for _, memberChan := range memberMaxRevisionChans {
			memberChan <- maxRevision
		}
	}()
	wg.Wait()
	return reports, everFailed, nil
}

type WatchConfig struct {
	RequestProgress bool
}

// watchUntilRevision watches all changes until context is cancelled, it has observed revision provided via maxRevisionChan or maxRevisionChan was closed.
func watchUntilRevision(ctx context.Context, lg *zap.Logger, c *RecordingClient, maxRevisionChan <-chan int64, cfg WatchConfig) bool {
	var maxRevision int64
	var lastRevision int64 = 1
	var failed = false
	ctx, cancel := context.WithCancel(ctx)
	defer cancel()
resetWatch:
	for {
		watch := c.Watch(ctx, "", lastRevision+1, true, true, false)
		for {
			select {
			case <-ctx.Done():
				if maxRevision == 0 {
					lg.Error("Client didn't collect all events, max revision not set")
					failed = true
				}
				if lastRevision < maxRevision {
					lg.Sugar().Errorf("Client didn't collect all events, revision got %d, expected: %d", lastRevision, maxRevision)
					failed = true
				}
				return failed
			case revision, ok := <-maxRevisionChan:
				if ok {

Review comment (bug):
Code is good and correct. Just a nit: Returning bool for failure doesn't seem to be very Go like. 

Couple of possible improvements, implementing any of them should be ok:
* Avoid negative logic, it's starts getting confusing when you start negating or doing logic on it. Prefer returning `success` over `failed`.
* Return just error, you can both log and return error.
```
err = errors.New("client didn't collect all events")
lg.Error("Failed watch until revision", zap.Error(err))
```

Code after:
			defer c.Close()
			watchSuccessful := watchUntilRevision(ctx, lg, c, memberMaxRevisionChan, cfg)
			mux.Lock()
			neverFailed = neverFailed && watchSuccessful
			reports[i] = c.Report()
			mux.Unlock()
		}(i, c)
	}
	wg.Add(1)
	go func() {
		defer wg.Done()
		maxRevision := <-maxRevisionChan
		for _, memberChan := range memberMaxRevisionChans {
			memberChan <- maxRevision
		}
	}()
	wg.Wait()
	return reports, neverFailed, nil
}

type WatchConfig struct {
	RequestProgress bool
}

// watchUntilRevision watches all changes until context is cancelled, it has observed revision provided via maxRevisionChan or maxRevisionChan was closed.
func watchUntilRevision(ctx context.Context, lg *zap.Logger, c *RecordingClient, maxRevisionChan <-chan int64, cfg WatchConfig) bool {
	var maxRevision int64
	var lastRevision int64 = 1
	var success = true
	ctx, cancel := context.WithCancel(ctx)
	defer cancel()
resetWatch:
	for {
		watch := c.Watch(ctx, "", lastRevision+1, true, true, false)
		for {
			select {
			case <-ctx.Done():
				if maxRevision == 0 {
					lg.Error("Client didn't collect all events, max revision not set")
					success = false
				}
				if lastRevision < maxRevision {
					lg.Error("Client didn't collect all events", zap.Int64("revision-got", lastRevision), zap.Int64("revision-expected", maxRevision))
					success = false
				}
				return success
			case revision, ok := <-maxRevisionChan:
				if ok {
					maxRevision = revision
					if lastRevision >= maxRevision {
						cancel()