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.

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

Волков Кирилл #236

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

caimanchik
Copy link

Comment on lines 28 to 30
.Excluding(p => p.Parent!.Id)
.Excluding(p => p.Parent!.Weight)
.Excluding(p => p.Parent!.Parent)

Choose a reason for hiding this comment

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

Объясни почему выбрал именно такие проверки.

Choose a reason for hiding this comment

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

Почему именно такая просьба, потому что если сравнить 2 этих теста, то можно увидеть что у них разная логика сравнения

Copy link
Author

Choose a reason for hiding this comment

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

.Excluding(p => p.Parent!.Id) - данное поле у нас статический автоиндекс, как в бдшках. Его нужно исключить, одинаковые цари будут падать на нем, потому что это уникальный идентификатор

.Excluding(p => p.Parent!.Weight) - данной проверки не было в тесте, который был до рефакторинга. И поэтому решил исключить

.Excluding(p => p.Parent!.Parent) - если не исключить данное поле, то нужно будет бесконечно дописывать игнорирование поля Id в родителях. А так как мы не знаем, сколько их у нас, поэтому исключил. Также при написании теста я руководствовался логикой, что мы проверяем именно данного царя/персону, а падение теста при проверке его прародителей в данном случае будет являться неожиданным поведением

Также, если посмотреть на метод CheckCurrentTsar_WithCustomEquality, то можно заметить рекурсивный вызов, который может сильно замедлять время работы теста. И опять же действия, которые мы выполняем при рекурсии не соответствуют названию теста

Choose a reason for hiding this comment

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

.Excluding(p => p.Parent!.Weight) - данной проверки не было в тесте, который был до рефакторинга. И поэтому решил исключить

Иногда когда ты приходишь рефачить тесты бывают баги) Я с точки зрения не вижу почему у родителя должен отличаться Weight

Также, если посмотреть на метод CheckCurrentTsar_WithCustomEquality, то можно заметить рекурсивный вызов, который может сильно замедлять время работы теста. И опять же действия, которые мы выполняем при рекурсии не соответствуют названию теста

не согласен с этим пунктом, т.к. мы если у нас будет бага на 4 или допустим 100 родителе, то у нас тест не будет выполнят свою функцию отлавливать баги

.Excluding(p => p.Parent!.Parent) - если не исключить данное поле, то нужно будет бесконечно дописывать игнорирование поля Id в родителях. А так как мы не знаем, сколько их у нас, поэтому исключил. Также при написании теста я руководствовался логикой, что мы проверяем именно данного царя/персону, а падение теста при проверке его прародителей в данном случае будет являться неожиданным поведением

попробуй посмотреть перегрузку метода Excluding

Comment on lines 105 to 107
new Func<NumberValidator>(() => new NumberValidator(precision, scale, onlyPositive))
.Should()
.ThrowExactly<ArgumentException>();

Choose a reason for hiding this comment

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

Бывают случаи когда тебе важна не факт ошибки в тесте, но и текст этой ошибки, допустим когда этот текст выводиться клиенту

Comment on lines 107 to 108
.ThrowExactly<ArgumentException>()
.Where(e => e.Message.Contains("precision"));

Choose a reason for hiding this comment

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

на самом делал тут тебе просто повезло, что Message содержится в обоих ошибках. Я бы явно зафиксировал текс ошибки, и там в кейсах не хватает с нулем

refactored message checking on constructor fails
switched to another excluding overload to handle Id in Person
.Excluding(p => p.Parent!.Id)
.Excluding(p => p.Parent!.Weight)
.Excluding(p => p.Parent!.Parent)
.Excluding(memberInfo => memberInfo.SelectedMemberInfo.Name == "Id")

Choose a reason for hiding this comment

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

Давай представим:

  1. что мы добавим кота, кото который будет жить вечность, и у него будет всегда будет одинаковый Id
  2. мы переименуем поле Id

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants