Add tests to improve code coverage

glide.yaml

@@ -1,7 +1,7 @@
 package: github.com/m3db/m3metrics
 import:
 - package: github.com/m3db/m3x
-  version: 47d20dcef699f77d5560b364cd3f636cc4e42f74
+  version: d19f7459d421a726af8ecd086015be7e4f327de5
   subpackages:
   - pool
   - time

policy/compressor.go

@@ -73,47 +73,51 @@ func (m *dynamicCompressor) run(w kv.ValueWatch) {
 	for {
 		_, ok := <-w.C()
 		if ok {
-			m.logger.Debug("Received an update to Compression policies")
+			m.logger.Debug("Received an update to Compressor policies")
 			m.update(w.Get())
 		} else {
-			m.logger.Debug("ValueWatch for Compression Policies was closed")
+			m.logger.Debug("ValueWatch for Compressor Policies was closed")
 			return
 		}
 	}
 }
 
 func (m *dynamicCompressor) update(v kv.Value) {
 	if v == nil {
-		m.logger.Warn("Received a nil Value for Compression update")
+		m.logger.Warn("Received a nil Value for Compressor update")
 		return
 	}
 
 	activePolicies := schema.ActivePolicies{}
 	if err := v.Unmarshal(&activePolicies); err != nil {
-		m.logger.Error("Failed to unmarshal Compression update", zap.Error(err))
+		m.logger.Error("Failed to unmarshal Compressor update", zap.Error(err))
 		return
 	}
 
 	encodedPolicies := activePolicies.GetCompressedPolicies()
 	if encodedPolicies == nil {
-		m.logger.Warn("Encoded policies in update to Compression is nil")
+		m.logger.Warn("Encoded policies in update to Compressor is nil")
 		return
 	}
 
 	newPolicies := make(compressorMap, len(encodedPolicies))
 	for _, encodedPolicy := range encodedPolicies {
 		if encodedPolicy == nil {
-			m.logger.Warn("An encoded policy in update to Compression is nil")
+			m.logger.Warn("An encoded policy in update to Compressor is nil")
 			continue
 		}
 
 		policy := encodedPolicy.GetPolicy()
 		if policy == nil {
-			m.logger.Warn("The policy of an encoded policy in update to Compression is nil")
+			m.logger.Warn("The policy of an encoded policy in update to Compressor is nil")
 			continue
 		}
 
-		p := fromSchema(*policy)
+		p, err := NewPolicyFromSchema(policy)
+		if err != nil {
+			m.logger.Error("Failed to convert schema policy to actual policy in update to Compressor", zap.Error(err))
+			continue
+		}
 		newPolicies[p] = encodedPolicy.Id
 	}
 

policy/compressor_mock.go

@@ -7,6 +7,7 @@ import (
 	"github.com/golang/protobuf/proto"
 	"github.com/m3db/m3cluster/kv"
 	"github.com/m3db/m3metrics/generated/proto/schema"
+	"github.com/m3db/m3x/time"
 )
 
 // mockCompressionValue is a mock of a kv.Value that can be used for testing policy compression
@@ -22,7 +23,11 @@ func (m *mockCompressorValue) Unmarshal(v proto.Message) error {
 	}
 
 	for policy, id := range m.policies {
-		res := schema.Resolution{WindowSize: int64(policy.resolution.Window), Precision: int64(policy.resolution.Precision)}
+		precision, err := xtime.DurationFromUnit(policy.resolution.Precision)
+		if err != nil {
+			return err
+		}
+		res := schema.Resolution{WindowSize: int64(policy.resolution.Window), Precision: int64(precision)}
 		ret := schema.Retention{Period: int64(policy.retention)}
 		p := schema.Policy{Resolution: &res, Retention: &ret}
 		e := schema.CompressedPolicy{Policy: &p, Id: id}

policy/compressor_test.go

@@ -39,6 +39,15 @@ var (
 	}
 )
 
+func TestNoopCompressor(t *testing.T) {
+	c := NewNoopCompressor()
+
+	for policy := range compressorPolicies {
+		_, ok := c.Get(policy)
+		assert.False(t, ok)
+	}
+}
+
 func TestStaticCompressor(t *testing.T) {
 	c := NewStaticCompressor(compressorPolicies)
 

policy/decompressor.go

@@ -73,28 +73,28 @@ func (m *dynamicDecompressor) run(w kv.ValueWatch) {
 	for {
 		select {
 		case <-w.C():
-			m.logger.Debug("Received an update to Decompression policies")
+			m.logger.Debug("Received an update to Decompressor policies")
 			m.update(w.Get())
 		}
 	}
 }
 
 func (m *dynamicDecompressor) update(v kv.Value) {
 	if v == nil {
-		m.logger.Warn("Received a nil Value for Decompression update")
+		m.logger.Warn("Received a nil Value for Decompressor update")
 		return
 	}
 
 	activePolicies := schema.ActivePolicies{}
 	err := v.Unmarshal(&activePolicies)
 	if err != nil {
-		m.logger.Error("Failed to unmarshal Decompression update", zap.Error(err))
+		m.logger.Error("Failed to unmarshal Decompressor update", zap.Error(err))
 		return
 	}
 
 	encodedPolicies := activePolicies.GetCompressedPolicies()
 	if encodedPolicies == nil {
-		m.logger.Warn("Encoded policies in update to Decompression is nil")
+		m.logger.Warn("Encoded policies in update to Decompressor is nil")
 		return
 	}
 
@@ -109,7 +109,11 @@ func (m *dynamicDecompressor) update(v kv.Value) {
 			continue
 		}
 
-		p := fromSchema(*policy)
+		p, err := NewPolicyFromSchema(policy)
+		if err != nil {
+			m.logger.Error("Failed to convert schema policy to actual policy in update to Decompressor", zap.Error(err))
+			continue
+		}
 		newPolicies[encodedPolicy.Id] = p
 	}
 

policy/decompressor_mock.go

@@ -7,6 +7,7 @@ import (
 	"github.com/golang/protobuf/proto"
 	"github.com/m3db/m3cluster/kv"
 	"github.com/m3db/m3metrics/generated/proto/schema"
+	"github.com/m3db/m3x/time"
 )
 
 // mockDecompressorValue is a mock of a kv.Value that can be used for testing policy decompression
@@ -22,7 +23,11 @@ func (m *mockDecompressorValue) Unmarshal(v proto.Message) error {
 	}
 
 	for id, policy := range m.policies {
-		res := schema.Resolution{WindowSize: int64(policy.resolution.Window), Precision: int64(policy.resolution.Precision)}
+		precision, err := xtime.DurationFromUnit(policy.resolution.Precision)
+		if err != nil {
+			return err
+		}
+		res := schema.Resolution{WindowSize: int64(policy.resolution.Window), Precision: int64(precision)}
 		ret := schema.Retention{Period: int64(policy.retention)}
 		p := schema.Policy{Resolution: &res, Retention: &ret}
 		e := schema.CompressedPolicy{Policy: &p, Id: id}

policy/decompressor_test.go

@@ -39,6 +39,15 @@ var (
 	}
 )
 
+func TestNoopDecompressor(t *testing.T) {
+	c := NewNoopDecompressor()
+
+	for id := range decompressorPolicies {
+		_, ok := c.Get(id)
+		assert.False(t, ok)
+	}
+}
+
 func TestStaticDecompressor(t *testing.T) {
 	c := NewStaticDecompressor(decompressorPolicies)
 

policy/policy.go

@@ -52,7 +52,7 @@ var (
 		NewPolicy(time.Minute, xtime.Minute, 30*24*time.Hour),
 	}
 
-	errNilPolicySchema = errors.New("nil policy schema")
+	errNilPolicySchema = errors.New("policy schema is nil or contains nil fields")
 )
 
 // Policy represents the resolution and retention period metric datapoints
@@ -75,7 +75,7 @@ func NewPolicy(window time.Duration, precision xtime.Unit, retention time.Durati
 
 // NewPolicyFromSchema creates a new policy from a schema policy
 func NewPolicyFromSchema(p *schema.Policy) (Policy, error) {
-	if p == nil {
+	if p == nil || p.Resolution == nil || p.Retention == nil {
 		return EmptyPolicy, errNilPolicySchema
 	}
 	precision := time.Duration(p.Resolution.Precision)
@@ -120,23 +120,6 @@ func (p Policy) Retention() Retention {
 	return p.retention
 }
 
-// fromSchema returns a new Policy from a schema policy
-func fromSchema(policy schema.Policy) Policy {
-	if policy.Resolution == nil || policy.Retention == nil {
-		return EmptyPolicy
-	}
-
-	p := Policy{
-		resolution: Resolution{
-			Window:    time.Duration(policy.Resolution.WindowSize),
-			Precision: xtime.Unit(policy.Resolution.Precision),
-		},
-		retention: Retention(policy.Retention.Period),
-	}
-
-	return p
-}
-
 // ByResolutionAsc implements the sort.Sort interface to sort
 // policies by resolution in ascending order, finest resolution first.
 // If two policies have the same resolution, the one with longer

policy/policy_test.go

@@ -75,45 +75,63 @@ func TestCustomVersionedPolicies(t *testing.T) {
 	require.Equal(t, policies, vp.Policies())
 }
 
-func TestFromSchema(t *testing.T) {
+func TestNewPolicyFromSchema(t *testing.T) {
 	tests := []struct {
-		policy   schema.Policy
-		expected Policy
+		policy         *schema.Policy
+		expectedPolicy Policy
+		expectedErr    bool
 	}{
+		// nil policy
 		{
-			policy: schema.Policy{
-				Resolution: nil,
-				Retention:  nil,
-			},
-			expected: EmptyPolicy,
+			policy:      nil,
+			expectedErr: true,
 		},
+		// nil retention
 		{
-			policy: schema.Policy{
+			policy: &schema.Policy{
 				Resolution: &schema.Resolution{WindowSize: 100, Precision: 10},
 				Retention:  nil,
 			},
-			expected: EmptyPolicy,
+			expectedErr: true,
 		},
+		// nil resolution
 		{
-			policy: schema.Policy{
+			policy: &schema.Policy{
 				Resolution: nil,
 				Retention:  &schema.Retention{Period: 20},
 			},
-			expected: EmptyPolicy,
+			expectedErr: true,
 		},
+		// invalid precision
 		{
-			policy: schema.Policy{
-				Resolution: &schema.Resolution{WindowSize: 100, Precision: 10},
+			policy: &schema.Policy{
+				Resolution: &schema.Resolution{WindowSize: 100, Precision: 42},
+				Retention:  &schema.Retention{Period: 20},
+			},
+			expectedErr: true,
+		},
+		// valid policy
+		{
+			policy: &schema.Policy{
+				Resolution: &schema.Resolution{WindowSize: 100, Precision: int64(time.Second)},
 				Retention:  &schema.Retention{Period: 20},
 			},
-			expected: Policy{
-				resolution: Resolution{Window: 100, Precision: 10},
+			expectedPolicy: Policy{
+				resolution: Resolution{Window: 100, Precision: xtime.Second},
 				retention:  20,
 			},
+			expectedErr: false,
 		},
 	}
 
 	for _, test := range tests {
-		assert.Equal(t, test.expected, fromSchema(test.policy))
+		actualPolicy, actualErr := NewPolicyFromSchema(test.policy)
+		if test.expectedErr {
+			assert.Error(t, actualErr)
+			continue
+		}
+
+		assert.NoError(t, actualErr)
+		assert.Equal(t, test.expectedPolicy, actualPolicy)
 	}
 }

protocol/msgpack/aggregated_encoder_test.go

@@ -161,3 +161,10 @@ func TestAggregatedEncoderReset(t *testing.T) {
 	encoder.Reset(NewBufferedEncoder())
 	require.NoError(t, testAggregatedEncode(t, encoder, testMetric, testPolicy))
 }
+
+func TestAggregatedEncoderNilOptions(t *testing.T) {
+	// use constructor directly here to test nil options
+	encoder := NewAggregatedEncoder(NewBufferedEncoder(), nil).(*aggregatedEncoder)
+	encoder.encodeMetricAsRaw(testMetric)
+	require.NoError(t, encoder.err())
+}

protocol/msgpack/unaggregated_encoder_test.go

@@ -354,3 +354,13 @@ func TestUnaggregatedEncoderReset(t *testing.T) {
 	encoder.Reset(NewBufferedEncoder())
 	require.NoError(t, testUnaggregatedEncode(t, encoder, metric, policies))
 }
+
+func TestUnaggregatedEncoderNilOptions(t *testing.T) {
+	// use constructor directly here to test nil options
+	encoder := NewUnaggregatedEncoder(NewBufferedEncoder(), nil).(*unaggregatedEncoder)
+	encoder.EncodeCounterWithPolicies(unaggregated.CounterWithPolicies{
+		Counter:           testCounter.Counter(),
+		VersionedPolicies: testDefaultVersionedPolicies,
+	})
+	require.NoError(t, encoder.err())
+}