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.

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

Яценко Ирина #235

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
138 changes: 119 additions & 19 deletions cs/HomeExercises/NumberValidatorTests.cs
Copy link

Choose a reason for hiding this comment

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

Круто, что коммиты логически разбиты

Original file line number Diff line number Diff line change
Expand Up @@ -8,27 +8,127 @@ namespace HomeExercises
public class NumberValidatorTests
Copy link

Choose a reason for hiding this comment

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

using FluentAssertions; не используется. Обычно ide сама убирает неиспользуемые неймспейсы, но не всегда.

Copy link

Choose a reason for hiding this comment

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

Ранее об этом не говорил, но класс тоже можно пометить атрибутом TestFixture, причем туда же можно по желанию так же впихнуть TestOf: [TestFixture(TestOf = typeof(NumberValidator))]

{
[Test]
public void Test()
public void NumberValidator_WhenPassNegativePrecision_ShouldThrowsArgumentException()
{
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"));
TestDelegate testDelegate = () => new NumberValidator(-1, 2, true);

Assert.Throws<ArgumentException>(testDelegate);
}
}

[Test]
public void NumberValidator_WhenPassNegativeScale_ShouldThrowsArgumentException()
{
TestDelegate testDelegate = () => new NumberValidator(1, -2);

Assert.Throws<ArgumentException>(testDelegate);
}

[Test]
public void NumberValidator_WhenPassValidArguments_ShouldDoesNotThrows()
Copy link

Choose a reason for hiding this comment

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

Хорошо. Ещё, как вариант в борьбе с дублированием кода, 2 предыдущих теста можно было бы объединить в один с именем NumberValidator_WhenPassInvalidArguments_ShouldThrowsArgumentException, подтягивая данные из TestCaseSource . Подобный подход хорош тем, что на один тест можно клепать много входных данных, проверяя n сценариев сразу. При этом можно манипулировать параметрами входных данных всякими SetXXX и Object Mother / Test Data Buider. Из минусов можно назвать, что тесту приходится давать чуть более общее имя (которое, впрочем, можно уточнить через SetName) и придумывать какое-то название для TestCaseSource, но это мелочи по сравнению с тем, какого объёма кода иногда можно избежать. Это просто к сведению, переделывать не нужно.

Буду оставлять ещё nit: замечания, которые цепляют глаз только у меня. Ты можешь принимать их к сведению, но не обязана что-то с этим делать - исправлять или даже отвечать.

{
TestDelegate testDelegate = () => new NumberValidator(1, 0, true);

Assert.DoesNotThrow(testDelegate);
Copy link

Choose a reason for hiding this comment

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

Тут тоже можно до одной строки упростить.

}

[Test]
public void NumberValidator_WhenPrecisionIsEqualToTheScale_ShouldReturnFalse()
Copy link

Choose a reason for hiding this comment

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

Ожидаемый результат ShouldReturnFalse не соответствует секции Assert.

nit: к NumberValidator в названии теста я бы добавил ctor: NumberValidatorCtor, чтоб подчеркнуть, что тестируется именно конструктор, а не класс.

{
TestDelegate testDelegate = () => new NumberValidator(2, 2, true);

Assert.Throws<ArgumentException>(testDelegate);
}

[Test]
public void IsValidNumber_WhenPassOnlyPositiveIsFalseButNumbersDoesNotHaveNegativeSign_ShouldReturnTrue()
Copy link

Choose a reason for hiding this comment

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

Откровенно говоря, я не понял условия 🙃
Почему PassOnlyPositive - IsFalse ?

Copy link

Choose a reason for hiding this comment

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

nit: numbers множественное число, does следовало бы заменить на do - does используется только для he/she/it. Обращаю внимание только по той причине, что можно было сократить название длинного метода на пару буковок🙃

{
var validator = new NumberValidator(17, 2);
Copy link

Choose a reason for hiding this comment

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

nit: в лекции видел, что у вас есть упоминание System Under Test. По некоторым гайдам/книгам/статьям, именно так и стоит называть тестируемый объект - sut. Если бы секция Arrange была больше, то из nit это превратилось в момент, который стоит поправить.


var numberIsValid = validator.IsValidNumber("1.0");

Assert.True(numberIsValid);
}

[Test]
public void IsValidNumber_WhenLettersInsteadOfNumber_ShouldReturnFalse()
Copy link

Choose a reason for hiding this comment

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

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

Обычно я делаю так: [Test][TestOf(nameof(NumberValidator.IsValidNumber))] public void ShouldBe[что-то]_When[что-то].

{
var validator = new NumberValidator(3, 2, true);

var numberIsValid = validator.IsValidNumber("a.sd");

Assert.IsFalse(numberIsValid);
}

[Test]
public void IsValidNumber_WhenSymbolsInsteadOfNumber_ShouldReturnFalse()
{
var validator = new NumberValidator(3, 2, true);
Copy link

Choose a reason for hiding this comment

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

Очень много раз повторяется строка var validator = new NumberValidator(3, 2, true);. Представь, что этот конструктор кто-то с неведомой целью изменит? Придется пройтись по всем тестам и в каждом что-то изменить. Давай этот момент обобщим


var numberIsValid = validator.IsValidNumber("2.!");

Assert.IsFalse(numberIsValid);
}

[Test]
public void IsValidNumber_WhenFractionalPartIsMissing_ShouldReturnTrue()
{
var validator = new NumberValidator(17, 2, true);

var numberIsValid = validator.IsValidNumber("0");

Assert.IsTrue(numberIsValid);
}

[Test]
public void IsValidNumber_WhenNumberIsNull_ShouldReturnFalse()
{
var validator = new NumberValidator(17, 2, true);

var numberIsValid = validator.IsValidNumber(null!);

Assert.IsFalse(numberIsValid);
}

[Test]
public void IsValidNumber_WhenPassNumberIsEmpty_ShouldReturnFalse()
{
var validator = new NumberValidator(3, 2, true);

var numberIsValid = validator.IsValidNumber("");

Assert.IsFalse(numberIsValid);
}

[Test]
public void IsValidNumber_WhenIntPartWithNegativeSignMoreThanPrecision_ShouldReturnFalse()
{
var validator = new NumberValidator(3, 2, true);

var numberIsValid = validator.IsValidNumber("-0.00");

Assert.IsFalse(numberIsValid);
}

[Test]
public void IsValidNumber_WhenIntPartWithPositiveSignMoreThanPrecision_ShouldReturnFalse()
{
var validator = new NumberValidator(3, 2, true);

var numberIsValid = validator.IsValidNumber("+1.23");

Assert.IsFalse(numberIsValid);
}

[Test]
public void IsValidNumber_WhenFractionalPartMoreThanScale_ShouldReturnFalse()
Copy link

Choose a reason for hiding this comment

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

8 последних тестов имеют схожую структуру:
// инициализация корректного валидатора
// передача некорректного параметра
// ожидание, что валидатор вернет false

Всё это можно (и нужно) обобщить

{
var validator = new NumberValidator(17, 2, true);

var numberIsValid = validator.IsValidNumber("0.000");

Assert.IsFalse(numberIsValid);
}
}

public class NumberValidator
{
Expand Down
140 changes: 71 additions & 69 deletions cs/HomeExercises/ObjectComparison.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,81 +3,83 @@

namespace HomeExercises
{
public class ObjectComparison
{
[Test]
[Description("Проверка текущего царя")]
[Category("ToRefactor")]
public void CheckCurrentTsar()
{
var actualTsar = TsarRegistry.GetCurrentTsar();
public class ObjectComparison
{
[Test]
[Description("Проверка текущего царя")]
[Category("ToRefactor")]
public void CheckCurrentTsar()
{
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));
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);
// Перепишите код на использование Fluent Assertions.
Copy link

Choose a reason for hiding this comment

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

В комменте уже нет смысла

actualTsar.Should()
.BeEquivalentTo(expectedTsar, x => x
.Excluding(x => x.Id)
.Excluding(x => x.Parent!.Id));
}

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);
}
[Test]
[Description("Альтернативное решение. Какие у него недостатки?")]
public void CheckCurrentTsar_WithCustomEquality()
{
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));

[Test]
[Description("Альтернативное решение. Какие у него недостатки?")]
public void CheckCurrentTsar_WithCustomEquality()
{
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));
// Какие недостатки у такого подхода?
//Данный подход делает класс труднорасширяемым, ведь при добавлении новых полей в класс Person
Copy link

Choose a reason for hiding this comment

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

Казалось бы мелочь, но после // принято ставить пробел - так читается проще 🙃

//нужно переписывать и метод сравнения для всех этих полей, так же новые поля cможет добавить
//другой разработчик, который не знает о таком методе сравнения, что приведет к новым, неотловоенным ошибкам -
//новые поля не будут сравниваться.
//В моем решении (CheckCurrentTsar), такой ошибки не возникнет и класс Person сможет без ошибок расширяться
//при условии, что сравниваться будут все поля, кроме Id (так же и их Parent), так как код написан не перебиранием
//всех полей для сравнения, а сравнением объекта в целом с исключением его Id.
Copy link

Choose a reason for hiding this comment

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

Да, круто, всё верно.

Assert.True(AreEqual(actualTsar, expectedTsar));
}

// Какие недостатки у такого подхода?
Assert.True(AreEqual(actualTsar, expectedTsar));
}
private bool AreEqual(Person? actual, Person? expected)
{
if (actual == expected) return true;
if (actual == null || expected == null) return false;
return
actual.Name == expected.Name
&& actual.Age == expected.Age
&& actual.Height == expected.Height
&& actual.Weight == expected.Weight
&& AreEqual(actual.Parent, expected.Parent);
Copy link

Choose a reason for hiding this comment

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

Ранее не обратил внимание, но объясни ещё, пж, про 52 строку - в чем тут потенциальная опасность?

}
}

private bool AreEqual(Person? actual, Person? expected)
{
if (actual == expected) return true;
if (actual == null || expected == null) return false;
return
actual.Name == expected.Name
&& actual.Age == expected.Age
&& actual.Height == expected.Height
&& actual.Weight == expected.Weight
&& AreEqual(actual.Parent, expected.Parent);
}
}
public class TsarRegistry
{
public static Person GetCurrentTsar()
{
return new Person(
"Ivan IV The Terrible", 54, 170, 70,
new Person("Vasili III of Russia", 28, 170, 60, null));
}
}

public class TsarRegistry
{
public static Person GetCurrentTsar()
{
return new Person(
"Ivan IV The Terrible", 54, 170, 70,
new Person("Vasili III of Russia", 28, 170, 60, null));
}
}
public class Person
{
public static int IdCounter = 0;
public int Age, Height, Weight;
public string Name;
public Person? Parent;
public int Id;

public class Person
{
public static int IdCounter = 0;
public int Age, Height, Weight;
public string Name;
public Person? Parent;
public int Id;

public Person(string name, int age, int height, int weight, Person? parent)
{
Id = IdCounter++;
Name = name;
Age = age;
Height = height;
Weight = weight;
Parent = parent;
}
}
public Person(string name, int age, int height, int weight, Person? parent)
{
Id = IdCounter++;
Name = name;
Age = age;
Height = height;
Weight = weight;
Parent = parent;
}
}
}