From 57581b55103527d1c74411e8044d6bdd22599859 Mon Sep 17 00:00:00 2001 From: Dave Johnston Date: Tue, 1 Feb 2022 08:19:50 +0000 Subject: [PATCH] [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 --- evaluation/feature.go | 17 ++++++++-- evaluation/feature_test.go | 45 +++++++++++++++++---------- evaluation/segment.go | 6 ++-- evaluation/segment_test.go | 64 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 111 insertions(+), 21 deletions(-) diff --git a/evaluation/feature.go b/evaluation/feature.go index e7bbb679..a1fc0287 100644 --- a/evaluation/feature.go +++ b/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 } } diff --git a/evaluation/feature_test.go b/evaluation/feature_test.go index 8255bb9f..5258428f 100644 --- a/evaluation/feature_test.go +++ b/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, diff --git a/evaluation/segment.go b/evaluation/segment.go index e44974fe..4ee2befc 100644 --- a/evaluation/segment.go +++ b/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 } diff --git a/evaluation/segment_test.go b/evaluation/segment_test.go index 233396e2..aa83a250 100644 --- a/evaluation/segment_test.go +++ b/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"] = "john@doe.com" + 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) + } + }) + } +}