Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Бабин Георгий #240

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Бабин Георгий #240

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e5c6bb5
Rewrite CheckCurrentTsar on using FluentAssertions
tripples25 Nov 27, 2023
3deaa3c
Refactored NumberValidatorTests and added missing tests
tripples25 Nov 28, 2023
d100a68
Added an answer to the question
tripples25 Nov 28, 2023
5ec33cb
Merge number-validator-tests-refactor branch
tripples25 Nov 28, 2023
b0124c5
Split files in HomeExercises in separate classes
tripples25 Nov 29, 2023
f0d9880
Added additional tests in NumberValidatorTests
tripples25 Nov 29, 2023
8478ac8
Removed unnecessary constructor tests and did refactor in NumberValid…
tripples25 Nov 29, 2023
6cb0cc9
Changed onlyPositive argument in some TestCases in NumberValidatorTests
tripples25 Nov 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 45 additions & 19 deletions cs/HomeExercises/NumberValidatorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,28 +5,54 @@

namespace HomeExercises
Copy link

@desolver desolver Nov 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Понимаю, что этот файл создавал не ты, но быть может задумка автора как раз и была в том, чтобы с какими-то конвентами знакомить

В большом продакшене придется работать с тысячами файлов и классов, и всегда удобно, когда каждый класс, интерфейс, enum и тд лежит в своем собственном файле. Давай тут сделаем так же

Аналогично с соседним файлом

{
[TestFixture]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[TestFixture] можно не писать, если внутри класса есть [Test] или [TestCase], .net всё поймет)

public class NumberValidatorTests
{
[Test]
public void Test()
[TestCase(0, 2, true, TestName = "precision is zero")]
[TestCase(-1, 2, true, TestName = "precision is negative")]
[TestCase(1, -1, false, TestName = "scale is negative")]
[TestCase(1, 2, false, TestName = "scale is greater than precision")]
[TestCase(1, 1, false, TestName = "scale is equals precision")]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Почему в последних трех тестах поменялся флаг onlyPositive? Разве мы можем гарантировать, что не он стал виновником падения при создании объекта?

Обычно в тесткейсе меняется какой-то один проверяемый или пара связанных параметров, тогда явно видна зависимость параметров от результатов работы кода. Сейчас же эта связь размазалась, т.к. в названии тесткейса фигурируют только scale и precision, а меняется ещё и onlyPositive

public void Constructor_Fails_OnIncorrectArguments(int precision, int scale, bool onlyPositive)
{
Assert.Throws<ArgumentException>(() => new NumberValidator(-1, 2, true));
Assert.DoesNotThrow(() => new NumberValidator(1, 0, true));
Assert.Throws<ArgumentException>(() => new NumberValidator(-1, 2, false));
Assert.DoesNotThrow(() => new NumberValidator(1, 0, true));

Assert.IsTrue(new NumberValidator(17, 2, true).IsValidNumber("0.0"));
Assert.IsTrue(new NumberValidator(17, 2, true).IsValidNumber("0"));
Assert.IsTrue(new NumberValidator(17, 2, true).IsValidNumber("0.0"));
Assert.IsFalse(new NumberValidator(3, 2, true).IsValidNumber("00.00"));
Assert.IsFalse(new NumberValidator(3, 2, true).IsValidNumber("-0.00"));
Assert.IsTrue(new NumberValidator(17, 2, true).IsValidNumber("0.0"));
Assert.IsFalse(new NumberValidator(3, 2, true).IsValidNumber("+0.00"));
Assert.IsTrue(new NumberValidator(4, 2, true).IsValidNumber("+1.23"));
Assert.IsFalse(new NumberValidator(3, 2, true).IsValidNumber("+1.23"));
Assert.IsFalse(new NumberValidator(17, 2, true).IsValidNumber("0.000"));
Assert.IsFalse(new NumberValidator(3, 2, true).IsValidNumber("-1.23"));
Assert.IsFalse(new NumberValidator(3, 2, true).IsValidNumber("a.sd"));
Action a = () => { new NumberValidator(precision, scale, onlyPositive); };
a.Should().Throw<ArgumentException>();
}

[TestCase(1, 0, true, TestName = "with all arguments")]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Нужен ли [TestCase], когда тесткейс всего один?

public void Constructor_Success_OnCorrectArguments(int precision, int scale, bool onlyPositive)
{
Action a = () => { new NumberValidator(precision, scale, onlyPositive); };
a.Should().NotThrow();
}

[TestCase(3, 2, true, null, TestName = "value is null")]
[TestCase(3, 2, true, "", TestName = "value is empty")]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Зачастую null, пустая строка и ещё пробел идут паровозиком

[TestCase(3, 2, true, "+1..23", TestName = "value has incorrect format")]
[TestCase(17, 2, true, "0.000", TestName = "value's fraction part length is greater than scale")]
[TestCase(5, 2, true, "-0.00", TestName = "negative sign when onlyPositive is true")]
[TestCase(3, 2, true, "+0.00", TestName = "intPart and fractPart together is greater than precision")]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Давай подумаем, точно ли все тесткейсы здесь обработаны. Безусловно, ты уже обработал основные, но есть так же и некоторые граничные и совсем неадекватные вещи)) Никогда не знаешь, чего можно ожидать от пользователя)

public void IsValidNumber_ReturnsFalse_OnIncorrectArguments(int precision, int scale, bool onlyPositive, string value)
{
new NumberValidator(precision, scale, onlyPositive)
.IsValidNumber(value)
.Should()
.BeFalse();
}

[TestCase(17, 2, true, "0", TestName = "value without sign")]
[TestCase(17, 2, true, "+0", TestName = "value with positive sign")]
[TestCase(17, 2, false, "-0", TestName = "value with negative sign")]
[TestCase(17, 2, true, "0.0", TestName = "value with period as delimiter")]
[TestCase(17, 2, true, "0,0", TestName = "value with comma as delimiter")]
[TestCase(17, 2, true, "+0,0", TestName = "value with sign and delimiter")]
[TestCase(40, 20, true, "1234567890", TestName = "value with different numbers")]
public void IsValidNumber_ReturnsTrue_OnCorrectArguments(int precision, int scale, bool onlyPositive, string value)
{
new NumberValidator(precision, scale, onlyPositive)
.IsValidNumber(value)
.Should()
.BeTrue();
}
}

Expand Down
32 changes: 18 additions & 14 deletions cs/HomeExercises/ObjectComparison.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,28 +7,33 @@ public class ObjectComparison
{
[Test]
[Description("Проверка текущего царя")]
[Category("ToRefactor")]
public void CheckCurrentTsar()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ты написал, что нейминг соседнего теста не блещет читаемостью и понятностью, а что скажешь насчет этого?)

{
var actualTsar = TsarRegistry.GetCurrentTsar();

var expectedTsar = new Person("Ivan IV The Terrible", 54, 170, 70,
new Person("Vasili III of Russia", 28, 170, 60, null));

// Перепишите код на использование Fluent Assertions.
Assert.AreEqual(actualTsar.Name, expectedTsar.Name);
Assert.AreEqual(actualTsar.Age, expectedTsar.Age);
Assert.AreEqual(actualTsar.Height, expectedTsar.Height);
Assert.AreEqual(actualTsar.Weight, expectedTsar.Weight);

Assert.AreEqual(expectedTsar.Parent!.Name, actualTsar.Parent!.Name);
Assert.AreEqual(expectedTsar.Parent.Age, actualTsar.Parent.Age);
Assert.AreEqual(expectedTsar.Parent.Height, actualTsar.Parent.Height);
Assert.AreEqual(expectedTsar.Parent.Parent, actualTsar.Parent.Parent);

actualTsar
.Should()
.BeEquivalentTo(expectedTsar, options => options
.Excluding(memberInfo => memberInfo.SelectedMemberInfo.DeclaringType == typeof(Person)
&& memberInfo.SelectedMemberInfo.Name == nameof(Person.Id)));
}

[Test]
[Description("Альтернативное решение. Какие у него недостатки?")]
/* 1. Непонятно из названия, что тест проверяет и какого результата ожидает.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Всё верно, хорошо расписал

* 2. При использовании такого Assert, при падении теста мы узнаем лишь, что мы получили False,
* без дополнительной информации об отличающихся полях и т.д.
* 3. При изменении класса Person нам всегда придётся соответственно менять и локальный метод
* AreEqual для класса Person.
* 4. Если каким-то образом произойдёт бесконечная рекурсия внутри класса Person, то данный способ привдёт к
* бесконечному проходу по нему. Проверка через FluentAssertions ограничена глубиной
* в 10 вхождений и это значение кастомизируемо.
* 5. При добавлении новых полей в класс Person, которые мы хотим включать в этот Equals - метод может
* сильно разрастись и его читаемость сильно снизится.
*/
public void CheckCurrentTsar_WithCustomEquality()
{
var actualTsar = TsarRegistry.GetCurrentTsar();
Expand Down Expand Up @@ -56,8 +61,7 @@ public class TsarRegistry
{
public static Person GetCurrentTsar()
{
return new Person(
"Ivan IV The Terrible", 54, 170, 70,
return new Person("Ivan IV The Terrible", 54, 170, 70,
new Person("Vasili III of Russia", 28, 170, 60, null));
}
}
Expand Down