Козлов Данил by SenyaPevko · Pull Request #228 · kontur-courses/testing

SenyaPevko · 2023-11-26T12:18:32Z

pakapik

В целом всё круто)

pakapik · 2023-11-28T11:21:36Z

cs/HomeExercises/NumberValidatorTests.cs

+                    .SetName("IsValidNumber_ReturnsFalse_WhenSymbolWithNumberLengthGreaterThanPrecision");
+                yield return new TestCaseData(3, 2, true, "00.00").Returns(false)
+                    .SetName("IsValidNumber_ReturnsFalse_WhenIntPartWithFracPartGreaterThanPrecision");
+                yield return new TestCaseData(17, 2, true, "0").Returns(true)


Было бы неплохо разделить пустой строкой кейсы с false и true

pakapik · 2023-11-28T11:28:36Z

cs/HomeExercises/NumberValidatorTests.cs

+                yield return new TestCaseData(4, 2, false, "-1.23").Returns(true)
+                    .SetName("IsValidNumber_ReturnsTrue_WhenNegativeSymbolWithNumberLengthNotGreaterThanPrecision");
+            }
+        }


Было бы неплохо разделить пустой строкой источники данных

pakapik · 2023-11-28T11:33:22Z

cs/HomeExercises/NumberValidatorTests.cs

-				throw new ArgumentException("precision must be a non-negative number less or equal than precision");
-			numberRegex = new Regex(@"^([+-]?)(\d+)([.,](\d+))?$", RegexOptions.IgnoreCase);
-		}
+        [Test, TestCaseSource(nameof(IsValidNumberPositivityTests)), 


Атрибут Test необязателен, если имеются TestCaseSource. Лучше несколько TestCaseSource указывать раздельно, не через запятую - оно так смотрится банально симпатичнее.

pakapik · 2023-11-28T11:33:53Z

cs/HomeExercises/NumberValidatorTests.cs

+            TestCaseSource(nameof(IsValidNumberSymbolsTests))]
+        public bool IsValidNumber_Returns(int precision, int scale, bool onlyPositive, string number)
+        {
+            return new NumberValidator(precision, scale, onlyPositive).IsValidNumber(number);


Можно заменить на expression body

pakapik · 2023-11-28T11:39:21Z

cs/HomeExercises/NumberValidatorTests.cs

+        [Test, TestCaseSource(nameof(ConstructorArgumentExceptions))]
+        public void Constructor_ThrowsArgumentException(int precision, int scale, bool onlyPositive)
+        {
+            Invoking(() => new NumberValidator(precision, scale, onlyPositive)).Should().Throw<ArgumentException>();


Применение using static только ради вызова Invoking необоснованно.

pakapik · 2023-11-28T11:40:27Z

cs/HomeExercises/NumberValidatorTests.cs

+    [TestFixture]
+    public class NumberValidatorTests
+    {
+        public static IEnumerable IsValidNumberPrecisionTests


Почему public?

pakapik · 2023-11-28T11:42:59Z

cs/HomeExercises/NumberValidatorTests.cs

+        {
+            get
+            {
+                yield return new TestCaseData(3, 2, true, "+0.00").Returns(false)


Зачем здесь yield return? Можно обойтись без него, выйдет лаконичнее.

pakapik · 2023-11-28T11:48:22Z

cs/HomeExercises/NumberValidatorTests.cs

+            get
+            {
+                yield return new TestCaseData(3, 2, true, "+0.00").Returns(false)
+                    .SetName("IsValidNumber_ReturnsFalse_WhenSymbolWithNumberLengthGreaterThanPrecision");


Нет нужды и в названии теста, и в названии сценарии теста дублировать информацию.
Тест у тебя называется IsValidNumber_Returns. В дереве тестов у тебя в итоге будет что-то навроде

IsValidNumber_Returns IsValidNumber_ReturnsFalse_WhenSymbolWithNumberLengthGreaterThanPrecision IsValidNumber_ReturnsFalse_WhenIntPartWithFracPartGreaterThanPrecision

Можно и нужно упростить, оставив только нужную для понимания теста информацию.

pakapik · 2023-11-28T12:17:35Z

cs/HomeExercises/ObjectComparison.cs

-			Assert.AreEqual(actualTsar.Age, expectedTsar.Age);
-			Assert.AreEqual(actualTsar.Height, expectedTsar.Height);
-			Assert.AreEqual(actualTsar.Weight, expectedTsar.Weight);
+            // сравнение родителей такое же как и в коде, который нужно было отрефакторить - сравнивает по ссылке,


Весьма любопытно🧐

pakapik · 2023-11-28T13:55:14Z

cs/HomeExercises/NumberValidatorTests.cs

+    {
+        private static IEnumerable<TestCaseData> IsValidNumberPrecisionTests
+        {
+            get


Не совсем то, что я имел в виду.

Про разделение false / true - кейсы с false друг за дружкой без разрывов, {пустая строка}, затем кейсы с true так же без разрывов.

new TestCaseData(1).Returns(false).SetName("a"), new TestCaseData(2).Returns(false).SetName("b"), // пустая строка new TestCaseData(3).Returns(true).SetName("c"), new TestCaseData(4).Returns(true).SetName("d"),

Каждый новый кейс начинать отделять пустой строкой имеет смысл только, если кейсы сами по себе большие, но это не тот случай.

Про разделение источников данных пустой строкой - это что после объявления IsValidNumberPrecisionTests нужно сделать пустую строку. Затем объявлять следующий источник данных.

Так что придется ещё немного поправить)

pakapik · 2023-11-28T13:59:44Z

cs/HomeExercises/NumberValidatorTests.cs

+        {
+            get
+            {
+                return new List<TestCaseData>()


В List здесь нет смысла, т.к. ты не работаешь с этой структурой. Можно было бы даже не типизировать IEnumerable. get здесь тоже сам по себе не нужен.

Избавься от геттера, заюзай массив и не забудь для него использовать инициализатор, тогда будет по красоте. Типизацию IEnumerable оставь на свой вкус, я бы типизировал, но и без этого всё ок будет.

pakapik · 2023-11-28T14:01:16Z

cs/HomeExercises/NumberValidatorTests.cs

+
+        [TestCaseSource(nameof(ConstructorArgumentExceptions))]
+        public void Constructor_ThrowsArgumentException(int precision, int scale, bool onlyPositive) =>
+            FluentActions.Invoking(() => new NumberValidator(precision, scale, onlyPositive))


Почти. Предлагаю вообще избавиться от FluentActions. С Assert.Throws<> выйдет лаконичнее.

pakapik · 2023-11-28T14:29:07Z

cs/HomeExercises/NumberValidatorTests.cs

+            .SetName("True_WhenNegativeSymbolWithNumberLengthNotGreaterThanPrecision")
+        };
+
+        private static IEnumerable<TestCaseData> IsValidNumberScaleTests = new TestCaseData[]


Раз IEnumerable<> типизирован, то после new нет нужды ещё раз писать тип - можно его убрать. Компилятор сам разберется.

pakapik · 2023-11-28T14:29:11Z

cs/HomeExercises/NumberValidatorTests.cs

+            new NumberValidator(precision, scale, onlyPositive).IsValidNumber(number);
+
+
+        [TestCaseSource(nameof(ConstructorArgumentExceptions))]


Найс. После рефакторинга видно, что источник данных для теста с конструктором слишком далеко от самого теста.

Давай протащим ConstructorArgumentExceptions ближе к тесту, который использует этот источник данных.

Да и в целом история хорошая, когда данные лежат как можно ближе к месту использования.

pakapik · 2023-11-29T06:49:31Z

cs/HomeExercises/NumberValidatorTests.cs

+        private static IEnumerable<TestCaseData> IsValidNumberSymbolsTests = new []
+        {
+            new TestCaseData(3, 2, true, "a.sd").Returns(false).SetName("False_WhenGivenNotDigits"),
+            new TestCaseData(17, 2, true, "").Returns(false).SetName("False_WhenEmptyStringGiven")


Давай добавим чуть больше данных для проверок - null, спец.символы - /r , /n, просто символы %$# и т.д. Число с каким-то символом? Символ с каким-то числом? Строка из пробелов?

Что будет, если разделитель не точка, а запятая? А если разделителей несколько? А если в строке только разделитель?

Как валидатор ведет себя с очень большими числами? С очень большой точностью? А все вместе? Умеет ли он понимать числа в экспоненциальном формате?

А может, валидатор считает верным число в другой системе счисления? Иррациональные числа?

Возвращает ли валидатор на один и тот же входной параметр один и тот же результат? Для этого можно использовать атрибут [Repeat()].

А может, валидатор как-то меняет свой стейт? Что будет если подать сначала одно число, затем второе, а затем снова первое? Кстати, для того, чтоб обозначить, что метод ничего не изменяет (что-то внутри себя / входные параметры), то его можно пометить атрибутом [Pure] из неймспейса System.Diagnostics.Contracts или JetBrains.Annotations.

Тесты как документация кода должны отвечать на все эти вопросы и даже чуть больше)

pakapik · 2023-11-29T09:00:36Z

cs/HomeExercises/NumberValidatorTests.cs

+            .SetName("True_WhenGivenEndlessRationalNumber"),
+            new TestCaseData(1001, 1000, true, $"{Math.PI}").Returns(true)
+            .SetName("True_WhenGivenEndlessIrrationalNumber"),
+            new TestCaseData(2, 1, true, "01").Returns(true)


Этот же тест в IsValidNumberDiffrentFormsOfNumbersTests

pakapik · 2023-11-29T09:02:48Z

cs/HomeExercises/NumberValidatorTests.cs

+            new TestCaseData(17, 2, true, "   ").Returns(false).SetName("False_WhenStringOfSpacesGiven")
+        };
+
+        [Pure]


[Pure] стоило прицепить на метод IsValidNumber, с тестами их юзать не стоит.

Атрибуты обычно стараются выравнивать по длине

Чтоб Было вот так

В случае, когда были только источники данных, я пропускал этот момент, но [Repeat(5)] прям глаз режет)

pakapik · 2023-11-29T09:03:28Z

cs/HomeExercises/NumberValidatorTests.cs

+
+        [Pure]
+        [TestCaseSource(nameof(ConstructorArgumentExceptions))]
+        public void Constructor_ThrowsArgumentException(int precision, int scale, bool onlyPositive) =>


Возможно, стоит добавить тест, когда конструктор не кидает исключений.

pakapik · 2023-11-29T09:05:23Z

cs/HomeExercises/NumberValidatorTests.cs

+        [Test]
+        public void ValidatorState_ShouldStayUnchanged()
+        {
+            var validator = new NumberValidator(10, 9, true);


Нет структуры ААА - Arrange, Act, Assert. Если тест небольшой, то обычно просто пустыми строками разделяют. Если большой (либо есть желание), то дополнительно прям пишут, где какая секция, используя комментарии.

Тут писать комменты смысла нет, но логически всё равно стоит разделить.

pakapik · 2023-11-29T09:07:28Z

cs/HomeExercises/NumberValidatorTests.cs

+            .SetName("True_WhenFracPartNotGreaterThanScale")
+        };
+
+        private static IEnumerable<TestCaseData> IsValidNumberBigNumbersTests = new[]


Было бы правильнее назвать не BigNumbers, а LongNumbers, т.к. 1/3 в виде десятичной дроби - число длинное, но не большое). В то время, как Int64.MaxValue и большое, и длинное.

pakapik · 2023-11-29T09:10:00Z

cs/HomeExercises/NumberValidatorTests.cs

+
+        private static IEnumerable<TestCaseData> IsValidNumberDiffrentFormsOfNumbersTests = new[]
+        {
+            new TestCaseData(2, 1, true, "01").Returns(true)


В программировании относительно часто используется так же и шестнадцатеричная система, её тоже стоит добавить).

Двоичная как-то не очень выглядит. Глядя на тест я вижу, что валидатор считает число 01 верным. А если увеличить разрядность и написать 0001? Должно быть это зависит от передаваемых параметров?

refactored tests

da2fc5c

pakapik reviewed Nov 28, 2023

View reviewed changes

SenyaPevko force-pushed the master branch from 3e42e4c to da2fc5c Compare November 28, 2023 13:40

refactored after reseting branch

a703de1

pakapik reviewed Nov 28, 2023

View reviewed changes

refactored after feedback 2

dc64fc8

pakapik reviewed Nov 28, 2023

View reviewed changes

refactored after feedback 3

55a9716

pakapik reviewed Nov 29, 2023

View reviewed changes

added tests

b50bce9

pakapik reviewed Nov 29, 2023

View reviewed changes

added tests and refactored

02eaf46

		new NumberValidator(precision, scale, onlyPositive).IsValidNumber(number);


		[TestCaseSource(nameof(ConstructorArgumentExceptions))]

Conversation

SenyaPevko commented Nov 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pakapik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pakapik Nov 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pakapik Nov 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pakapik Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pakapik Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pakapik Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pakapik Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SenyaPevko commented Nov 26, 2023 •

edited

Loading

pakapik Nov 28, 2023 •

edited

Loading

pakapik Nov 28, 2023 •

edited

Loading

pakapik Nov 29, 2023 •

edited

Loading

pakapik Nov 29, 2023 •

edited

Loading

pakapik Nov 29, 2023 •

edited

Loading

pakapik Nov 29, 2023 •

edited

Loading