new: fix algorithm for array compare #98
Conversation
compare/compare.go
Outdated
| } | ||
|
|
||
| // scalars | ||
| return fmt.Sprintf("%v", value) | ||
| expectedFound = append(expectedFound, expectedError...) |
There was a problem hiding this comment.
После всех этих манипуляций мы не только подготовили данные для сравнения, а уже можем сделать вывод о том, что массивы совпадают/не совпадают, поэтому уже надобность в последующей проверке отпадает.
Надо бы их совместить, как мне кажется.
There was a problem hiding this comment.
да, теперь из массива возвращаю только отличающиеся части
compare/compare.go
Outdated
| 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" | ||
| func matchArrays(expected, actual interface{}, params *CompareParams) (interface{}, interface{}) { |
There was a problem hiding this comment.
Там есть проверка на длины массивов. Можно вызывать этот метод после проверки длин, тогда он сильно упростится.
compare/compare.go
Outdated
| expectedFound = append(expectedFound, expectedElem) | ||
| actualFound = append(actualFound, actualElem) | ||
| // remove actualElem from actualArray | ||
| if len(actualArray) != 1 { |
There was a problem hiding this comment.
Вместо этой арифметики со слайсами в качестве результата можно просто собрать маппинг индексов:
индекс expected => соответствующий ему индекс actual
There was a problem hiding this comment.
да, но тогда надо будет хранить два маппинга, чтобы понимать какие элементы уже проверять не надо. Выйгрыш не очевиден - перестановка элементов достаточно дешевая операция.
There was a problem hiding this comment.
из функции возвращаю теперь только не совпадающие части. Заодно уменьшил число массивов до одного.
lansfy
force-pushed
the
fix_array_compare
branch
from
7d0e179 to
e68f8a6
Compare
| return errors | ||
| } | ||
|
|
||
| if params.IgnoreArraysOrdering { | ||
| expectedArray, actualArray = getUnmatchedArrays(expectedArray, actualArray, params) |
There was a problem hiding this comment.
ОК, я вижу, что ты стараешься написать действительно хороший код, да еще и учесть мои комментарии. Как еще можно улучшить его - избавиться от двойного сравнения одного и того же: внутри getUnmatchedArrays ты вызываешь compareBranch "вхолостую" и потом здесь ниже в цикле вызывается compareBranch для тех же значений.
Мое предложение по объединению было - изменить вот этот цикл сравнения, чтобы он для одного из режимов сравнения действовал как сейчас, а для включенного IgnoreArraysOrdering действовал по твоему алгоритму, перебирая все элементы actual-массива, запоминая "использованные". Кстати - помечать использованные можно присваиванием nil-а такому элементу.
There was a problem hiding this comment.
Не, ну присвоить "nil" то мы точно не можем. Ничего не противоречит тому, чтобы у пользователя допускались значения "null" в массиве, поэтому и я не могу вводить такое ограничение. Не знаю зачем, но сервисов много, возможно какому-то такое понадобится.
There was a problem hiding this comment.
поправил. Добавил новый режим FailFast в режим сравнения (который завершает процедуру сравнения после первой ошибки), чтобы ускорить поиск пар при обработке массивов.
There was a problem hiding this comment.
Но ведь это не то, что я имел в виду 😁
Кто будет выставлять этот параметр FailFast? Нужен ли он, если в тестах важна информативность (то есть, сообщение обо всех ошибках в данных, а не только о первой) больше скорости исполнения?
В общем, тут нужна помощь других контрибьютеров - возможно, я виноват, что завел тебя не туда.
@JustSkiv @What-If-I
There was a problem hiding this comment.
Никто его не будет выставлять. Я его использую, чтобы при поиске пар в функции getUnmatchedArrays не делать полное сравнение, а переходить к следующему элементу сразу, как только понятно, что сравнение пофейлится. Благодаря этому поиск пар должен происходить намного быстрее. А вот уже когда будут найдены все несовпадающие элементы (после выхода из getUnmatchedArrays) к результату будет применено полное сравнение, так что внешне никаких изменений пользователи не заметят. Скрыл данный флаг, чтобы было понятно, что он для внутреннего использования.
lansfy
force-pushed
the
fix_array_compare
branch
from
e68f8a6 to
291bdc4
Compare
lansfy
force-pushed
the
fix_array_compare
branch
from
291bdc4 to
1fbcb0d
Compare
| return errors | ||
| } | ||
|
|
||
| if params.IgnoreArraysOrdering { | ||
| expectedArray, actualArray = getUnmatchedArrays(expectedArray, actualArray, params) |
fetinin
added
bug
|
🚀 PR was released in |
Основная идея новой реализации: мы идем по массиву, который указан в тесте (expected) и ищем первый элемент из вывода сервиса (actual), который ему соответствует. Если такая пара найдена, то помещаем оба элемента в массив успешно связанных элементов (expectedFound и actualFound соответственно). Эту операцию проделываем для всех элементов из expected.
В конце операции все несвязанный элементы добавляем в конец expectedFound и actualFound. И эти два массива возвращаем из функции.