Repository: stretchr/testify
Pull request #733 (2019-02-22 08:09:25)

refactoring of assertion_order
Refactoring of the assertion order functionality:
 - Using constants `greater`, `equal`, `less` instead of `-1`, `0`, `1`. Now the code become more readable.
 - Unify working with comparing results. There was almost the same copy-pasted code for `Greater`, `GreaterOrEqual`, `Less`, `LessOrEqual` functions. Now these call one function with all common logic. It could help to change behavior for all of them in future.

Linked issue https://github.com/stretchr/testify/issues/903 .

alexpantyukhin: @georgelesica-wf Could you please review this PR?
georgelesica-wf: Please update the description so that it contains a high-level overview of your changes. You should include the problem you're attempting to solve, an overview of the solution, and how your changes might impact users. Also, if this PR is tied to any open issues, please link to them.
alexpantyukhin: @georgelesica-wf Sorry to be late. I have added a detail description into the head of the PR. This is not linked with some issue, Just some thoughts and implementations of refactoring.
boyan-soubachov: @alexpantyukhin , please rebase the branch and resolve the conflicts before I can merge it :)
alexpantyukhin: @boyan-soubachov thamk you very much for your review) It is rebased.

Patch:
diff --git a/assert/assertion_order.go b/assert/assertion_order.go
index 15a486c..66961ba 100644
--- a/assert/assertion_order.go
+++ b/assert/assertion_order.go
@@ -5,20 +5,28 @@ import (
 	"reflect"
 )
 
-func compare(obj1, obj2 interface{}, kind reflect.Kind) (int, bool) {
+type CompareType int
+
+const (
+	compareLess CompareType = iota - 1
+	compareEqual
+	compareGreater
+)
+
+func compare(obj1, obj2 interface{}, kind reflect.Kind) (CompareType, bool) {
 	switch kind {
 	case reflect.Int:
 		{
 			intobj1 := obj1.(int)
 			intobj2 := obj2.(int)
 			if intobj1 > intobj2 {
-				return -1, true
+				return compareGreater, true
 			}
 			if intobj1 == intobj2 {
-				return 0, true
+				return compareEqual, true
 			}
 			if intobj1 < intobj2 {
-				return 1, true
+				return compareLess, true
 			}
 		}
 	case reflect.Int8:
@@ -26,13 +34,13 @@ func compare(obj1, obj2 interface{}, kind reflect.Kind) (int, bool) {
 			int8obj1 := obj1.(int8)
 			int8obj2 := obj2.(int8)
 			if int8obj1 > int8obj2 {
-				return -1, true
+				return compareGreater, true
 			}
 			if int8obj1 == int8obj2 {
-				return 0, true
+				return compareEqual, true
 			}
 			if int8obj1 < int8obj2 {
-				return 1, true
+				return compareLess, true
 			}
 		}
 	case reflect.Int16:
@@ -40,13 +48,13 @@ func compare(obj1, obj2 interface{}, kind reflect.Kind) (int, bool) {
 			int16obj1 := obj1.(int16)
 			int16obj2 := obj2.(int16)
 			if int16obj1 > int16obj2 {
-				return -1, true
+				return compareGreater, true
 			}
 			if int16obj1 == int16obj2 {
-				return 0, true
+				return compareEqual, true
 			}
 			if int16obj1 < int16obj2 {
-				return 1, true
+				return compareLess, true
 			}
 		}
 	case reflect.Int32:
@@ -54,13 +62,13 @@ func compare(obj1, obj2 interface{}, kind reflect.Kind) (int, bool) {
 			int32obj1 := obj1.(int32)
 			int32obj2 := obj2.(int32)
 			if int32obj1 > int32obj2 {
-				return -1, true
+				return compareGreater, true
 			}
 			if int32obj1 == int32obj2 {
-				return 0, true
+				return compareEqual, true
 			}
 			if int32obj1 < int32obj2 {
-				return 1, true
+				return compareLess, true
 			}
 		}
 	case reflect.Int64:
@@ -68,13 +76,13 @@ func compare(obj1, obj2 interface{}, kind reflect.Kind) (int, bool) {
 			int64obj1 := obj1.(int64)
 			int64obj2 := obj2.(int64)
 			if int64obj1 > int64obj2 {
-				return -1, true
+				return compareGreater, true
 			}
 			if int64obj1 == int64obj2 {
-				return 0, true
+				return compareEqual, true
 			}
 			if int64obj1 < int64obj2 {
-				return 1, true
+				return compareLess, true
 			}
 		}
 	case reflect.Uint:
@@ -82,13 +90,13 @@ func compare(obj1, obj2 interface{}, kind reflect.Kind) (int, bool) {
 			uintobj1 := obj1.(uint)
 			uintobj2 := obj2.(uint)
 			if uintobj1 > uintobj2 {
-				return -1, true
+				return compareGreater, true
 			}
 			if uintobj1 == uintobj2 {
-				return 0, true
+				return compareEqual, true
 			}
 			if uintobj1 < uintobj2 {
-				return 1, true
+				return compareLess, true
 			}
 		}
 	case reflect.Uint8:
@@ -96,13 +104,13 @@ func compare(obj1, obj2 interface{}, kind reflect.Kind) (int, bool) {
 			uint8obj1 := obj1.(uint8)
 			uint8obj2 := obj2.(uint8)
 			if uint8obj1 > uint8obj2 {
-				return -1, true
+				return compareGreater, true
 			}
 			if uint8obj1 == uint8obj2 {
-				return 0, true
+				return compareEqual, true
 			}
 			if uint8obj1 < uint8obj2 {
-				return 1, true
+				return compareLess, true
 			}
 		}
 	case reflect.Uint16:
@@ -110,13 +118,13 @@ func compare(obj1, obj2 interface{}, kind reflect.Kind) (int, bool) {
 			uint16obj1 := obj1.(uint16)
 			uint16obj2 := obj2.(uint16)
 			if uint16obj1 > uint16obj2 {
-				return -1, true
+				return compareGreater, true
 			}
 			if uint16obj1 == uint16obj2 {
-				return 0, true
+				return compareEqual, true
 			}
 			if uint16obj1 < uint16obj2 {
-				return 1, true
+				return compareLess, true
 			}
 		}
 	case reflect.Uint32:
@@ -124,13 +132,13 @@ func compare(obj1, obj2 interface{}, kind reflect.Kind) (int, bool) {
 			uint32obj1 := obj1.(uint32)
 			uint32obj2 := obj2.(uint32)
 			if uint32obj1 > uint32obj2 {
-				return -1, true
+				return compareGreater, true
 			}
 			if uint32obj1 == uint32obj2 {
-				return 0, true
+				return compareEqual, true
 			}
 			if uint32obj1 < uint32obj2 {
-				return 1, true
+				return compareLess, true
 			}
 		}
 	case reflect.Uint64:
@@ -138,13 +146,13 @@ func compare(obj1, obj2 interface{}, kind reflect.Kind) (int, bool) {
 			uint64obj1 := obj1.(uint64)
 			uint64obj2 := obj2.(uint64)
 			if uint64obj1 > uint64obj2 {
-				return -1, true
+				return compareGreater, true
 			}
 			if uint64obj1 == uint64obj2 {
-				return 0, true
+				return compareEqual, true
 			}
 			if uint64obj1 < uint64obj2 {
-				return 1, true
+				return compareLess, true
 			}
 		}
 	case reflect.Float32:
@@ -152,13 +160,13 @@ func compare(obj1, obj2 interface{}, kind reflect.Kind) (int, bool) {
 			float32obj1 := obj1.(float32)
 			float32obj2 := obj2.(float32)
 			if float32obj1 > float32obj2 {
-				return -1, true
+				return compareGreater, true
 			}
 			if float32obj1 == float32obj2 {
-				return 0, true
+				return compareEqual, true
 			}
 			if float32obj1 < float32obj2 {
-				return 1, true
+				return compareLess, true
 			}
 		}
 	case reflect.Float64:
@@ -166,13 +174,13 @@ func compare(obj1, obj2 interface{}, kind reflect.Kind) (int, bool) {
 			float64obj1 := obj1.(float64)
 			float64obj2 := obj2.(float64)
 			if float64obj1 > float64obj2 {
-				return -1, true
+				return compareGreater, true
 			}
 			if float64obj1 == float64obj2 {
-				return 0, true
+				return compareEqual, true
 			}
 			if float64obj1 < float64obj2 {
-				return 1, true
+				return compareLess, true
 			}
 		}
 	case reflect.String:
@@ -180,18 +188,18 @@ func compare(obj1, obj2 interface{}, kind reflect.Kind) (int, bool) {
 			stringobj1 := obj1.(string)
 			stringobj2 := obj2.(string)
 			if stringobj1 > stringobj2 {
-				return -1, true
+				return compareGreater, true
 			}
 			if stringobj1 == stringobj2 {
-				return 0, true
+				return compareEqual, true
 			}
 			if stringobj1 < stringobj2 {
-				return 1, true
+				return compareLess, true
 			}
 		}
 	}
 
-	return 0, false
+	return compareEqual, false
 }
 
 // Greater asserts that the first element is greater than the second
@@ -200,26 +208,7 @@ func compare(obj1, obj2 interface{}, kind reflect.Kind) (int, bool) {
 //    assert.Greater(t, float64(2), float64(1))
 //    assert.Greater(t, "b", "a")
 func Greater(t TestingT, e1 interface{}, e2 interface{}, msgAndArgs ...interface{}) bool {
-	if h, ok := t.(tHelper); ok {
-		h.Helper()
-	}
-
-	e1Kind := reflect.ValueOf(e1).Kind()
-	e2Kind := reflect.ValueOf(e2).Kind()
-	if e1Kind != e2Kind {
-		return Fail(t, "Elements should be the same type", msgAndArgs...)
-	}
-
-	res, isComparable := compare(e1, e2, e1Kind)
-	if !isComparable {
-		return Fail(t, fmt.Sprintf("Can not compare type \"%s\"", reflect.TypeOf(e1)), msgAndArgs...)
-	}
-
-	if res != -1 {
-		return Fail(t, fmt.Sprintf("\"%v\" is not greater than \"%v\"", e1, e2), msgAndArgs...)
-	}
-
-	return true
+	return compareTwoValues(t, e1, e2, []CompareType{compareGreater}, "\"%s\" is not greater than \"%s\"", msgAndArgs)
 }
 
 // GreaterOrEqual asserts that the first element is greater than or equal to the second
@@ -229,26 +218,7 @@ func Greater(t TestingT, e1 interface{}, e2 interface{}, msgAndArgs ...interface
 //    assert.GreaterOrEqual(t, "b", "a")
 //    assert.GreaterOrEqual(t, "b", "b")
 func GreaterOrEqual(t TestingT, e1 interface{}, e2 interface{}, msgAndArgs ...interface{}) bool {
-	if h, ok := t.(tHelper); ok {
-		h.Helper()
-	}
-
-	e1Kind := reflect.ValueOf(e1).Kind()
-	e2Kind := reflect.ValueOf(e2).Kind()
-	if e1Kind != e2Kind {
-		return Fail(t, "Elements should be the same type", msgAndArgs...)
-	}
-
-	res, isComparable := compare(e1, e2, e1Kind)
-	if !isComparable {
-		return Fail(t, fmt.Sprintf("Can not compare type \"%s\"", reflect.TypeOf(e1)), msgAndArgs...)
-	}
-
-	if res != -1 && res != 0 {
-		return Fail(t, fmt.Sprintf("\"%v\" is not greater than or equal to \"%v\"", e1, e2), msgAndArgs...)
-	}
-
-	return true
+	return compareTwoValues(t, e1, e2, []CompareType{compareGreater, compareEqual}, "\"%s\" is not greater than or equal to \"%s\"", msgAndArgs)
 }
 
 // Less asserts that the first element is less than the second
@@ -257,26 +227,7 @@ func GreaterOrEqual(t TestingT, e1 interface{}, e2 interface{}, msgAndArgs ...in
 //    assert.Less(t, float64(1), float64(2))
 //    assert.Less(t, "a", "b")
 func Less(t TestingT, e1 interface{}, e2 interface{}, msgAndArgs ...interface{}) bool {
-	if h, ok := t.(tHelper); ok {
-		h.Helper()
-	}
-
-	e1Kind := reflect.ValueOf(e1).Kind()
-	e2Kind := reflect.ValueOf(e2).Kind()
-	if e1Kind != e2Kind {
-		return Fail(t, "Elements should be the same type", msgAndArgs...)
-	}
-
-	res, isComparable := compare(e1, e2, e1Kind)
-	if !isComparable {
-		return Fail(t, fmt.Sprintf("Can not compare type \"%s\"", reflect.TypeOf(e1)), msgAndArgs...)
-	}
-
-	if res != 1 {
-		return Fail(t, fmt.Sprintf("\"%v\" is not less than \"%v\"", e1, e2), msgAndArgs...)
-	}
-
-	return true
+	return compareTwoValues(t, e1, e2, []CompareType{compareLess}, "\"%s\" is not less than \"%s\"", msgAndArgs)
 }
 
 // LessOrEqual asserts that the first element is less than or equal to the second
@@ -286,6 +237,10 @@ func Less(t TestingT, e1 interface{}, e2 interface{}, msgAndArgs ...interface{})
 //    assert.LessOrEqual(t, "a", "b")
 //    assert.LessOrEqual(t, "b", "b")
 func LessOrEqual(t TestingT, e1 interface{}, e2 interface{}, msgAndArgs ...interface{}) bool {
+	return compareTwoValues(t, e1, e2, []CompareType{compareLess, compareEqual}, "\"%s\" is not less than or equal to \"%s\"", msgAndArgs)
+}
+
+func compareTwoValues(t TestingT, e1 interface{}, e2 interface{}, allowedComparesResults []CompareType, failMessage string, msgAndArgs ...interface{}) bool {
 	if h, ok := t.(tHelper); ok {
 		h.Helper()
 	}
@@ -296,14 +251,24 @@ func LessOrEqual(t TestingT, e1 interface{}, e2 interface{}, msgAndArgs ...inter
 		return Fail(t, "Elements should be the same type", msgAndArgs...)
 	}
 
-	res, isComparable := compare(e1, e2, e1Kind)
+	compareResult, isComparable := compare(e1, e2, e1Kind)
 	if !isComparable {
 		return Fail(t, fmt.Sprintf("Can not compare type \"%s\"", reflect.TypeOf(e1)), msgAndArgs...)
 	}
 
-	if res != 1 && res != 0 {
-		return Fail(t, fmt.Sprintf("\"%v\" is not less than or equal to \"%v\"", e1, e2), msgAndArgs...)
+	if !containsValue(allowedComparesResults, compareResult) {
+		return Fail(t, fmt.Sprintf(failMessage, e1, e2), msgAndArgs...)
 	}
 
 	return true
 }
+
+func containsValue(values []CompareType, value CompareType) bool {
+	for _, v := range values {
+		if v == value {
+			return true
+		}
+	}
+
+	return false
+}


Test patch:
diff --git a/assert/assertion_order_test.go b/assert/assertion_order_test.go
index 4dd897a..daf9bea 100644
--- a/assert/assertion_order_test.go
+++ b/assert/assertion_order_test.go
@@ -29,7 +29,7 @@ func TestCompare(t *testing.T) {
 			t.Error("object should be comparable for type " + currCase.cType)
 		}
 
-		if resLess != 1 {
+		if resLess != compareLess {
 			t.Errorf("object less should be less than greater for type " + currCase.cType)
 		}
 
@@ -38,7 +38,7 @@ func TestCompare(t *testing.T) {
 			t.Error("object are comparable for type " + currCase.cType)
 		}
 
-		if resGreater != -1 {
+		if resGreater != compareGreater {
 			t.Errorf("object greater should be greater than less for type " + currCase.cType)
 		}
 
@@ -116,3 +116,76 @@ func TestLessOrEqual(t *testing.T) {
 		t.Error("Greater should return false")
 	}
 }
+
+func Test_compareTwoValuesDifferentValuesTypes(t *testing.T) {
+	mockT := new(testing.T)
+
+	for _, currCase := range []struct {
+		v1            interface{}
+		v2            interface{}
+		compareResult bool
+	}{
+		{v1: 123, v2: "abc"},
+		{v1: "abc", v2: 123456},
+		{v1: float64(12), v2: "123"},
+		{v1: "float(12)", v2: float64(1)},
+	} {
+		compareResult := compareTwoValues(mockT, currCase.v1, currCase.v2, []CompareType{compareLess, compareEqual, compareGreater}, "testFailMessage")
+		False(t, compareResult)
+	}
+}
+
+func Test_compareTwoValuesNotComparableValues(t *testing.T) {
+	mockT := new(testing.T)
+
+	type CompareStruct struct {
+	}
+
+	for _, currCase := range []struct {
+		v1 interface{}
+		v2 interface{}
+	}{
+		{v1: CompareStruct{}, v2: CompareStruct{}},
+		{v1: map[string]int{}, v2: map[string]int{}},
+		{v1: make([]int, 5, 5), v2: make([]int, 5, 5)},
+	} {
+		compareResult := compareTwoValues(mockT, currCase.v1, currCase.v2, []CompareType{compareLess, compareEqual, compareGreater}, "testFailMessage")
+		False(t, compareResult)
+	}
+}
+
+func Test_compareTwoValuesCorrectCompareResult(t *testing.T) {
+	mockT := new(testing.T)
+
+	for _, currCase := range []struct {
+		v1           interface{}
+		v2           interface{}
+		compareTypes []CompareType
+	}{
+		{v1: 1, v2: 2, compareTypes: []CompareType{compareLess}},
+		{v1: 1, v2: 2, compareTypes: []CompareType{compareLess, compareEqual}},
+		{v1: 2, v2: 2, compareTypes: []CompareType{compareGreater, compareEqual}},
+		{v1: 2, v2: 2, compareTypes: []CompareType{compareEqual}},
+		{v1: 2, v2: 1, compareTypes: []CompareType{compareEqual, compareGreater}},
+		{v1: 2, v2: 1, compareTypes: []CompareType{compareGreater}},
+	} {
+		compareResult := compareTwoValues(mockT, currCase.v1, currCase.v2, currCase.compareTypes, "testFailMessage")
+		True(t, compareResult)
+	}
+}
+
+func Test_containsValue(t *testing.T) {
+	for _, currCase := range []struct {
+		values []CompareType
+		value  CompareType
+		result bool
+	}{
+		{values: []CompareType{compareGreater}, value: compareGreater, result: true},
+		{values: []CompareType{compareGreater, compareLess}, value: compareGreater, result: true},
+		{values: []CompareType{compareGreater, compareLess}, value: compareLess, result: true},
+		{values: []CompareType{compareGreater, compareLess}, value: compareEqual, result: false},
+	} {
+		compareResult := containsValue(currCase.values, currCase.value)
+		Equal(t, currCase.result, compareResult)
+	}
+}