[FFM-2217]: Evaluation returning wrong result with multiple target groups (#70)

Segments evaluation had two problems:
 1) It returned false if any segment evaluate returned false (even if segment was not part of the clause)
 2) It evaluated the target in segmentMatch clause against all segments, where it should only evaluate against
   the segments defined by the clause.

Evaluation Percentage Rollout was also distributing values incorrectly when there were more than
two variations.

Updated unit tests to ensure segment Evaluation performed as expected

Author: Dave Johnston

evaluation/feature.go

@@ -51,7 +51,16 @@ func (c *Clause) Evaluate(target *Target, segments Segments, operator types.Valu
 		if segments == nil {
 			return false
 		}
-		return segments.Evaluate(target)
+		// Determine if the given target belongs to one of the segments,
+		// that was specified by the clause
+		for _, segmentName := range c.Value {
+			if segment, ok := segments[segmentName]; ok {
+				if segment.Evaluate(target) {
+					return true
+				}
+			}
+		}
+		return false
 	}
 
 	// Ensure operator is valid and not nil
@@ -100,6 +109,7 @@ func (c Clauses) Evaluate(target *Target, segments Segments) bool {
 				log.Warn(err)
 			}
 		}
+
 		if !clause.Evaluate(target, segments, op) {
 			return false
 		}
@@ -118,9 +128,12 @@ type Distribution struct {
 // GetKeyName returns variation identifier based on target
 func (d *Distribution) GetKeyName(target *Target) string {
 	variation := ""
+
+	totalPercentage := 0
 	for _, tdVariation := range d.Variations {
 		variation = tdVariation.Variation
-		if d.isEnabled(target, tdVariation.Weight) {
+		totalPercentage += tdVariation.Weight
+		if d.isEnabled(target, totalPercentage) {
 			return variation
 		}
 	}

evaluation/feature_test.go

@@ -556,34 +556,47 @@ func TestClause_Evaluate(t *testing.T) {
 		Anonymous:  &f,
 		Attributes: &m,
 	}
-	tests := []struct {
+	tests := map[string]struct {
 		name   string
 		fields fields
 		args   args
 		want   bool
 	}{
-		{name: "segment match operator (include)", fields: struct {
-			Attribute string
-			ID        string
-			Negate    bool
-			Op        string
-			Value     []string
-		}{Op: segmentMatchOperator, Value: []string{"beta"}},
-			args: struct {
-				target   *Target
-				segments Segments
-				operator types.ValueType
-			}{target: &target, segments: map[string]*Segment{
+		"segment match operator (include)": {
+			fields: fields{Op: segmentMatchOperator, Value: []string{"beta"}},
+			args: args{target: &target, segments: map[string]*Segment{
 				"beta": {
 					Identifier: "beta",
 					Name:       "Beta users",
 					Included:   []string{target.Identifier},
 				},
-			}, operator: nil}, want: true},
+			}, operator: nil},
+			want: true,
+		},
+		"evaluate returns false when clause value does not match any segment": {
+			fields: fields{Op: segmentMatchOperator, Value: []string{"beta"}},
+			args: args{
+				target: &target,
+				segments: Segments{
+					"beta":  {Identifier: "beta", Included: []string{}},
+					"alpha": {Identifier: "alpha", Included: []string{target.Identifier}},
+				}, operator: nil},
+			want: false,
+		},
+		"evaluate returns true when clause value matches segment that target belongs to": {
+			fields: fields{Op: segmentMatchOperator, Value: []string{"alpha"}},
+			args: args{
+				target: &target,
+				segments: Segments{
+					"beta":  {Identifier: "beta", Excluded: []string{target.Identifier}},
+					"alpha": {Identifier: "alpha", Included: []string{target.Identifier}},
+				}, operator: nil},
+			want: true,
+		},
 	}
-	for _, tt := range tests {
+	for name, tt := range tests {
 		val := tt
-		t.Run(tt.name, func(t *testing.T) {
+		t.Run(name, func(t *testing.T) {
 			c := &Clause{
 				Attribute: val.fields.Attribute,
 				ID:        val.fields.ID,

evaluation/segment.go

@@ -107,9 +107,9 @@ type Segments map[string]*Segment
 // Evaluate through all segments based on target input
 func (s Segments) Evaluate(target *Target) bool {
 	for _, segment := range s {
-		if !segment.Evaluate(target) {
-			return false
+		if segment.Evaluate(target) {
+			return true
 		}
 	}
-	return true
+	return false
 }

evaluation/segment_test.go

@@ -51,3 +51,67 @@ func TestSegment_Evaluate(t *testing.T) {
 		})
 	}
 }
+
+func TestSegments_Evaluate(t *testing.T) {
+	f := false
+	m := make(map[string]interface{})
+	m["email"] = "[email protected]"
+	target := Target{
+		Identifier: "john",
+		Anonymous:  &f,
+		Attributes: &m,
+	}
+
+	tests := map[string]struct {
+		segments Segments
+		target   Target
+		want     bool
+	}{
+		"test target included by segment alpha returns true": {
+			segments: Segments{"alpha": {Identifier: "alpha", Included: []string{target.Identifier}}},
+			target:   target,
+			want:     true,
+		},
+		"test target not included segment alpha, but included in beta returns true": {
+			segments: Segments{
+				"alpha": {Identifier: "alpha", Included: []string{}},
+				"beta":  {Identifier: "beta", Included: []string{target.Identifier}},
+			},
+			target: target,
+			want:   true,
+		},
+		"test target not included segment alpha, and not included in beta returns false": {
+			segments: Segments{
+				"alpha": {Identifier: "alpha", Included: []string{}},
+				"beta":  {Identifier: "beta", Included: []string{}},
+			},
+			target: target,
+			want:   false,
+		},
+		"test target included segment alpha, and excluded in beta returns true": {
+			segments: Segments{
+				"alpha": {Identifier: "alpha", Included: []string{target.Identifier}},
+				"beta":  {Identifier: "beta", Excluded: []string{target.Identifier}},
+			},
+			target: target,
+			want:   true,
+		},
+		"test target excluded from segment alpha, and included in beta returns true": {
+			segments: Segments{
+				"alpha": {Identifier: "alpha", Excluded: []string{target.Identifier}},
+				"beta":  {Identifier: "beta", Included: []string{target.Identifier}},
+			},
+			target: target,
+			want:   true,
+		},
+	}
+	for name, tt := range tests {
+		val := tt
+		t.Run(name, func(t *testing.T) {
+			s := val.segments
+			if got := s.Evaluate(&val.target); got != val.want {
+				t.Errorf("Evaluate() = %v, want %v", got, val.want)
+			}
+		})
+	}
+}