Минеев Максим #2
Conversation
…дготовку данных в отдельные методы
…ишних проверок, добавление недостающих
| actualTsar = TsarRegistry.GetCurrentTsar(); | ||
| expectedTsar = CreateDefaultTsar(); | ||
| } | ||
|
|
||
| [Test] | ||
| [Description("Проверка текущего царя")] | ||
| [Category("ToRefactor")] | ||
| public void CheckCurrentTsar() |
There was a problem hiding this comment.
хочется более подробного названия, не совсем понятно что именно проверяется у текущего царя. Я за нейминг типа GetCurrentTsar_ShouldReturn_ValidTsar
| actualTsar = TsarRegistry.GetCurrentTsar(); | ||
| expectedTsar = CreateDefaultTsar(); | ||
| } | ||
|
|
||
| [Test] | ||
| [Description("Проверка текущего царя")] | ||
| [Category("ToRefactor")] |
| actualTsar.Parent.Name.Should().Be(expectedTsar.Parent.Name); | ||
| actualTsar.Parent.Age.Should().Be(expectedTsar.Parent.Age); | ||
| actualTsar.Parent.Height.Should().Be(expectedTsar.Parent.Height); | ||
| actualTsar.Parent.Parent.Should().Be(expectedTsar.Parent.Parent); |
There was a problem hiding this comment.
Реализовать IEqutable и сравнивать с помощью Should().BeEquivalentTo возможно будет проще. В таком случае при расширении объекта person не придется редактировать тесты, только правила сравнения объектов (кажется, этого в любом случае не избежать). Ошибка тоже будет информативная
There was a problem hiding this comment.
Возможно, я не до конца понял задание, в первом тесте переписал просто каждую строчку на "более читаемый" FluentAssertions, не меняя логики и структуры теста, а второй переписал на свое "более удачное" решение. Если я сейчас перепишу первый тест с помощью Should().BeEquivalentTo(...), то у меня получится просто два одинаковых теста. Мне надо было только переписать первый, а второй оставить как есть (оставив комментарии)?
There was a problem hiding this comment.
Ага, переписать первый
Переписал
| public void NumberValidator_Throws_WhenNegativePrecision() | ||
| { | ||
| Action action = () => new NumberValidator(-1, 2, true); | ||
| action.Should().Throw<ArgumentException>(); |
There was a problem hiding this comment.
можно еще текст ошибки проверить (в следующих 3 тоже)
| public void NumberValidator_Throws_WhenScaleEqualPrecision() | ||
| { | ||
| Action action = () => new NumberValidator(2, 2, true); | ||
| action.Should().Throw<ArgumentException>(); |
There was a problem hiding this comment.
а вообще эти тесты можно объединить и сделать testCases. По сути все тесты должны проверить что выбрасывается ArgumentException с нужным текстом при невалидных значениях
| public void NumberValidator_DontThrows_WhenScaleLessThanPrecision() | ||
| { | ||
| Action action = () => new NumberValidator(2, 1, true); | ||
| action.Should().NotThrow<Exception>(); |
| [TestCase(3, 1, false, "-3.21")] | ||
| [TestCase(2, 1, false, "-1.1")] | ||
| public void NumberValidator_ReturnsFalse_WithInvalidFloatNumbers(int precision, int scale, bool onlyPositive, | ||
| string number) => TestWithInvalidParameters(precision, scale, onlyPositive, number); |
There was a problem hiding this comment.
Давай объединим все методы которые используют TestWithInvalidParameters.Тогда необходимости выделять отдельный метод не будет. В них один и тот же код, разница только в testCases и названиях. Давай назовем тест согласно тому что он делает (возвращает false когда параметры некорректны). И в тесткейс добавим TestName для описания того что проверяется в данном случае (с TestWithValidParameters по аналогии). На мой взгляд так будет намного читабельнее
|
|
||
| [TestCase(2, 1, true, "1.12")] | ||
| [TestCase(4, 3, true, "2.2112")] | ||
| [TestCase(5, 1, true, "51.43")] |
There was a problem hiding this comment.
Мне кажется что есть дублирующие проверки, например, почему недостаточно проверить 1 из этих случаев? [TestCase(2, 1, true, "1.12")] [TestCase(4, 3, true, "2.2112")] [TestCase(5, 1, true, "51.43")]
Если необходимости нет, то давай удалим лишнее (и в других тестах)
| .Where(field => field.Name != nameof(Id)) | ||
| .Where(field => field.GetValue(this) != null || field.GetValue(other) != null) | ||
| .All(field => field.GetValue(this) != null && field.GetValue(this)!.Equals(field.GetValue(other))); | ||
|
|
There was a problem hiding this comment.
P.S. из минусов такого подхода: если в Person добавить хотя бы одно свойство, то код сравнения придется модернизировать (возможно неочевидным образом). Если работаем только с полями и объектами типов, содержащих в себе только поля, то все ок, но если попадаются любые свойства, то они будут проигнорированы. В общем, скорее всего нам все равно придется рано или поздно касаться этого кода и что-то менять, но кажется, этого в любом случае не избежать
Ну и вывод об ошибке хоть и достаточно полный, но не сразу видно, где именно проблема
Пример: я просто добавил пробел в имя, в глаза это не бросается
Expected actualTsar to be HomeExercise.Tasks.ObjectComparison.Person
{
Age = 54,
Height = 170,
Id = 7,
Name = "Ivan IV The Terrible",
Parent = HomeExercise.Tasks.ObjectComparison.Person
{
Age = 28,
Height = 170,
Id = 6,
Name = "Vasili III of Russia",
Parent = <null>,
Weight = 60
},
Weight = 70
}, but found HomeExercise.Tasks.ObjectComparison.Person
{
Age = 54,
Height = 170,
Id = 5,
Name = "Ivan IV The Terrible ",
Parent = HomeExercise.Tasks.ObjectComparison.Person
{
Age = 28,
Height = 170,
Id = 4,
Name = "Vasili III of Russia",
Parent = <null>,
Weight = 60
},
Weight = 70
}
There was a problem hiding this comment.
P.S.S. Конечно, думая о будущем, можно было бы сразу добавить сравнение свойств, тогда метод получился бы полноценно универсальным, но пока что нам этого не надо и мы можем придерживаться принципа YAGNI и не реализовывать избыточную на данный момент функциональность
@masssha1308