Медведев Фома, ДЗ по тестированию - #44
Conversation
…rtions.BeEquivalentTo() Переделал тест в NumberValidatorTests, сделав его через генераторы и имена для тестов, добавил своих
OvchinnikovNikita
left a comment
OvchinnikovNikita
left a comment
There was a problem hiding this comment.
Общий комментарий:
- Правок по задаче мало, но даже в таких случаях хорошим тоном было бы залить решение не одним коммитом, а, например, двумя: в одном изменения
ObjectComparison, в другом -NumberValidator. В следующих задачах давай делать коммиты более атомарными (но не переусердствуй с этим). - Продолжение пункта 1: сообщение к коммиту у тебя получилось довольно большое. Давай в будущем стараться делать их более строгими и лаконичными, но четко отражающими суть изменений (более атомарные коммиты поспособствуют этому). Я понимаю, что это похоже на урок русского языка в школе, но всё же такое оформление коммитов сильно упрощает другим разработчикам процесс погружения в контекст того, как шла разработка. Да, текущая задача маленькая, но представляешь, что может быть в реальных продуктовых проектах?)
| @@ -0,0 +1,13 @@ | |||
| # Default ignored files | |||
There was a problem hiding this comment.
Расскажи, что это за правки и зачем они нужны в рамках данной задачи?)
There was a problem hiding this comment.
При первом запуске Testing.sln через Rider появилась папка с этими файлами, надо было её в .gitignore положить, как папку .vs, упустил этот момент
There was a problem hiding this comment.
Сейчас можно так оставить, но в следующий раз старайся делать PR чистыми - без лишних изменений
| @@ -0,0 +1,8 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
Расскажи, что это за правки и зачем они нужны в рамках данной задачи?)
| @@ -0,0 +1,6 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
Расскажи, что это за правки и зачем они нужны в рамках данной задачи?)
| @@ -0,0 +1,8 @@ | |||
| <wpf:ResourceDictionary xml:space="preserve" xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:s="clr-namespace:System;assembly=mscorlib" xmlns:ss="urn:shemas-jetbrains-com:settings-storage-xaml" xmlns:wpf="http://schemas.microsoft.com/winfx/2006/xaml/presentation"> | |||
There was a problem hiding this comment.
Расскажи, что это за правки и зачем они нужны в рамках данной задачи?)
| new Person("Vasili III of Russia", 28, 170, 60, 99999999, null)); | ||
|
|
||
| actualTsar.Should().BeEquivalentTo(expectedTsar, config => config.Excluding(x => x.Id).Excluding(x => x.Parent.Id)); | ||
| // Я специально для теста добавил одно поле, а сам тест остался неизменен. По сути, его нужно изменять только в случае, если нужно добавить какое-то поле в игнор |
There was a problem hiding this comment.
На будущее отмечу: в рамках учебных заданий это ОК, но в реальных проектах комментарии, адресованные ревьюеру, обычно оставляют в github/gitlab и т.д.
There was a problem hiding this comment.
А как следует оставлять комментарии в github? В pull request их добавлять, или как-то иначе?
There was a problem hiding this comment.
В pull request. При этом оставлять поясняющие комментарии в коде - это нормальная практика, если ты хочешь, чтобы этот комментарий остался в коде "навсегда" и предназначался для всех разработчиков, с которыми ты работаешь над одним кодом. В нашем же случае ты оставлял комментарий для наставника - просто, чтобы я его увидел и имел ввиду. Такое лучше не оставлять в коде, а выносить в pull request (или merge request в случае с gitlab, с которым если ещё не сталкивался, то обязательно столкнешься)
| var expectedTsar = new Person("Ivan IV The Terrible", 54, 170, 70, 99999999, | ||
| new Person("Vasili III of Russia", 28, 170, 60, 99999999, null)); | ||
|
|
||
| actualTsar.Should().BeEquivalentTo(expectedTsar, config => config.Excluding(x => x.Id).Excluding(x => x.Parent.Id)); |
There was a problem hiding this comment.
- Строчка довольно длинная. Как думаешь, можно ли её отформатировать и сделать эту строчку более читаемой?
- Мелочь, но всё же: точно ли
x- подходящее название переменной в данном контексте? Может можно сделать лучше?
| new Person("Vasili III of Russia", 28, 170, 60, 99999999, null)); | ||
|
|
||
| // Какие недостатки у такого подхода? | ||
| // Этот код более читаем, чем был изначальный вариант в первом тесте, но его необходимо будет изменять каждый раз, когда мы будем менять класс, который тестируем |
There was a problem hiding this comment.
Давай ещё раз проведём сравнение и аккуратно, но развернуто распишем (оставь комментарий здесь или в коде):
- В чём плюсы/минусы изначального подхода в тесте CheckCurrentTsar?
- В чём плюсы/минусы альтернативного решения в CheckCurrentTsar_WithCustomEquality?
- В чём плюсы/минусы твоего решения?
There was a problem hiding this comment.
- Плюсом оригинального подхода можно назвать то, что такой тест можно быстро написать просто для того, чтобы оперативно и один раз убедиться, что что-то работает. Его проблема в том, что такой тест очень сложно поддерживать: если вдруг мы будем менять поля тестируемого класса, то нам придётся вручную добавлять эти поля в тесте. К тому же, такой тест не очень удобно читается из-за большого количества сравнений.
- Альтернативное решение оставляет в себе проблему расширяемости теста, поэтому нам также, как и раньше, придётся вручную добавлять или убирать поля при изменении тестируемого класса, но он более читаемый, потому что сравнение вынесено в отдельный метод. К тому же в этом решении сравнение вложенных экземпляров класса происходит через AreEqual, а не по каждому полю как в 1 варианте, что тоже явно плюс.
- В моём решении сравнение в тесте происходит при помощи метода из FluentAssertions, в котором объекты автоматически сравниваются по всем полям, поэтому тест будет работать даже если изменить поля тестируемых классов. Единственный минус - необходимость исключать те поля, которые будут уникальными для каждого объекта, чтобы сравнение их пропускало.
2 participants
Изменил тест в ObjectComparison, реализовав проверку через FluentAssertions.BeEquivalentTo()
Переделал тест в NumberValidatorTests, сделав его через генераторы и имена для тестов, добавил своих
@OvchinnikovNikita