Рыбин Леонид #17

JavaScriptHaters · 2024-11-04T13:15:43Z

No description provided.

Testing/Basic/Homework/1. ObjectComparison/ObjectComparison.cs

-        ClassicAssert.AreEqual(expectedTsar.Parent.Height, actualTsar.Parent.Height);
-        ClassicAssert.AreEqual(expectedTsar.Parent.Parent, actualTsar.Parent.Parent);
+        actualTsar.Should().BeEquivalentTo(expectedTsar, options =>
+            options.Excluding(o => o.Name == "Id" && o.DeclaringType == typeof(Person)));


Testing/Basic/Homework/2. NumberValidator/NumberValidatorTests.cs

+        action.Should().Throw<ArgumentException>();
+    }
+
+    [TestCase(1, 0, true, TestName = "default")]


Testing/Basic/Homework/2. NumberValidator/NumberValidatorTests.cs

+    [TestCase(1, -2, true, TestName = "negative scale")]
+    [TestCase(1, 2, false, TestName = "scale > precision")]
+    [TestCase(1, 1, true, TestName = "scale == precision")]
+    public void NumberValidation_ConstructorHasArgumentException(int precision, int scale, bool onlyPositive)


Testing/Basic/Homework/2. NumberValidator/NumberValidatorTests.cs

-        Assert.Throws<ArgumentException>(() => new NumberValidator(-1, 2, false));
-        Assert.DoesNotThrow(() => new NumberValidator(1, 0, true));
+        var action = () => new NumberValidator(precision, scale, onlyPositive);
+        action.Should().Throw<ArgumentException>();


Testing/Basic/Homework/2. NumberValidator/NumberValidatorTests.cs

+    [TestCase(3, 2, true, "4321.5", ExpectedResult = false)]
+    [TestCase(3, 0, true, "+145", ExpectedResult = false)]
+    [TestCase(3, 0, false, "-124", ExpectedResult = false)]
+    public bool IsValidNumber(int precision, int scale, bool onlyPositive, string value)


Testing/Basic/Homework/2. NumberValidator/NumberValidatorTests.cs

+    [TestCase(10, 9, true, "127.0.0.1", ExpectedResult = false)]
+    [TestCase(3, 2, true, "4321.5", ExpectedResult = false)]
+    [TestCase(3, 0, true, "+145", ExpectedResult = false)]
+    [TestCase(3, 0, false, "-124", ExpectedResult = false)]


.gitignore

@@ -1,3 +1,5 @@
 bin/
 obj/
 .vs/
+Basic.sln
+Basic.sln.DotSettings.user


SlavikGh0st · 2024-11-05T09:21:46Z

Testing/Basic/Homework/2. NumberValidator/NumberValidatorTests.cs

-    [Test]
-    public void Test()
+    [TestCase(-1, 2, true, TestName = "negative precision")]
+    [TestCase(0, 2, false, TestName = "precision == 0")]


Всё конечно ситуативно, но лучше прям словами такие кейсы прописывать.

SlavikGh0st · 2024-11-05T19:38:29Z

Testing/Basic/Homework/2. NumberValidator/NumberValidatorTests.cs

+        return new NumberValidator(precision, scale, onlyPositive).IsValidNumber(value);
+    }
+
+    public class TrueCasesData


А зачем в статический класс заворачивать?
Можно же просто было public static IEnumerable MyTestCases а на тест навесить [TestCaseSource(nameof(MyTestCases)].

JavaScriptHaters · 2024-11-06T09:00:24Z

Testing/Basic/Basic.sln

+	GlobalSection(ExtensibilityGlobals) = postSolution
+		SolutionGuid = {54C706B2-21C4-4C49-A162-2E765ACC0F80}
+	EndGlobalSection
+EndGlobal


Да, вроде бы. Я помню он мне файл .sln предложил сохранить.

SlavikGh0st · 2024-11-05T19:44:43Z

Testing/Basic/Homework/2. NumberValidator/NumberValidatorTests.cs

-        Assert.DoesNotThrow(() => new NumberValidator(1, 0, true));
+        var action = () => new NumberValidator(precision, scale, onlyPositive);
+        action.Should()
+            .Throw<ArgumentException>();


Так вроде ж договорились в прошлый раз на блоки AAA разделить во всех тестах: т.е. нужно явно добавить переносы строк между блоками подготовки данных, непосредственного тестирующего действия и проверки актуального значения с ожидаемым.

Я так понял мне просто эти две строчки новой строкой отделить. Тестирующие действие это первая строка а проверка вторая, а между ними пустая строка.

Да, всё так

SlavikGh0st · 2024-11-05T19:45:31Z

Testing/Basic/Homework/2. NumberValidator/NumberValidatorTests.cs

+            .Throw<ArgumentException>();
+    }
+
+    [Test(Description = "default case")]


А что нам даёт такое описание теста?

JavaScriptHaters · 2024-11-06T09:02:43Z

Testing/Basic/Basic.sln.DotSettings.user

+      &lt;TestId&gt;NUnit3x::3C8EF388-9BF7-47CF-8B51-CA46F08B9253::net8.0::HomeExercise.Tasks.NumberValidator.NumberValidatorTests&lt;/TestId&gt;&#xD;
+    &lt;/TestAncestor&gt;&#xD;
+  &lt;/Or&gt;&#xD;
+&lt;/SessionState&gt;</s:String></wpf:ResourceDictionary>


Я погуглил и вроде как файл DotSettings.user добавляют в .gitignore. Это обязательно ?

В DotSettings.user обычно какие-то локальные настройки хранятся, скорее всего он создался из-за того что появился солюшен. В данном случае, да - его логично заигнорить.

В следующей домашке попробуй без солюшена редактировать проект, скорее всего такой проблемы просто не возникнет.

SlavikGh0st · 2024-11-05T20:51:28Z

Testing/Basic/Homework/2. NumberValidator/NumberValidatorTests.cs

+                yield return new TestCaseData(2, 0, true, "").SetName("empty string is incorrect").Returns(false);
+                yield return new TestCaseData(2, 1, true, ".0").SetName("before . always places at least one digit").Returns(false);
+                yield return new TestCaseData(2, 0, true, "0.").SetName("after . always places at least one digit").Returns(false);
+                yield return new TestCaseData(2, 0, true, "-1").SetName("- not accounted in precision").Returns(false);


В этом тесте не условие precision проверяется, а другое.

SlavikGh0st · 2024-11-05T20:56:06Z

Testing/Basic/Homework/2. NumberValidator/NumberValidatorTests.cs

+                yield return new TestCaseData(10, 9, true, "127.0.0.1").SetName("only real numbers format").Returns(false);
+                yield return new TestCaseData(3, 2, true, "4321.5").SetName("precision must be >= sum of all digits").Returns(false);
+                yield return new TestCaseData(3, 0, true, "+145").SetName("+ not accounted in precision").Returns(false);
+                yield return new TestCaseData(3, 0, false, "-124").SetName("- not accounted in precision for number > 9").Returns(false);


Почему именно для цифр больше 9?

SlavikGh0st · 2024-11-05T20:58:13Z

Testing/Basic/Homework/2. NumberValidator/NumberValidatorTests.cs

+                yield return new TestCaseData(10, 2, true, "abcde.f").SetName("use only digits").Returns(false);
+                yield return new TestCaseData(10, 9, true, "127.0.0.1").SetName("only real numbers format").Returns(false);
+                yield return new TestCaseData(3, 2, true, "4321.5").SetName("precision must be >= sum of all digits").Returns(false);
+                yield return new TestCaseData(3, 0, true, "+145").SetName("+ not accounted in precision").Returns(false);


По текущему наименованию теста я не могу понять - плюс должен учитываться или нет в точности.

JavaScriptHaters · 2024-11-06T09:04:27Z

Testing/Basic/Homework/2. NumberValidator/NumberValidatorTests.cs

+                yield return new TestCaseData(3, 0, true, "+145").SetName("+ not accounted in precision").Returns(false);
+                yield return new TestCaseData(3, 0, false, "-124").SetName("- not accounted in precision for number > 9").Returns(false);
+            }
+        }


Я переписал название тестов в развернутом виде, но PascalCase требует каждое слово с большой буквы писать, но в названии теста там предложение обычное, и как-то не очень в предложении каждое слово с большой буквы писать.

SlavikGh0st · 2024-11-05T21:14:21Z

Testing/Basic/Homework/2. NumberValidator/NumberValidatorTests.cs

-        ClassicAssert.IsFalse(new NumberValidator(3, 2, true).IsValidNumber("-1.23"));
-        ClassicAssert.IsFalse(new NumberValidator(3, 2, true).IsValidNumber("a.sd"));
+    [TestCaseSource(typeof(FalseCasesData), nameof(FalseCasesData.TestCases))]
+    public bool IsValidNumber_ReturnFalse(int precision, int scale, bool onlyPositive, string value)


Давай так:

[TestCaseSource(nameof(TestCases1))] [TestCaseSource(nameof(TestCases2))] public bool IsValidNumber_WorksCorrectly()

А ожидания уже в наименовании тест-кейсов

SlavikGh0st · 2024-11-06T09:49:13Z

Testing/Basic/Homework/2. NumberValidator/NumberValidatorTests.cs

+        return new NumberValidator(precision, scale, onlyPositive).IsValidNumber(value);
+    }
+
+    public static IEnumerable TestCase1


Ну давай уж осмысленно назовем: ValidNumbersCases и InvalidNumbersCases
Можно даже ещё отдельно выделить InvalidInputCases и туда разместить тесты с некорректным инпутом: пустая строка, нет целой части у Decimal и прочие

SlavikGh0st · 2024-11-06T14:09:07Z

Testing/Basic/Homework/2. NumberValidator/NumberValidatorTests.cs

+        get
+        {
+            yield return new TestCaseData(10, 9, true, "127.0.0.1").SetName("ForNumber_WithNonRealNumberFormat_ReturnFalse").Returns(false);
+            yield return new TestCaseData(2, 0, true, "").SetName("ForNumber_IsEmptyString_ReturnFalse").Returns(false);


Вот тут хорошо просто зайдет "ForEmptyString_ReturnFalse" - префикс "ForNumber" только путает

SlavikGh0st · 2024-11-06T14:11:15Z

Testing/Basic/Homework/2. NumberValidator/NumberValidatorTests.cs

+            yield return new TestCaseData(2, 0, true, "").SetName("ForNumber_IsEmptyString_ReturnFalse").Returns(false);
+            yield return new TestCaseData(2, 1, true, ".0").SetName("ForNumber_WithMissingIntegerPart_ReturnFalse").Returns(false);
+            yield return new TestCaseData(2, 0, true, "0.").SetName("ForNumber_WithMissingFractionalPart_ReturnFalse").Returns(false);
+            yield return new TestCaseData(10, 2, true, "abcde.f").SetName("ForNumber_WithNonDigitCharacters_ReturnFalse").Returns(false);


Как будто бы этот тест дублирует первый - тут нужен один с понятным названием "ForNotNumberInput_ReturnFalse"

SlavikGh0st · 2024-11-06T14:14:56Z

Testing/Basic/Homework/2. NumberValidator/NumberValidatorTests.cs

+    {
+        get
+        {
+            yield return new TestCaseData(2, 0, true, "-1").SetName("ForNegativeInteger_WithMinusSign_OnlyPositiveIsFalse").Returns(false);


Вот тут что-то не то "ForNegativeInteger_WithMinusSign" - у нас уже есть информация, что на входе отрицательное целое, зачем уточнять, что оно со знаком?) Может отрицательное без знака как-то быть?)
Я бы тут просто написал "ForNegativeNumber_WhenOnlyPositiveConfigured_ReturnFalse"

Можно 2 теста добавить: один ForDecimal, другой ForInteger

SlavikGh0st · 2024-11-06T14:18:06Z

Testing/Basic/Homework/2. NumberValidator/NumberValidatorTests.cs

+        get
+        {
+            yield return new TestCaseData(2, 0, true, "-1").SetName("ForNegativeInteger_WithMinusSign_OnlyPositiveIsFalse").Returns(false);
+            yield return new TestCaseData(3, 2, true, "4321.5").SetName("ForDecimal_WithIncorrectPrecision_ReturnFalse").Returns(false);


Вот я вижу тест для decimal, логичный вопрос возникает: а где для integer такой же?

SlavikGh0st · 2024-11-06T14:18:47Z

Testing/Basic/Homework/2. NumberValidator/NumberValidatorTests.cs

+        {
+            yield return new TestCaseData(2, 0, true, "-1").SetName("ForNegativeInteger_WithMinusSign_OnlyPositiveIsFalse").Returns(false);
+            yield return new TestCaseData(3, 2, true, "4321.5").SetName("ForDecimal_WithIncorrectPrecision_ReturnFalse").Returns(false);
+            yield return new TestCaseData(3, 0, true, "+145").SetName("ForPositiveDecimal_WithPlusSign_PrecisionCalculationAccountPlusSign").Returns(false);


"ForPositiveDecimal" - а в тесте integer

SlavikGh0st · 2024-11-06T14:20:04Z

Testing/Basic/Homework/2. NumberValidator/NumberValidatorTests.cs

+            yield return new TestCaseData(2, 0, true, "-1").SetName("ForNegativeInteger_WithMinusSign_OnlyPositiveIsFalse").Returns(false);
+            yield return new TestCaseData(3, 2, true, "4321.5").SetName("ForDecimal_WithIncorrectPrecision_ReturnFalse").Returns(false);
+            yield return new TestCaseData(3, 0, true, "+145").SetName("ForPositiveDecimal_WithPlusSign_PrecisionCalculationAccountPlusSign").Returns(false);
+            yield return new TestCaseData(3, 0, false, "-124").SetName("ForNegativeDecimal_WithMinusSign_PrecisionCalculationAccountMinusSign").Returns(false);


Также - в тесте передаем integer, а в наименовании decimal

SlavikGh0st · 2024-11-06T14:24:33Z

Testing/Basic/Homework/2. NumberValidator/NumberValidatorTests.cs

+            yield return new TestCaseData(3, 2, true, "4321.5").SetName("ForDecimal_WithIncorrectPrecision_ReturnFalse").Returns(false);
+            yield return new TestCaseData(3, 0, true, "+145").SetName("ForPositiveDecimal_WithPlusSign_PrecisionCalculationAccountPlusSign").Returns(false);
+            yield return new TestCaseData(3, 0, false, "-124").SetName("ForNegativeDecimal_WithMinusSign_PrecisionCalculationAccountMinusSign").Returns(false);
+        }


Вот сейчас, когда все тесты однообразно проименованы, я невооруженным глазом вижу:

Что работа Precision проверяется только для integer, хотя по хорошему бы и для decimal аналогичных кейсов накидать.

Нету тестов проверки на scale: когда у нас число не будет соответствовать этому параметру, должно вернуться false.

SlavikGh0st · 2024-11-06T14:35:01Z

Testing/Basic/Homework/2. NumberValidator/NumberValidatorTests.cs

+        action.Should().Throw<ArgumentException>();
+    }
+
+    [Test(Description = "NumberValidator_WithCorrectParameters_NotThrowException")]


До сих пор не понимаю, для чего тут описание теста)

Description - это не TestName, для атрибута [Test] за имя теста будет браться наименование метода.
Description обычно используется как коммент к тесту, и в нём в свободной форме можно написать, что делает тест, если он неочевиден.

В данном случае я по имени метода всё прекрасно понимаю, что делается и что ожидается.
Но есть замечания:

Не указаны условия, когда конструктор не должен вбросить ошибку (хотя бы просто WhenCorrectParameters).

В наименовании теста ты пишешь, что "конструктор не должен вбросить ArgumentException", а по факту у тебя проверка внутри на любую ошибку.

SlavikGh0st · 2024-11-06T14:38:58Z

Testing/Basic/Homework/2. NumberValidator/NumberValidatorTests.cs

+    [TestCase(1, -2, true, TestName = "NumberValidator_WithNegativeScale_ThrowsException")]
+    [TestCase(1, 2, false, TestName = "NumberValidator_WithScaleGreaterThanPrecision_ThrowsException")]
+    [TestCase(1, 1, true, TestName = "NumberValidator_WithScaleEqualsPrecision_ThrowsException")]
+    public void NumberValidation_ConstructorThrowsArgumentException(int precision, int scale, bool onlyPositive)


Тут всё хорошо, править не будем, но на будущее:
Здесь наименование метода уже говорит нам о действии и об ожиданиях, поэтому в тест-кейсах необязательно прописывать "NumberValidator_" и "_ThrowsException".

Сейчас теряются из вида условия, когда должна быть вкинута ошибка:

И вот как можно сделать:
У меня информация о том, что проверятся и что ожидается есть - она взята из наименования тест-метода
Все тест-кейсы объединились в группу этого тест-метода, и я точно и чётко вижу все условия, когда конструктор должен упасть с ошибкой

JavaScriptHaters added 2 commits November 4, 2024 15:31

Fix ObjectComparison

d00183c

Fix NumberValidatorTests

475de0f

SlavikGh0st approved these changes Nov 5, 2024

View reviewed changes

Refactor ObjectComparison and NumberValidatorTests

6ff61fe

JavaScriptHaters requested a review from SlavikGh0st November 5, 2024 13:23

SlavikGh0st approved these changes Nov 5, 2024

View reviewed changes

Rename tests and small fix NumberValidatorTests

26c0d7d

JavaScriptHaters requested a review from SlavikGh0st November 6, 2024 09:09

SlavikGh0st reviewed Nov 6, 2024

View reviewed changes

Rename Tests in NumberValidatorTests v2

afcbbf5

JavaScriptHaters requested a review from SlavikGh0st November 6, 2024 10:48

SlavikGh0st reviewed Nov 6, 2024

View reviewed changes

Fix Tests in NumberValidatorTests v3

b385885

JavaScriptHaters requested a review from SlavikGh0st November 6, 2024 15:28

Рыбин Леонид #17

Are you sure you want to change the base?

Рыбин Леонид #17

Conversation

JavaScriptHaters commented Nov 4, 2024

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

This comment was marked as off-topic.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SlavikGh0st Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SlavikGh0st Nov 6, 2024 •

edited

Loading