Conversation
SquirrelLeonid
left a comment
SquirrelLeonid
left a comment
There was a problem hiding this comment.
Здесь оставлю общий комментарий
-
Правок в задаче немного, но тем не менее было бы хорошо оформить в виде двух отдельных коммитов. Коммит стоит рассматривать как законченную мысль. Это как выполнить маленький шаг из большой задачи. В данном случае точно можно было разделить домашку на две задачи: ObjectComparison и NumberValidator. Дробить можно и дальше, но слишком мелкие шаги тоже не стоит делать, чтобы не засорять историю в гите (или другой системе контроля версий)
-
Сообщения в коммитах стоит делать более информативными (но не длинными).
К примеру, в первой строке можно коротко описать основной шаг, а в последующих строках по пунктам тезисно указать подробности. При внесени правок как раз будет возможность потренироваться :) -
Проверь, что код отформатирован с помощью Resharper
|
|
||
| public class ObjectComparison | ||
| { | ||
| [Test] |
There was a problem hiding this comment.
Оставлю комментарий здесь, т.к. github не дает мне выбрать неизмененные строки кода
Обычно имена тестов следуют некоторым правилам (соглашениям). В разных компаниях (да и командах внутри компании) они могут различаться. Например, один из способов именования такой: <ClassName>_<ShouldBe...>_[When...]_Test.
В данном случае можно ограничиться добавлением суффикса _Test в конец имени тестов. Так мы явно выделим эти методы от "обычных" методов.
К слову, почти такой же способ ты использовал в NumberValidatorTests
There was a problem hiding this comment.
поменял названия, которые совсем не подходили по структуре
обновил названия, которые не полностью совпадали со структурой
| @@ -14,16 +16,37 @@ public void CheckCurrentTsar() | |||
| var expectedTsar = new Person("Ivan IV The Terrible", 54, 170, 70, | |||
There was a problem hiding this comment.
В способе получения expextedTsar есть две проблемы:
- Код в тестах CheckCurrentTsar_WithCustomEquality и CheckCurrentTsar дублируется. Если мы захотим изменить ожидаемого царя, то изменения придется внести в двух местах. Это повышает риск ошибки.
- При создании expectedTsar (да и в методе TsarRegistry.GetCurrentTsar) читаемость кода снижается при указании родителя в параметрах. Легко может возникнуть вопрос: внутренний объект Person - это родитель или потомок? Для ответа надо смотреть в конструктор класса. В данном случае для повышения читаемости стоит вынести создание родителя в отдельную строку. Тогда о назначении объекта будет говорить имя переменной, в которую он записан.
There was a problem hiding this comment.
- вынес в метод-хелпер в файле с тестами получение текущего предполагаемого царя
- объявляю переменную с родителем в отдельной строке
There was a problem hiding this comment.
Ага, именно то, что хотелось увидеть здесь
| actualTsar.Should().BeEquivalentTo(expectedTsar, config => config | ||
| .Excluding(x => x.Id) | ||
| .Excluding(x => x.Parent.Id)); |
There was a problem hiding this comment.
Решение рабочее и, формально, удовлетворяет требованиям в задаче. Нам действительно нужно писать меньше кода при расширении Person. Но давай подумаем о двух моментах:
- Сейчас тест упадет, если мы начнем указывать внуков (или дедов). Подумай, как этого можно избежать.
- Сейчас мы никак не обрабатываем циклические ссылки.
Подумай, как это можно сделать.
Подсказка: решение есть в рамках fluentAssertions
There was a problem hiding this comment.
- добавил решение через перебор Id, а не конкретных переменных
- добавил .IgnoringCyclicReferences() из FluentAssertions
| public void Constructor_ThrowsException_OnInvalidArguments(int precision, int scale) | ||
| { | ||
| Action constructor = () => new NumberValidator(precision, scale); | ||
| constructor.Should().Throw<ArgumentException>(); |
There was a problem hiding this comment.
В целом нам может быть достаточно, что конструктор кидает ArgumentException. Однако часто в тестах важно отследить, что проверяемый код кидает не только исключение определенного типа, но и с определенным сообщением. Это будет являться дополнительной гарантией, что код делает то, что от него ожидается.
Здесь стоит сделать тест более информативным в этом плане.
There was a problem hiding this comment.
добавил expectedMessage в Testcase по тем, что есть в конструкторе, с помощью WithMessage() и маски, чтобы не хранить полные сообщения
There was a problem hiding this comment.
Маска действительно позволяет хранить меньше, но в тестах все таки важна конкретика.
К примеру, если конструктор вместо "precision must be a positive number" начнет выдавать "precision must NOT BE a positive number", то тест все равно пройдет. Хотя по хорошему он должен упасть.
Т.е. в данном случае нам наоборот стоит хранить полный текст сообщения - при любом его изменении тест поможет нам отловить это.
На зачет задачи это не повлияет, но можешь залить соответствующую правку
| [TestCase(1, 0, TestName = "Precision and scale are minimum possible")] | ||
| [TestCase(1000, 500, TestName = "Precision and scale are big enough")] | ||
| public void Constructor_DoesNotThrowException_OnValidArguments(int precision, int scale) | ||
| { | ||
| Action constructor = () => new NumberValidator(precision, scale, true); | ||
| constructor.Should().NotThrow(); | ||
| } |
There was a problem hiding this comment.
С одной стороны, под рефакторингом понимается изменение кода без изменения его поведения. С этой точки зрения тесты действительно должны быть - ведь они были в исходной версии.
Но с другой стороны - какую пользу дает этот набор тестов сравнительно с тестами Constructor_ThrowsException_OnInvalidArguments?
Т.е. мы очевидно ожидаем, что конструктор не будет кидать исключение при корректных параметрах. Вместе с этим мы описали некорректные параметры в пессимистичном тесте Constructor_ThrowsException_OnInvalidArguments. Я не предлагаю снести эти тесты - этот комментарий тебе просто на обдумывание
There was a problem hiding this comment.
комментарий обработал, не стал удалять, сделал логику теста в том, что он проверяет какие-то граничные значения, вместо обычного пессимистичного теста
There was a problem hiding this comment.
Это хороший подход
Граничные случаи - достаточно частое место для ошибок
| [TestCase(6, 0, "1234", TestName = "Simple integer")] | ||
| [TestCase(20, 10, "123.54", TestName = "Number with fraction part")] | ||
| public void Validator_ReturnsTrue_WithProperNumbers(int precision, int scale, string input) |
There was a problem hiding this comment.
Здесь проверяем оптимистичный сценарий, когда число указано корректно. Но у читателя может возникнуть вполне закономерный вопрос - почему выбраны именно такие значения для precision и scale в тестах? В данном случае стоит указать минимально допустимые значения для precision и scale в тест-кейсах.
There was a problem hiding this comment.
изначально думал сделать такие "рандомные" числа для того, чтобы не вызывать падение теста на каких-то недочетах (не серьезных проблем) при работе со scale и precision, поскольку для этого были предназначены другие тесты, но понял, что тогда и смысл этого теста в целом теряется, поправил
There was a problem hiding this comment.
Я на практике ни разу не видел, чтобы для теста использовались случайные числа.
Наверняка есть ситуации, когда именно этот подход применим, но в моем понимании тесты должны быть, скажем так, стабильны
| [TestCase("1234567", TestName = "Precision overflow with numerals")] | ||
| [TestCase("0000000", TestName = "Precision overflow with zeros")] | ||
| public void Validator_ReturnsFalse_WithPrecisionOverflow(string input) |
There was a problem hiding this comment.
В чем то похоже на предыдущий комментарий.
Можно обойтись более короткими числами и меньшим значением для precision. В этом тесте ключевым фактом для нас должно быть то, что количество знаков в числе больше значения precision
| } | ||
|
|
||
| [TestCase] | ||
| public void Validator_ReturnsTrue_WithCommaSeparator() |
There was a problem hiding this comment.
А только ли запятая у нас бывает разделителем?
There was a problem hiding this comment.
нет, может быть еще точка, но предполагалось, что точка уже везде используется и какой-то тест точно упадет, но кажется ошибку свою понял, добавил очевидную проверку на точку в этом же тесте, чтобы можно было понять, что именно с ней есть проблемы
There was a problem hiding this comment.
Ага
Тесты часто пересекаются в чем то. Примеры выше уже захватывают точку и, как бы, неявно проверяют корректность ее обработки.
Здесь важно различать что каждый конкретный тест проверяет. Этот вот прям направлен на разделитель дробной части. По факту он же может служить документацией по допустимым разделителям
|
Задачу засчитываю на полный балл. |
2 participants
No description provided.