[FFM-5387]: Fix Golang Group Rule Evaluate and Add Tests (#101)

Run evaluator tests against the ff-test-grid test-cases
Improve test coverage
Fix the group rule evaluator.  Currently rules are being AND'd but they
should be OR'd

Author: Dave Johnston

Makefile

@@ -28,8 +28,8 @@ test:
 	go test -race -v --cover ./...
 
 report:
-	go test ./... -covermode=atomic -coverpkg=./...  -coverprofile=c.out
-	gocov convert ./c.out | gocov-html > ~/go-sdk-test-report.html
+	go test -race -v -covermode=atomic -coverprofile=cover.out ./... | tee /dev/stderr | go-junit-report -set-exit-code > junit.xml
+	gocov convert ./cover.out | gocov-html > coverage-report.html
 
 
 build-test-wrapper:

evaluation/evaluator.go

@@ -179,6 +179,18 @@ func (e Evaluator) evaluateRules(servingRules []rest.ServingRule, target *Target
 	return ""
 }
 
+// evaluateGroupRules evaluates the groups rules.  Note Group rule are represented by a rest.Clause, instead
+// of a rest.Rule.   Unlike feature clauses which are AND'd, in a case of  a group these must be OR'd.
+func (e Evaluator) evaluateGroupRules(rules []rest.Clause, target *Target) (bool, rest.Clause) {
+	for _, r := range rules {
+		rule := r
+		if e.evaluateClause(&rule, target) {
+			return true, r
+		}
+	}
+	return false, rest.Clause{}
+}
+
 func (e Evaluator) evaluateVariationMap(variationsMap []rest.VariationMap, target *Target) string {
 	if variationsMap == nil || target == nil {
 		return ""
@@ -252,11 +264,14 @@ func (e Evaluator) isTargetIncludedOrExcludedInSegment(segmentList []string, tar
 		// Should Target be included via segment rules
 		rules := segment.Rules
 		// if rules is nil pointer or points to the empty slice
-		if (rules != nil && len(*rules) > 0) && e.evaluateClauses(*rules, target) {
-			e.logger.Debugf(
-				"Target %s included in segment %s via rules", target.Name, segment.Name)
-			return true
+		if rules != nil && len(*rules) > 0 {
+			if included, clause := e.evaluateGroupRules(*rules, target); included {
+				e.logger.Debugf(
+					"Target [%s] included in group [%s] via rule %+v", target.Name, segment.Name, clause)
+				return true
+			}
 		}
+
 	}
 	return false
 }

tests/evaluator_test.go

@@ -4,10 +4,13 @@ import (
 	"encoding/json"
 	"fmt"
 	"io/ioutil"
+	"os"
 	"path/filepath"
 	"reflect"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
+
 	"github.com/harness/ff-golang-server-sdk/logger"
 
 	"github.com/harness/ff-golang-server-sdk/evaluation"
@@ -34,25 +37,32 @@ type testFile struct {
 }
 
 func loadFiles() []testFile {
-	files, err := ioutil.ReadDir(source)
+
+	slice := []testFile{}
+	err := filepath.Walk(source,
+		func(path string, info os.FileInfo, err error) error {
+			if err != nil {
+				return err
+			}
+			if info.IsDir() || filepath.Ext(info.Name()) != ".json" {
+				return nil
+			}
+			if f, err := loadFile(path); err == nil {
+				slice = append(slice, f)
+			} else {
+				log.Errorf("unable to load %s because %v", info.Name(), err)
+			}
+			return nil
+		})
 	if err != nil {
 		log.Error(err)
 	}
 
-	slice := make([]testFile, 0, len(files))
-	for _, file := range files {
-		if file.IsDir() || filepath.Ext(file.Name()) != ".json" {
-			continue
-		}
-		if f, err := loadFile(file.Name()); err == nil {
-			slice = append(slice, f)
-		}
-	}
 	return slice
 }
 
 func loadFile(filename string) (testFile, error) {
-	fp := filepath.Clean(filepath.Join(source, filename))
+	fp := filepath.Clean(filename)
 	content, err := ioutil.ReadFile(fp)
 	if err != nil {
 		log.Error(err)
@@ -91,7 +101,7 @@ func TestEvaluator(t *testing.T) {
 		}
 
 		for _, testCase := range fixture.Tests {
-			testName := fmt.Sprintf("test fixture %s with flag %s", fixture.Filename, testCase.Flag)
+			testName := fixture.Filename
 			if testCase.Target != nil {
 				testName = fmt.Sprintf("%s and target %s", testName, *testCase.Target)
 			}
@@ -114,16 +124,25 @@ func TestEvaluator(t *testing.T) {
 					got = evaluator.BoolVariation(testCase.Flag, target, false)
 				case rest.FeatureConfigKindString:
 					got = evaluator.StringVariation(testCase.Flag, target, "blue")
-				case rest.FeatureConfigKindInt:
-					got = evaluator.IntVariation(testCase.Flag, target, 100)
-				case "number":
+				case rest.FeatureConfigKindInt, "number":
 					got = evaluator.NumberVariation(testCase.Flag, target, 50.00)
 				case rest.FeatureConfigKindJson:
 					got = evaluator.JSONVariation(testCase.Flag, target, map[string]interface{}{})
+					str, _ := json.Marshal(&got)
+					got = string(str)
+
 				}
-				if !reflect.DeepEqual(got, testCase.Expected) {
-					t.Errorf("eval engine got = %v, want %v", got, testCase.Expected)
+
+				if flag.Kind == rest.FeatureConfigKindJson {
+					expected := fmt.Sprintf("%s", testCase.Expected)
+					jsonStr := fmt.Sprintf("%s", got)
+					assert.JSONEq(t, expected, jsonStr, "eval engine got = [%v], want [%v]", jsonStr, expected)
+				} else {
+					if !reflect.DeepEqual(got, testCase.Expected) {
+						t.Errorf("eval engine got = [%v], want [%v]", got, testCase.Expected)
+					}
 				}
+
 			})
 		}
 	}