Минеев Максим #2
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,52 +1,59 @@ | ||
| using FluentAssertions; | ||
| using NUnit.Framework; | ||
|
|
||
| namespace HomeExercise.Tasks.ObjectComparison; | ||
|
|
||
| public class ObjectComparison | ||
| { | ||
| private Person actualTsar; | ||
| private Person expectedTsar; | ||
|
|
||
| [SetUp] | ||
| public void Setup() | ||
| { | ||
| actualTsar = TsarRegistry.GetCurrentTsar(); | ||
| expectedTsar = CreateDefaultTsar(); | ||
| } | ||
|
|
||
| [Test] | ||
| [Description("Проверка текущего царя")] | ||
| [Category("ToRefactor")] | ||
| public void CheckCurrentTsar() | ||
|
There was a problem hiding this comment. хочется более подробного названия, не совсем понятно что именно проверяется у текущего царя. Я за нейминг типа GetCurrentTsar_ShouldReturn_ValidTsar |
||
| { | ||
| actualTsar.Name.Should().Be(expectedTsar.Name); | ||
| actualTsar.Age.Should().Be(expectedTsar.Age); | ||
| actualTsar.Height.Should().Be(expectedTsar.Height); | ||
| actualTsar.Weight.Should().Be(expectedTsar.Weight); | ||
|
|
||
| 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, не меняя логики и структуры теста, а второй переписал на свое "более удачное" решение. Если я сейчас перепишу первый тест с помощью There was a problem hiding this comment.
Переписал |
||
| } | ||
|
|
||
| [Test] | ||
| [Description("Альтернативное решение. Какие у него недостатки?")] | ||
| public void CheckCurrentTsar_WithCustomEquality() | ||
| { | ||
| // Какие недостатки у такого подхода? | ||
| // AreEqual не содержит в себе Assert, при падении теста мы не знаем, что произошло (неинформативный вывод) | ||
| // При расширении класса Person, нам необходимо менять код уже написанного метода AreEqual | ||
| // и компилятор нам не подскажет, что это нужно сделать | ||
| // В тесте только вызывается метод AreEqual, приходится писать инфраструктурный код для его создания | ||
|
|
||
| // Плюсы моего подхода: | ||
| // При расширении класса Person, программа не скомпилируется, если не поменять код (предотвращает ошибки). | ||
| // Код приходится менять только в генераторах объектов | ||
| // При расширении класса Person, нам не нужно изменять тест (первый тест придется, компилятор не подскажет) | ||
| // Логика сравнения перенесена в сам тест, не приходится писать лишний инфраструктурный код | ||
| // P.S. создание объектов вынесено в Setup. Мы не сравниваем Id, создавать каждый раз новые данные для теста нет необходимости | ||
| actualTsar.Should().BeEquivalentTo(expectedTsar, options => | ||
| options.Excluding(y => y.Id).Excluding(y => y.Parent.Id)); | ||
| } | ||
|
|
||
| private static Person CreateDefaultTsar() | ||
| => new("Ivan IV The Terrible", 54, 170, 70, CreateDefaultParent()); | ||
|
|
||
| private static Person CreateDefaultParent() | ||
| => new("Vasili III of Russia", 28, 170, 60, null); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,31 +1,145 @@ | ||
| using FluentAssertions; | ||
| using NUnit.Framework; | ||
|
|
||
| namespace HomeExercise.Tasks.NumberValidator; | ||
|
|
||
| [TestFixture] | ||
| public class NumberValidatorTests | ||
| { | ||
| [Test] | ||
| public void NumberValidator_Throws_WhenNegativePrecision() | ||
| { | ||
| Action action = () => new NumberValidator(-1, 2, true); | ||
| action.Should().Throw<ArgumentException>(); | ||
|
There was a problem hiding this comment. можно еще текст ошибки проверить (в следующих 3 тоже) |
||
| } | ||
|
|
||
| [Test] | ||
| public void NumberValidator_Throws_WhenNegativeScale() | ||
| { | ||
| Action action = () => new NumberValidator(2, -1, true); | ||
| action.Should().Throw<ArgumentException>(); | ||
| } | ||
|
|
||
| [Test] | ||
| public void NumberValidator_Throws_WhenScaleGreaterThanPrecision() | ||
| { | ||
| Action action = () => new NumberValidator(2, 3, true); | ||
| action.Should().Throw<ArgumentException>(); | ||
| } | ||
|
|
||
| [Test] | ||
| 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 с нужным текстом при невалидных значениях |
||
| } | ||
|
|
||
| [Test] | ||
| public void NumberValidator_DontThrows_WhenPrecisionIsPositive() | ||
| { | ||
| Action action = () => new NumberValidator(2, 1, true); | ||
| action.Should().NotThrow<Exception>(); | ||
| } | ||
|
|
||
| [Test] | ||
| public void NumberValidator_DontThrows_WhenScaleLessThanPrecision() | ||
| { | ||
| Action action = () => new NumberValidator(2, 1, true); | ||
| action.Should().NotThrow<Exception>(); | ||
|
|
||
| } | ||
|
|
||
| [TestCase(1, true, "1")] | ||
| [TestCase(2, true, "21")] | ||
| [TestCase(3, true, "13")] | ||
| [TestCase(5, true, "514")] | ||
| [TestCase(10, false, "-314")] | ||
| [TestCase(2, false, "-3")] | ||
| [TestCase(3, false, "-99")] | ||
| public void NumberValidator_ReturnsTrue_WithValidIntegerNumbers_WithScaleZero(int precision, bool onlyPositive, | ||
| string number) => TestWithValidParameters(precision, 0, onlyPositive, number); | ||
|
|
||
| [TestCase(1, true, "11")] | ||
| [TestCase(2, true, "001")] | ||
| [TestCase(3, true, "4134124")] | ||
| [TestCase(10, false, "-031442424324243")] | ||
| [TestCase(2, false, "-13")] | ||
| [TestCase(3, false, "-993")] | ||
| public void NumberValidator_ReturnsFalse_WithInvalidIntegerNumbers_WithScaleZero(int precision, | ||
| bool onlyPositive, string number) => TestWithInvalidParameters(precision, 0, onlyPositive, number); | ||
|
|
||
| [TestCase(2, 1, true, "1.1")] | ||
| [TestCase(4, 3, true, "2.21")] | ||
| [TestCase(4, 3, true, "0.000")] | ||
| [TestCase(4, 3, true, "00.00")] | ||
| [TestCase(5, 1, true, "51.4")] | ||
| [TestCase(10, 5, false, "-31.414")] | ||
| [TestCase(3, 1, false, "-3.2")] | ||
| [TestCase(2, 1, false, "-1")] | ||
| public void NumberValidator_ReturnsTrue_WithValidFloatNumbers(int precision, int scale, bool onlyPositive, | ||
| string number) => TestWithValidParameters(precision, scale, onlyPositive, number); | ||
|
|
||
| [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(10, 5, false, "-31.41431231")] | ||
| [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("a")] | ||
| [TestCase("seven")] | ||
| [TestCase("one.five")] | ||
| [TestCase(" ")] | ||
| [TestCase("")] | ||
| [TestCase(null)] | ||
| public void NumberValidator_ReturnsFalse_WithNonNumber(string number) | ||
| => TestWithInvalidParameters(5, 4, false, number); | ||
|
|
||
| [TestCase("IV")] | ||
| [TestCase("1 . 1")] | ||
| [TestCase("1. 1")] | ||
| [TestCase("1 .1")] | ||
| [TestCase("10_000")] | ||
| [TestCase("10 000")] | ||
| [TestCase("10.")] | ||
| [TestCase(".1")] | ||
| [TestCase("+.1")] | ||
| [TestCase("-.1")] | ||
| [TestCase("5*3")] | ||
| public void NumberValidator_ReturnsFalse_WithWrongFormat(string number) | ||
| => TestWithInvalidParameters(5, 4, false, number); | ||
|
|
||
| [TestCase("1,1")] | ||
| [TestCase("1.1")] | ||
| [TestCase("11")] | ||
| public void NumberValidator_CorrectWork_WithCorrectSeparatorFormat(string number) | ||
| => TestWithValidParameters(5, 4, false, number); | ||
|
|
||
| [TestCase("+11")] | ||
| [TestCase("+1111")] | ||
| [TestCase("+1.111")] | ||
| [TestCase("-1111")] | ||
| [TestCase("-1.111")] | ||
| [TestCase("-1.1")] | ||
| [TestCase("-11")] | ||
| [TestCase("+1.1")] | ||
| [TestCase("1.1")] | ||
| [TestCase("11")] | ||
| public void NumberValidator_CorrectWork_WithAndWithoutSign(string number) | ||
| => TestWithValidParameters(5, 3, false, number); | ||
|
|
||
| private static void TestWithValidParameters(int precision, int scale, bool onlyPositive, | ||
| string number) | ||
| { | ||
| var validator = new NumberValidator(precision, scale, onlyPositive); | ||
| validator.IsValidNumber(number).Should().BeTrue(); | ||
| } | ||
|
|
||
| private static void TestWithInvalidParameters(int precision, int scale, bool onlyPositive, | ||
| string number) | ||
| { | ||
| var validator = new NumberValidator(precision, scale, onlyPositive); | ||
| validator.IsValidNumber(number).Should().BeFalse(); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
почему такую категорию оставили?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не заметил, уберу