Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

new: fix algorithm for array compare #98

Merged
merged 1 commit into from
Sep 22, 2021
Merged

new: fix algorithm for array compare #98

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 47 additions & 58 deletions compare/compare.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@ import (
"fmt"
"reflect"
"regexp"
"sort"
"strings"

"github.com/fatih/color"
)
Expand All @@ -14,6 +12,7 @@ type CompareParams struct {
IgnoreValues bool
IgnoreArraysOrdering bool
DisallowExtraFields bool
failFast bool // End compare operation after first error
}

type leafsMatchType int
Expand Down Expand Up @@ -52,24 +51,26 @@ func compareBranch(path string, expected, actual interface{}, params *ComparePar

// compare arrays
if actualType == "array" {
if params.IgnoreArraysOrdering {
expected = sortArray(expected)
actual = sortArray(actual)
}

expectedRef := reflect.ValueOf(expected)
actualRef := reflect.ValueOf(actual)
expectedArray := convertToArray(expected)
actualArray := convertToArray(actual)

if expectedRef.Len() != actualRef.Len() {
errors = append(errors, makeError(path, "array lengths do not match", expectedRef.Len(), actualRef.Len()))
if len(expectedArray) != len(actualArray) {
errors = append(errors, makeError(path, "array lengths do not match", len(expectedArray), len(actualArray)))
return errors
}

if params.IgnoreArraysOrdering {
expectedArray, actualArray = getUnmatchedArrays(expectedArray, actualArray, params)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ОК, я вижу, что ты стараешься написать действительно хороший код, да еще и учесть мои комментарии. Как еще можно улучшить его - избавиться от двойного сравнения одного и того же: внутри getUnmatchedArrays ты вызываешь compareBranch "вхолостую" и потом здесь ниже в цикле вызывается compareBranch для тех же значений.

Мое предложение по объединению было - изменить вот этот цикл сравнения, чтобы он для одного из режимов сравнения действовал как сейчас, а для включенного IgnoreArraysOrdering действовал по твоему алгоритму, перебирая все элементы actual-массива, запоминая "использованные". Кстати - помечать использованные можно присваиванием nil-а такому элементу.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не, ну присвоить "nil" то мы точно не можем. Ничего не противоречит тому, чтобы у пользователя допускались значения "null" в массиве, поэтому и я не могу вводить такое ограничение. Не знаю зачем, но сервисов много, возможно какому-то такое понадобится.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

поправил. Добавил новый режим FailFast в режим сравнения (который завершает процедуру сравнения после первой ошибки), чтобы ускорить поиск пар при обработке массивов.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Но ведь это не то, что я имел в виду 😁

Кто будет выставлять этот параметр FailFast? Нужен ли он, если в тестах важна информативность (то есть, сообщение обо всех ошибках в данных, а не только о первой) больше скорости исполнения?

В общем, тут нужна помощь других контрибьютеров - возможно, я виноват, что завел тебя не туда.
@JustSkiv @What-If-I

Copy link
Contributor Author

@lansfy lansfy Sep 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Никто его не будет выставлять. Я его использую, чтобы при поиске пар в функции getUnmatchedArrays не делать полное сравнение, а переходить к следующему элементу сразу, как только понятно, что сравнение пофейлится. Благодаря этому поиск пар должен происходить намного быстрее. А вот уже когда будут найдены все несовпадающие элементы (после выхода из getUnmatchedArrays) к результату будет применено полное сравнение, так что внешне никаких изменений пользователи не заметят. Скрыл данный флаг, чтобы было понятно, что он для внутреннего использования.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Теперь все понятно.

}

// iterate over children
for i := 0; i < expectedRef.Len(); i++ {
for i, item := range expectedArray {
subPath := fmt.Sprintf("%s[%d]", path, i)
res := compareBranch(subPath, expectedRef.Index(i).Interface(), actualRef.Index(i).Interface(), params)
res := compareBranch(subPath, item, actualArray[i], params)
errors = append(errors, res...)
if params.failFast && len(errors) != 0 {
return errors
}
}
}

Expand All @@ -87,6 +88,9 @@ func compareBranch(path string, expected, actual interface{}, params *ComparePar
// check keys presence
if ok := actualRef.MapIndex(key); !ok.IsValid() {
errors = append(errors, makeError(path, "key is missing", key.String(), "<missing>"))
if params.failFast {
return errors
}
continue
}

Expand All @@ -99,6 +103,9 @@ func compareBranch(path string, expected, actual interface{}, params *ComparePar
params,
)
errors = append(errors, res...)
if params.failFast && len(errors) != 0 {
return errors
}
}
}

Expand Down Expand Up @@ -206,62 +213,44 @@ func makeError(path, msg string, expected, actual interface{}) error {
)
}

// Sort an array with respect of its elements of vary type.
func sortArray(array interface{}) interface{} {
func convertToArray(array interface{}) []interface{} {
ref := reflect.ValueOf(array)

interfaceSlice := make([]interface{}, 0)
for i := 0; i < ref.Len(); i++ {
interfaceSlice = append(interfaceSlice, ref.Index(i).Interface())
}

sort.Slice(interfaceSlice, func(i, j int) bool {
str1 := representAnythingAsString(interfaceSlice[i])
str2 := representAnythingAsString(interfaceSlice[j])
return strings.Compare(str1, str2) < 0
})

return interfaceSlice
}

func representAnythingAsString(value interface{}) string {
if value == nil {
return ""
}

valueType := getType(value)

if valueType == "array" {
// sort array
value = sortArray(value)
ref := reflect.ValueOf(value)

// represent array elements as a string
var stringChunks []string
for i := 0; i < ref.Len(); i++ {
stringChunks = append(stringChunks, representAnythingAsString(ref.Index(i).Interface()))
// For every elem in "expected" try to find elem in "actual". Returns arrays without matching.
func getUnmatchedArrays(expected, actual []interface{}, params *CompareParams) ([]interface{}, []interface{}) {
expectedError := make([]interface{}, 0)

failfastParams := *params
failfastParams.failFast = true

for _, expectedElem := range expected {
found := false
for i, actualElem := range actual {
if len(compareBranch("", expectedElem, actualElem, &failfastParams)) == 0 {
// expectedElem match actualElem
found = true
// remove actualElem from actual
if len(actual) != 1 {
actual[i] = actual[len(actual)-1]
}
actual = actual[:len(actual)-1]
break
}
}
return strings.Join(stringChunks, ".")
}

if valueType == "map" {
ref := reflect.ValueOf(value)

// sort keys ascending
mapKeys := ref.MapKeys()
sort.Slice(mapKeys, func(i, j int) bool {
return strings.Compare(mapKeys[i].String(), mapKeys[j].String()) < 0
})

// represent map keys & elements as a string
var stringChunks []string
for i := 0; i < len(mapKeys); i++ {
stringChunks = append(stringChunks, mapKeys[i].String())
stringChunks = append(stringChunks, representAnythingAsString(ref.MapIndex(mapKeys[i]).Interface()))
if !found {
expectedError = append(expectedError, expectedElem)
if params.failFast {
return expectedError, actual[0:1]
}
}
return strings.Join(stringChunks, ".")
}

// scalars
return fmt.Sprintf("%v", value)
return expectedError, actual
}
40 changes: 38 additions & 2 deletions compare/compare_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,10 +434,46 @@ func TestCompareDifferJsonScalars(t *testing.T) {
}
}

var expectedArrayJson = `
{
"data":[
{"name": "n111"},
{"name": "n222"},
{"name": "n333"}
]
}
`

var actualArrayJson = `
{
"data": [
{"message": "m555", "name": "n333"},
{"message": "m777", "name": "n111"},
{"message": "m999","name": "n222"}
]
}
`

func TestCompareEqualArraysWithIgnoreArraysOrdering(t *testing.T) {
var json1, json2 interface{}
json.Unmarshal([]byte(expectedArrayJson), &json1)
json.Unmarshal([]byte(actualArrayJson), &json2)
errors := Compare(json1, json2, CompareParams{
IgnoreArraysOrdering: true,
})
if len(errors) != 0 {
t.Error(
"must return no errors",
fmt.Sprintf("got result: %v", errors),
)
t.Fail()
}
}

func TestCompareEqualComplexJson(t *testing.T) {
var json1, json2 interface{}
json.Unmarshal([]byte(complexJson1), &json1)
json.Unmarshal([]byte(complexJson1), &json2)
json.Unmarshal([]byte(complexJson1), &json2) // compare json with same json
errors := Compare(json1, json2, CompareParams{})
if len(errors) != 0 {
t.Error(
Expand All @@ -459,7 +495,7 @@ func TestCompareDifferComplexJson(t *testing.T) {
"#/parameters/profile_id",
"#/parameters/profile_id2",
)
if errors[0].Error() != expectedErr {
if len(errors) == 0 || errors[0].Error() != expectedErr {
t.Error(
"must return one error",
fmt.Sprintf("got result: %v", errors),
Expand Down