[refactor] Clean-up tests from #2188 (#2200)

Signed-off-by: Yuri Shkuro <ys@uber.com>

Author: yurishkuro

plugin/sampling/strategystore/static/strategy_store.go

@@ -84,35 +84,40 @@ func (h *strategyStore) Close() {
 }
 
 func (h *strategyStore) autoUpdateStrategies(interval time.Duration, filePath string) {
-	lastString := ""
+	lastValue := ""
 	ticker := time.NewTicker(interval)
 	defer ticker.Stop()
 	for {
 		select {
 		case <-ticker.C:
-			currBytes, err := ioutil.ReadFile(filepath.Clean(filePath))
-			if err != nil {
-				h.logger.Error("ReadFile failed", zap.Error(err))
-			}
-			currStr := string(currBytes)
-			if lastString == currStr {
-				continue
-			}
-			if err = h.updateSamplingStrategy(currBytes); err != nil {
-				h.logger.Error("UpdateSamplingStrategy failed", zap.Error(err))
-			}
-			lastString = currStr
-
+			lastValue = h.reloadSamplingStrategyFile(filePath, lastValue)
 		case <-h.ctx.Done():
 			return
 		}
 	}
 }
 
+func (h *strategyStore) reloadSamplingStrategyFile(filePath string, lastValue string) string {
+	currBytes, err := ioutil.ReadFile(filepath.Clean(filePath))
+	if err != nil {
+		h.logger.Error("failed to load sampling strategies", zap.String("file", filePath), zap.Error(err))
+		return lastValue
+	}
+	newValue := string(currBytes)
+	if lastValue == newValue {
+		return lastValue
+	}
+	if err = h.updateSamplingStrategy(currBytes); err != nil {
+		h.logger.Error("failed to update sampling strategies from file", zap.Error(err))
+		return lastValue
+	}
+	return newValue
+}
+
 func (h *strategyStore) updateSamplingStrategy(bytes []byte) error {
 	var strategies strategies
 	if err := json.Unmarshal(bytes, &strategies); err != nil {
-		return fmt.Errorf("failed to unmarshal strategies: %w", err)
+		return fmt.Errorf("failed to unmarshal sampling strategies: %w", err)
 	}
 	h.parseStrategies(&strategies)
 	h.logger.Info("Updated sampling strategies:" + string(bytes))
@@ -124,12 +129,12 @@ func loadStrategies(strategiesFile string) (*strategies, error) {
 	if strategiesFile == "" {
 		return nil, nil
 	}
-	bytes, err := ioutil.ReadFile(strategiesFile) /* nolint #nosec , this comes from an admin, not user */
+	data, err := ioutil.ReadFile(strategiesFile) /* nolint #nosec , this comes from an admin, not user */
 	if err != nil {
 		return nil, fmt.Errorf("failed to open strategies file: %w", err)
 	}
 	var strategies strategies
-	if err := json.Unmarshal(bytes, &strategies); err != nil {
+	if err := json.Unmarshal(data, &strategies); err != nil {
 		return nil, fmt.Errorf("failed to unmarshal strategies: %w", err)
 	}
 	return &strategies, nil

plugin/sampling/strategystore/static/strategy_store_test.go

@@ -24,6 +24,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"go.uber.org/zap"
+	"go.uber.org/zap/zaptest/observer"
 
 	"github.com/jaegertracing/jaeger/pkg/testutils"
 	"github.com/jaegertracing/jaeger/thrift-gen/sampling"
@@ -242,54 +243,81 @@ func TestDeepCopy(t *testing.T) {
 			SamplingRate: 0.5,
 		},
 	}
-	copy := deepCopy(s)
-	assert.False(t, copy == s)
-	assert.EqualValues(t, copy, s)
+	cp := deepCopy(s)
+	assert.False(t, cp == s)
+	assert.EqualValues(t, cp, s)
 }
 
 func TestAutoUpdateStrategy(t *testing.T) {
-	// copy from fixtures/strategies.json
 	tempFile, _ := ioutil.TempFile("", "for_go_test_*.json")
-	tempFile.Close()
+	require.NoError(t, tempFile.Close())
+	defer func() {
+		require.NoError(t, os.Remove(tempFile.Name()))
+	}()
 
+	// copy known fixture content into temp file which we can later overwrite
 	srcFile, dstFile := "fixtures/strategies.json", tempFile.Name()
 	srcBytes, err := ioutil.ReadFile(srcFile)
 	require.NoError(t, err)
-	err = ioutil.WriteFile(dstFile, srcBytes, 0644)
-	require.NoError(t, err)
+	require.NoError(t, ioutil.WriteFile(dstFile, srcBytes, 0644))
 
-	interval := time.Millisecond * 10
-	store, err := NewStrategyStore(Options{
+	ss, err := NewStrategyStore(Options{
 		StrategiesFile: dstFile,
-		ReloadInterval: interval,
+		ReloadInterval: time.Millisecond * 10,
 	}, zap.NewNop())
 	require.NoError(t, err)
-	defer store.(*strategyStore).Close()
+	store := ss.(*strategyStore)
+	defer store.Close()
 
+	// confirm baseline value
 	s, err := store.GetSamplingStrategy("foo")
 	require.NoError(t, err)
 	assert.EqualValues(t, makeResponse(sampling.SamplingStrategyType_PROBABILISTIC, 0.8), *s)
 
-	// update file
-	newStr := strings.Replace(string(srcBytes), "0.8", "0.9", 1)
-	err = ioutil.WriteFile(dstFile, []byte(newStr), 0644)
-	require.NoError(t, err)
-
-	// wait for reloading
-	time.Sleep(interval * 4)
+	// verify that reloading in no-op
+	value := store.reloadSamplingStrategyFile(dstFile, string(srcBytes))
+	assert.Equal(t, string(srcBytes), value)
 
-	// verity reloading
-	s, err = store.GetSamplingStrategy("foo")
-	require.NoError(t, err)
+	// update file with new probability of 0.9
+	newStr := strings.Replace(string(srcBytes), "0.8", "0.9", 1)
+	require.NoError(t, ioutil.WriteFile(dstFile, []byte(newStr), 0644))
+
+	// wait for reload timer
+	for i := 0; i < 1000; i++ { // wait up to 1sec
+		s, err = store.GetSamplingStrategy("foo")
+		require.NoError(t, err)
+		if s.ProbabilisticSampling != nil && s.ProbabilisticSampling.SamplingRate == 0.9 {
+			break
+		}
+		time.Sleep(1 * time.Millisecond)
+	}
 	assert.EqualValues(t, makeResponse(sampling.SamplingStrategyType_PROBABILISTIC, 0.9), *s)
+}
 
-	// check bad file content
-	_ = ioutil.WriteFile(dstFile, []byte("bad value"), 0644)
-	time.Sleep(interval * 2)
+func TestAutoUpdateStrategyErrors(t *testing.T) {
+	tempFile, _ := ioutil.TempFile("", "for_go_test_*.json")
+	require.NoError(t, tempFile.Close())
+	defer func() {
+		_ = os.Remove(tempFile.Name())
+	}()
+
+	zapCore, logs := observer.New(zap.InfoLevel)
+	logger := zap.New(zapCore)
+
+	s, err := NewStrategyStore(Options{
+		StrategiesFile: "fixtures/strategies.json",
+		ReloadInterval: time.Hour,
+	}, logger)
+	require.NoError(t, err)
+	store := s.(*strategyStore)
+	defer store.Close()
 
-	// remove file(test read file failure)
-	_ = os.Remove(dstFile)
-	// wait for delete and update failure
-	time.Sleep(interval * 2)
+	// check invalid file path or read failure
+	assert.Equal(t, "blah", store.reloadSamplingStrategyFile(tempFile.Name()+"bad-path", "blah"))
+	assert.Len(t, logs.FilterMessage("failed to load sampling strategies").All(), 1)
 
+	// check bad file content
+	require.NoError(t, ioutil.WriteFile(tempFile.Name(), []byte("bad value"), 0644))
+	assert.Equal(t, "blah", store.reloadSamplingStrategyFile(tempFile.Name(), "blah"))
+	assert.Len(t, logs.FilterMessage("failed to update sampling strategies from file").All(), 1)
 }