Попов Захар

[BlizPerfect]

@Folleach

[Folleach]

Просто рекомендация к PR

Это не нужно править

Сейчас у тебя Pull Request создан из ветки master твоего репозитория, в целом допустимо, но тем самым могут возникнуть проблемы при сихронизации ветки master, когда будешь делать pull из основного репозитория.
Если в основном репозитории появились новые коммиты, то в твоей ветке master будет создан merge commit, который появится в этом PR. Таким образом, пока активен PR, не будет возможности что-то сделать с master веткой форка.

Из этих соображений, лучше создавать в форке отдельную ветку для Pull Request. Это позволит легко вносить изменения в PR и не получать нежелательных конфликтов. Будут только желательные конфликты, хех :)

[Folleach]

Может быть есть ещё недостатки?

[BlizPerfect]

Предположу, что так же недостатком является то, что данный метод является рекурсивным и возможна ситуация, при которой цепочка из царей превысит глубину рекурсии. Знаю, что это одна из проблем питона - малая глубина рекурсии по умолчанию. Шарп вроде за таким не замечен. Так же будет забиваться стек вызовов методов. Будут накладные расходы на постановку/снятие контекста метода в/из стек вызовов.

Я сам вообще не люблю писать рекурсивные методы, хоть они и придают порой элегантность коду. В подавляющем большинстве случаем пишу с использованием цикла. Мне кажется это нагляднее.

[Folleach]

Действительно, для большой вложенности могут возникнуть проблемы с конечным стеком потока. Но лишино ли решение с Fluent Assertions такой проблемы? Там точно цикл внутри а не подобная рекурсия?

Кроме того, конкретно при тестировании прошлая запись тестов имеет ряд других, более значимых недостатков, хотелось бы услышать их

[BlizPerfect]

Если я всё правильно понял, то исходя из документации, решение с Fluent Assertions на самом деле так же рекурсивно. По умолчанию глубина рекурсии выставлена как 10.

The comparison is recursive by default. To avoid infinite recursion, Fluent Assertions will recurse up to 10 levels deep by default, but if you want to force it to go as deep as possible, use the AllowingInfiniteRecursion option.

По недостаткам решения, могу ещё предложить следующее:

1. Нет конкретики, какое поле оказалось разным, просто сообщается о том, что цари различаются.
2. Если мы передаём оба царя как null, тогда цари считаются равными. Равны ли цари тогда? Философвски как-то.

[Folleach]

Ага, в прошлом решении, если произойдёт ошибка, то по ней будет не понятно, что было причиной различия и придётся дебажить каждый упавший тест отдельно - это муторно.

Второй пункт не совсем понял, мы всегда ожидаем какого-то царя. Если ожидаем null, вероятно так необходимо. Например, если в базе данных мы не ожидаем увидеть такого-то царя, то он может быть null ;)

[BlizPerfect]

Про второй пункт, мне это просто кажется логически неверно - нам пришли вообще не цари, оба null. Тогда тест вернёт true. Не цари равны.
Наверное я тут больше копаю в ту сторону, что до AreEqual не должно вообще доходить null, это должно отлавливаться заранее.

[Folleach]

Про второй пункт, мне это просто кажется логически неверно - нам пришли вообще не цари, оба null. Тогда тест вернёт true. Не цари равны.

Но ведь в тестах у нас есть два значения:

• actual - значение, которое мы получили из базы данных/внешнего API/метода
• expected - константа в тесте, которую ожидалась увидеть.

В этом плане не понятно, что такое "пришли не цари". Так как один параметр контролируется тестом.

Если имелось ввиду, что вместо царя со значением null пришла королева со значением null, то это не допускается статической типизацией языка :)

А также вопрос, разве сейчас два null сравниваются по-другому через Fluent Assertions?

[BlizPerfect]

Да, что-то я действительно тут переусложнил - мне это свойственно!