Conversation
| actualTsar | ||
| .Should() | ||
| .BeEquivalentTo(expectedTsar, options => options | ||
| .ExcludingMembersNamed("Id")); |
There was a problem hiding this comment.
Если свойство Id в классе Person переименуют, то в данном случае тест упадёт, а поведение в тесте не сохранится. Для этого можно сослаться напрямую на название свойства в классе
There was a problem hiding this comment.
Это решение возникло вследствие попытки исключить свойство Id у всех "родственников". Если в Person появятся дети, браться, сёстры, то каждого придётся вносить в исключение. Изначально решение выглядело так:
actualTsar
.Should()
.BeEquivalentTo(expectedTsar, options => options
.Excluding(person => person.Id)
.Excluding(person => person.Parent.Id));
| actualTsar | ||
| .Should() | ||
| .BeEquivalentTo(expectedTsar, options => options | ||
| .ExcludingMembersNamed("Id")); |
There was a problem hiding this comment.
Сейчас этот способ скроет для проверки все свойства и поля с именем Id. Если появится объект для которого мы хотим оставить сохранения по Id, то мы не сможем этого сделать. Нужно как-то сделать так, чтобы скрывали это свойство при сравнении только у конкретного класса.
| ClassicAssert.True(AreEqual(actualTsar, expectedTsar)); | ||
|
|
||
| //Какие недостатки у такого подхода? | ||
| //Недостатки: |
There was a problem hiding this comment.
Помимо того что метод AreEqual неудобный, есть ли у него какие-то ещё проблемы, из-за которых что-то может пойти не так при работе с ним?
There was a problem hiding this comment.
Теоретически при добавлении "родственников" может получится цикл, а значит и бесконечная рекурсия.
There was a problem hiding this comment.
Ну не только это на самом деле прям самый крайний случай. При достаточно большой структуре, которая будет иметь много Parent-ов, мы просто упадём со StackOverflowException т.к. зайдём слишком глубоко.
There was a problem hiding this comment.
Стоит убрать после окончания рефакторинга
| //Преимущества такой реализации: | ||
| //-тест занимает меньше строк кода, | ||
| //-добавление новых свойств не приводит к изменению теста (кроме самой инициализации expectedTsar), | ||
| //-добавление вложенных объектов не приводит к изменению теста (кроме самой инициализации expectedTsar), |
There was a problem hiding this comment.
А все ли вложенные объекты мы проверим? Советую почитать доку FluentAssertions https://fluentassertions.com/
There was a problem hiding this comment.
Нет, проверяются только до 10 уровня вложения, но можно отключить при помощи AllowingInfiniteRecursion
| [TestCase(-1, 2, true)] | ||
| [TestCase(17, -1, true)] | ||
| [TestCase(17, 18, true)] | ||
| public void Constructor_Should_Throw_For_InvalidParameters(int precision, int scale, bool onlyPositive) |
There was a problem hiding this comment.
Касательно названий тестов. Есть разные способы составления названия, сейчас ты названия пишешь неструктурированно, такое сложно читать и вникать, т.к. не понятно где условие, а где возвращаемый результат. Обычно используют <название метода>When<условие><Результат>. У нас в команде используется к примеру другая семантика - Should_<ожидаемый результат в snake case>_when<условие в snake case>, тестируемый метод мы обычно не указываем, т.к. тестовые файлы поделены по методам класса.
Так что можешь в целом почитать/посмотреть как в проектах шарповых идёт именование тестов и выбрать себе предпочтительный способ, а текущие названия поменять
| [TestCase(-1, 2, true)] | ||
| [TestCase(17, -1, true)] | ||
| [TestCase(17, 18, true)] | ||
| public void Constructor_Should_Throw_For_InvalidParameters(int precision, int scale, bool onlyPositive) |
There was a problem hiding this comment.
Вообще здесь было бы идеально перебирать все варианты значения onlyPositive, т.к. никакого влияния на конструктор он не имеет, но внутрь передаётся. Можно реализовать это через TestCaseSource https://docs.nunit.org/articles/nunit/writing-tests/attributes/testcasesource.html
| [TestCase(-1, 2, true)] | ||
| [TestCase(17, -1, true)] | ||
| [TestCase(17, 18, true)] |
| [TestCase(5, 2, false)] | ||
| [TestCase(10, 5, true)] | ||
| [TestCase(10, 0, false)] |
There was a problem hiding this comment.
А здесь зачем целых три тесткейса? Можно оставить было бы вообще один
| [TestCase(17, 2, true, null, false)] | ||
| [TestCase(17, 2, true, "", false)] | ||
| [TestCase(17, 2, true, "a.1d", false)] | ||
| [TestCase(2, 1, true, "+0.0", false)] | ||
| [TestCase(2, 1, true, "00.0", false)] | ||
| [TestCase(3, 1, true, "0.00", false)] | ||
| [TestCase(17, 2, true, "-0.0", false)] | ||
| [TestCase(17, 2, true, "123.45", true)] | ||
| [TestCase(17, 2, true, "123,45", true)] | ||
| [TestCase(17, 2, true, "+0.0", true)] | ||
| [TestCase(17, 2, false, "-0.0", true)] | ||
| [TestCase(17, 0, true, "0", true)] | ||
| [TestCase(17, 0, false, "-10", true)] |
| } | ||
|
|
||
| [Test, TestCaseSource(typeof(TestData), nameof(TestData.InvalidConstructorParameters))] | ||
| public void Constructor_Should_ReturnThrow_WhenInvalidParameters(int precision, int scale, bool onlyPositive) |
There was a problem hiding this comment.
Название нужно переделать. Строка Should_ReturnThrow во-первых зачем-то разделена нижним подчёркиванием(либо делай подчёркивание между каждым словом, либо делай только между смысловыми частями <метод><что возвращает><при каких условиях>, во-вторых метод не возвращает Throw. Если ты хочешь так его назвать, корректнее будет Constructor_shoud_throw_<тип ошибки>_when_invalid_parameters
There was a problem hiding this comment.
Constructor_ShouldThrowArgumentException_WhenInvalidParameters Вот такой вариант правильный?
| } | ||
| } | ||
|
|
||
| [Test, TestCaseSource(typeof(TestData), nameof(TestData.InvalidConstructorParameters))] |
There was a problem hiding this comment.
Можно оставить только атрибут TestCaseSource, typeof не обязателен
| action.Should().NotThrow(); | ||
| } | ||
|
|
||
| [Test] |
| [TestCase(17, 2, false, "123,45", TestName = "Valid number with with ',' between integer and scale parts")] | ||
| [TestCase(17, 0, true, "123", TestName = "Valid integer without scale part")] | ||
| [TestCase(17, 2, true, "0000123.45", TestName = "Valid number with leading zeros")] | ||
| [TestCase(int.MaxValue, 2, true, "2147483647.22", TestName = " Max int precision with scale part")] |
There was a problem hiding this comment.
Да, ошибся. Смешалось в голове самое число и количество знаков в нём.
| } | ||
|
|
||
| [TestCase(10, 5, true, TestName = "Valid positive number")] | ||
| public void ConstructorShould_ReturnNotThrow_WhenValidParameters(int precision, int scale, bool onlyPositive) |
There was a problem hiding this comment.
Название очень сложно читать, почитай в комментах я тебе уже написал про наименование тестов
| [Test] | ||
| public void Test() | ||
| [TestCase(17, 2, true, "1.2.3", TestName = "multiple dots")] | ||
| [TestCase(17, 2, true, "--123", TestName = "double minus")] |
There was a problem hiding this comment.
Есть проверка на двойной минус, но не на плюс. Почему решил добавить её?
There was a problem hiding this comment.
Хотел проверить двойной знак перед числом. Подумал, что с "++" будет повторяющийся тест.
| [TestCase(17, 2, true, "12a.3", TestName = "Invalid character in number")] | ||
| [TestCase(17, 2, true, "\n1.23", TestName = "Special character before")] | ||
| [TestCase(17, 2, true, "1234.", TestName = "Trailing dot without fractional part")] | ||
| [TestCase(17, 2, true, ".12", TestName = "Leading dot without integer part")] |
There was a problem hiding this comment.
onlyPositive во всех тестах равен true, но по факту влияет только на один тест. Лучше сделать его true только в тесте в котором он имеет смысл
| [TestCase(17, 2, true, "\n1.23", TestName = "Special character before")] | ||
| [TestCase(17, 2, true, "1234.", TestName = "Trailing dot without fractional part")] | ||
| [TestCase(17, 2, true, ".12", TestName = "Leading dot without integer part")] | ||
| [TestCase(17, 2, true, "-123", TestName = "Negative number with onlyPositive=true")] |
There was a problem hiding this comment.
Тут хорошим тестом видится тест на 0 с разными знаками
| [TestCase(17, 2, true, "1234.", TestName = "Trailing dot without fractional part")] | ||
| [TestCase(17, 2, true, ".12", TestName = "Leading dot without integer part")] | ||
| [TestCase(17, 2, true, "-123", TestName = "Negative number with onlyPositive=true")] | ||
| public void IsValidNumberShould_ReturnFalse_WhenInvalidNumbers(int precision, int scale, bool onlyPositive, |
There was a problem hiding this comment.
Всё ещё не достаточно кейсов. Пройдись по проверкам которые указаны в методе который проверяешь и посмотри на какие из них ещё нет теста
| ClassicAssert.True(AreEqual(actualTsar, expectedTsar)); | ||
|
|
||
| //Какие недостатки у такого подхода? | ||
| //Недостатки: |
There was a problem hiding this comment.
Ну не только это на самом деле прям самый крайний случай. При достаточно большой структуре, которая будет иметь много Parent-ов, мы просто упадём со StackOverflowException т.к. зайдём слишком глубоко.
2 participants
@tripples25