Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
2 changes: 1 addition & 1 deletion cs/HomeExercises/HomeExercises.csproj
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFramework>netcoreapp3.1</TargetFramework>
<TargetFramework>net6.0</TargetFramework>
<AppendTargetFrameworkToOutputPath>false</AppendTargetFrameworkToOutputPath>
<RootNamespace>HomeExercises</RootNamespace>
<AssemblyName>ObjectComparison</AssemblyName>
Expand Down
25 changes: 25 additions & 0 deletions cs/HomeExercises/HomeExercises.sln
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@

Microsoft Visual Studio Solution File, Format Version 12.00
# Visual Studio Version 17
VisualStudioVersion = 17.5.002.0
MinimumVisualStudioVersion = 10.0.40219.1
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "HomeExercises", "HomeExercises.csproj", "{B4229E79-0D13-4186-BD8D-89CAC9F1EA8A}"
EndProject
Global
GlobalSection(SolutionConfigurationPlatforms) = preSolution
Debug|Any CPU = Debug|Any CPU
Release|Any CPU = Release|Any CPU
EndGlobalSection
GlobalSection(ProjectConfigurationPlatforms) = postSolution
{B4229E79-0D13-4186-BD8D-89CAC9F1EA8A}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{B4229E79-0D13-4186-BD8D-89CAC9F1EA8A}.Debug|Any CPU.Build.0 = Debug|Any CPU
{B4229E79-0D13-4186-BD8D-89CAC9F1EA8A}.Release|Any CPU.ActiveCfg = Release|Any CPU
{B4229E79-0D13-4186-BD8D-89CAC9F1EA8A}.Release|Any CPU.Build.0 = Release|Any CPU
EndGlobalSection
GlobalSection(SolutionProperties) = preSolution
HideSolutionNode = FALSE
EndGlobalSection
GlobalSection(ExtensibilityGlobals) = postSolution
SolutionGuid = {5E766B1C-5638-4495-AC79-F7F4EC79629C}
EndGlobalSection
EndGlobal
93 changes: 74 additions & 19 deletions cs/HomeExercises/NumberValidatorTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,28 +5,77 @@

namespace HomeExercises
{
[TestFixture]

Choose a reason for hiding this comment

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

Можешь объяснить какую роль выполняет этот атрибут? Почему он нужен в этой ситуации?

Copy link
Author

Choose a reason for hiding this comment

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

Этим атрибутом помечается класс, содержащий тесты.
Насколько я знаю, ему можно передавать параметры, которые прокинуться в конструктор класса.
Таким образом можно автоматически создавать несколько классов тестов с разными настройками (действие похоже на атрибут TestCase для метода).
Использование этого атрибута не является обязательным, но я решил его добавить, чтобы подчеркнуть то, что NumberValidatorTests содержит тесты. Хотя это понятно и из названия класса :)

Choose a reason for hiding this comment

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

я думаю, что нет смысла навешивать этот атрибут, когда нет прямой необходимости (например, создания тестового класса с разными параметрами или параллельный запуск тестов на уровне фикстуры), поэтому я бы убрала) Но в целом это не критично

public class NumberValidatorTests

Choose a reason for hiding this comment

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

Для более простой навигации по проекту лучше разносить классы по отдельным файлам и называть их соответствующе + обычно тесты выносят в отдельный проект, чтобы не выпускать их вместе с основным кодом на рабочую площадку (относится к обеим частям задания)

Copy link
Author

Choose a reason for hiding this comment

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

Хорошо

{
[Test]
public void Test()
[Category("NumberValidator.Constructor; Exception")]

Choose a reason for hiding this comment

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

про категорию аналогично другому комменту

Copy link
Author

Choose a reason for hiding this comment

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

Убрал)

[TestCase(-5, 5, TestName = "Precision < 0")]
[TestCase(0, 5, TestName = "Precision = 0")]
[TestCase(5, -5, TestName = "Scale < 0")]
[TestCase(5, 5, TestName = "Precision = Scale")]
[TestCase(5, 10, TestName = "Precision < Scale")]
public void Constructor_With_IncorrectArguments_Should_ThrowException(int precision, int scale)

Choose a reason for hiding this comment

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

Я бы все-таки советовала именование тестов в формате [Метод (или конструктор)]should[что-то]when[условие]

{
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"));
var action = new Action(() => new NumberValidator(precision, scale));

Choose a reason for hiding this comment

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

это на самом деле Func а не Action

имхо (можно не исправлять) я бы написала Func<NumberValidator> action = () => new NumberValidator(precision, scale);

action.Should().Throw<ArgumentException>();
}

[Category("NumberValidator.Constructor; No exception")]
[TestCase(10, 5, false, TestName = "Correct parameters")]

Choose a reason for hiding this comment

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

А какие параметры будут корректными? Мне понравился формат, в котором ты описал некорректные в названиях, тут можно сделать так же)

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

[Category("NumberValidator.Constructor; No exception")]
[TestCase(15, TestName = "Only precision is given")]
public void Constructor_With_OneArgument_ShouldNot_ThrowException(int precision)
{
var action = new Action(() => new NumberValidator(precision));
action.Should().NotThrow();
}

[Category("NumberValidator.IsValid(...); Correct values")]
[TestCase(1, 0, true, "5", TestName = "No fraction part")]
[TestCase(3, 0, true, "+99", TestName = "Sign and no fraction part")]
[TestCase(4,2, false,"-1.25", TestName = "Negative number")]
public void IsValid_ShouldReturn_True_When_CorrectValue(int precision, int scale,
bool onlyPositive, string value)
{
var validator = new NumberValidator(precision, scale, onlyPositive);
validator.IsValidNumber(value).Should().BeTrue();
}

[Category("NumberValidator.IsValid(...); Separators")]
[TestCase(4, 2, true, "+5,33", TestName = "Works with dot as separator")]
[TestCase(4, 2, true, "+5.33", TestName = "Works with comma as separator")]
public void IsValid_ShouldWork_WithDotAndComma_Separators(int precision, int scale,
bool onlyPositive, string value)
{
var validator = new NumberValidator(precision, scale, onlyPositive);
validator.IsValidNumber(value).Should().BeTrue();
}

[Category("NumberValidator.IsValid(...); Incorrect values")]
[TestCase(3, 2, true, "00.00", TestName = "Actual precision < expected")]
[TestCase(3, 2, true, "+0.00",
TestName = "Sign was not taken into account when forming precision value")]
[TestCase(17, 2, true, "0.000", TestName = "Actual scale < expected")]
[TestCase(3, 2, true, "a.sd", TestName = "Letters instead of digits")]
[TestCase(3, 2, true, "-1.25", TestName = "Negative number when (onlyPositive = true)")]
[TestCase(10, 5, true, "+", TestName = "No number given")]
[TestCase(10, 5, true, "+ 5, 956", TestName = "Spaces are forbidden")]
[TestCase(10, 5, true, "45!34", TestName = "Incorrect separator")]
[TestCase(10, 5, true, "++3.45", TestName = "Two signs")]
[TestCase(10, 5, true, "2,,66", TestName = "Two separators")]
[TestCase(10, 5, true, "", TestName = "Empty string as number")]
public void IsValid_ShouldReturn_False_When_IncorrectValue(int precision, int scale,

Choose a reason for hiding this comment

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

Какие еще граничные ситуации проверяют при таком тестировании обычно? (есть еще как минимум 1 очень простой распространенный тесткейс)

bool onlyPositive, string value)
{
var validator = new NumberValidator(precision, scale, onlyPositive);
validator.IsValidNumber(value).Should().BeFalse();
}
}

Expand All @@ -42,10 +91,13 @@ public NumberValidator(int precision, int scale = 0, bool onlyPositive = false)
this.precision = precision;
this.scale = scale;
this.onlyPositive = onlyPositive;

if (precision <= 0)
throw new ArgumentException("precision must be a positive number");

if (scale < 0 || scale >= precision)
throw new ArgumentException("precision must be a non-negative number less or equal than precision");

numberRegex = new Regex(@"^([+-]?)(\d+)([.,](\d+))?$", RegexOptions.IgnoreCase);
}

Expand All @@ -61,11 +113,13 @@ public bool IsValidNumber(string value)
return false;

var match = numberRegex.Match(value);

if (!match.Success)
return false;

// Знак и целая часть
var intPart = match.Groups[1].Value.Length + match.Groups[2].Value.Length;

// Дробная часть
var fracPart = match.Groups[4].Value.Length;

Expand All @@ -74,6 +128,7 @@ public bool IsValidNumber(string value)

if (onlyPositive && match.Groups[1].Value == "-")
return false;

return true;
}
}
Expand Down
60 changes: 47 additions & 13 deletions cs/HomeExercises/ObjectComparison.cs
Original file line number Diff line number Diff line change
@@ -1,30 +1,55 @@
using FluentAssertions;
using System.Text.RegularExpressions;
using FluentAssertions;
using NUnit.Framework;

namespace HomeExercises
{
public class ObjectComparison

Choose a reason for hiding this comment

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

В названии лучше отразить, что это класс с тестами

{
[Test]
[Description("Проверка текущего царя")]
[Description("Проверка текущего царя. Информативная реализация")]

Choose a reason for hiding this comment

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

Description не понадобится, если дать тесту достаточно понятное название

[Category("ToRefactor")]

Choose a reason for hiding this comment

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

Кажется, что эта категория не нужна)

public void CheckCurrentTsar()
{
// Эта реализация теста будет выводить информативные сообщения!

Choose a reason for hiding this comment

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

Комментарии в коде - не очень хорошая практика. Лучше использовать только при большой необходимости (когда прям реально совсем непонятно и по-другому не переписать). Но раз уж было такое задание, то можно написать ответ на вопрос где-то внизу или вообще в комментах к pr

Copy link
Author

Choose a reason for hiding this comment

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

Ок! Удаляю комментарии

Copy link
Author

Choose a reason for hiding this comment

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

Я удалил тест, который использовал некорректный метод сравнения, чтобы убрать путаницу из кода.
Метод AreEqual(...) я оставил и к нему переместил комментарии.

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);
// В начальном наборе тестов был сделан акцент на то, что у объекта класса Person
// должнен быть родитель первого поколения.
actualTsar.Parent.Should().NotBe(null, "Должен быть родитель 1-го поколения");

// Полностью повторяем логику проверки до рефакторинга.
// Из родителей проверяем только первое поколение, причём не сравниваем у них вес.
actualTsar.Should().BeEquivalentTo(expectedTsar, config =>

Choose a reason for hiding this comment

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

Думаю, что такое изначальное поведение теста было неправильным. Цари с разными предками (хоть и не 1 колена) не должны являться равными. Предков предыдущих поколений тоже надо проверять.

+отсутствие проверки веса родителя тоже скорее является недостатком изначального теста.

Поэтому думаю, что стоит оставить только полный тест

Copy link
Author

Choose a reason for hiding this comment

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

Ок! Уберу этот тест

{
return config.Excluding(person => person.Id)
.Excluding(person => person.Parent!.Parent)
.Excluding(person => person.Parent!.Weight)
.Excluding(person => person.Parent!.Id);
});
}

[Test]
[Description("Проверка текущего царя и всех его родителей")]

Choose a reason for hiding this comment

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

Тут аналогично с description

[Category("AlternativeTest")]

Choose a reason for hiding this comment

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

Я не вижу смысла всегда весить атрибут категории
На мой взгляд, он полезен только в случае, когда нужно отдельно запускать (или наоборот пропускать) какие-то определенные тесты

public void CheckCurrentTsar_WithCustomEquality_Informative()

Choose a reason for hiding this comment

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

Кажется, что название теста неинформативно

Copy link
Author

Choose a reason for hiding this comment

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

Исправлю

{
// Этим шаблоном мы исключаем все идентификаторы из проверки.
// То есть исключаем Id у 1-го, 2-го, ..., n - 1, n родителя.
const string excludingPattern = @"^(Parent\.)*Id$";

Choose a reason for hiding this comment

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

Тут явно можно обойтись без regex, подумай как? (решение на поверхности на самом деле)

Copy link
Author

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));

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);
// Эта версия теста является информативным аналогом метода CheckCurrentTsar_WithCustomEquality(...)
// До сих пор возможна глубокая рекурсия!
actualTsar.Should().BeEquivalentTo(expectedTsar, config =>

Choose a reason for hiding this comment

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

А что будет, если дойдет до 10? 20? колена? Проверял?)

Copy link
Author

Choose a reason for hiding this comment

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

Если у сравниваемых объектов будет 9 родственников, то есть всего 10 звеньев в цепи для проверки, то всё отработает сравнительно быстро.
Если в цепи будет больше 10 звеньев, ты выпадет ошибка "The maximum recursion depth was reached."
При использовании метода AllowingInfiniteRecursion(), на моём ноутбуке задача для 10.000 родственников решалась за ~25 секунд.
При 50.000 родственников задача не успела решиться, так как операционке стало плохо, и я это дело принудительно завершил :)
изображение
изображение

Choose a reason for hiding this comment

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

Во! Круто, что потестил, всё верно
То есть нужно использовать метод AllowingInfiniteRecursion(), чтобы проверять больше предков
(Можно не править, главное, что дошел до этого)

{
return config.Excluding(ctx => Regex.IsMatch(ctx.SelectedMemberPath, excludingPattern));
});
}

[Test]
Expand All @@ -35,7 +60,16 @@ public void CheckCurrentTsar_WithCustomEquality()
var expectedTsar = new Person("Ivan IV The Terrible", 54, 170, 70,
new Person("Vasili III of Russia", 28, 170, 60, null));

// Какие недостатки у такого подхода?
/* Недостатки этого подхода:
* 1. AreEqual(...) сильно связан со структурой класса Person:
* любая модификация этого класса может потребовать изменения проверяющего метода.
* 2. AreEqual(...) включает в себя слишком много проверок. Из-за этого мы получим
* неинформативное сообщение об ошибке в случае разности проверяемых объектов.
* 3. В методе типа AreEqual(...) легко забыть о каком-либо свойстве, т.е. допустить ошибку в сравнении.
* 4. Если у проверяемого объекта много свойств, то AreEqual(...) тяжело читать.
* 5. Возможно переполнение стека из-за рекурсивных вызовов AreEqual(...) внутри самого себя.
* 6. Для любого другого класса придётся писать новую версию метода AreEqual(...).
*/
Assert.True(AreEqual(actualTsar, expectedTsar));
}

Expand Down