Conversation
| ClassicAssert.AreEqual(expectedTsar.Parent.Parent, actualTsar.Parent.Parent); | ||
| // Преимущества решение с FluentAssertions: | ||
| // - Легко расширяем при добавлении новых свойств в Person | ||
| // - Автоматически проверяем все свойства, включая вложенные объекты |
There was a problem hiding this comment.
А все ли вложенные объекты мы проверим? Советую почитать доку FluentAssertions https://fluentassertions.com/
There was a problem hiding this comment.
Да, я неправильно написал, что все вложенные объекты будут обязательно проверены. Вроде бы глубина вложенности будет по умолчанию только 10 уровней для избежания бесконечной рекурсии, но можно использовать .AllowingInfiniteRecursion() для снятия этого ограничения. Но наверное тут нам это не нужно.
| // - Автоматически проверяем все свойства, включая вложенные объекты | ||
| // - При несовпадении конкретного свойства будет выдана информация какое именно свойство не совпало | ||
| actualTsar.Should().BeEquivalentTo(expectedTsar, options => options | ||
| .Excluding(p => p.Id) |
There was a problem hiding this comment.
Два раза вызываем Excluding для того чтобы исключить поле Id у объектов одного и того же класса Person. Можем ли мы как-то сделать общую настройку, которая будет затрагивать все экземпляры одного класса?
К примеру, если сейчас появится поле Sister того же класса, то лля него тоже нужно будет вызвать Excluding, что неудобно
| // Какие недостатки у такого подхода? | ||
| // Недостатки подхода с CustomEquality: | ||
| // - Нет детальной информации о том, какое именно свойство не совпало | ||
| // - При добавлении новых свойств в Person нужно менять метод AreEqual |
There was a problem hiding this comment.
Помимо того что метод AreEqual неудобный, есть ли у него какие-то ещё проблемы, из-за которых что-то может пойти не так при работе с ним?
There was a problem hiding this comment.
Наверное проблемой может быть переполнения стека при большом уровне вложенности. Метод уйдет в бесконечную рекурсию.
There was a problem hiding this comment.
Стоит убрать после окончания рефакторинга
| // - Легко расширяем при добавлении новых свойств в Person | ||
| // - Автоматически проверяем все свойства, включая вложенные объекты | ||
| // - При несовпадении конкретного свойства будет выдана информация какое именно свойство не совпало | ||
| actualTsar.Should().BeEquivalentTo(expectedTsar, options => options |
There was a problem hiding this comment.
Ещё хорошей практикой будет выделить этот способ проверки в отдельный метод, как снизу сделано с AreEqual. Это удобно, когда тебе нужно для разных тесткейсов вызывать сравнение объектов класса Person, а логика сравнения лежит в одном месте.
| [TestCase("1.23a4", 10, 7, true)] | ||
| [TestCase("t", 4, 3, true)] | ||
| [TestCase("b.0", 4, 3, true)] | ||
| [TestCase("1.2b34", 10, 7, true)] |
There was a problem hiding this comment.
Вот это всё по факту дубли. Для того чтобы составить тесткейсы, могу предложить дополнительно зайти на какой-либо сайт с онлайн Regex-ом. В него можно вставить паттерн из метода и посмотреть, что именно там происходит. Тогда получится избавиться от немалого количества дублей
| [TestCase(2, 1, "123", false)] | ||
| [TestCase(3, 0, "-12", true)] | ||
| [TestCase(3, 1, "-123", false)] | ||
| public void IsValid_IntegerBounds_Expected(int precision, int scale, string value, bool expected) |
There was a problem hiding this comment.
Части с IntegerBound и Expected не понятны, стоит дополнить аналогично верхним тестам
| [TestCase("11.234", 5, 4, true)] | ||
| [TestCase("11,234", 5, 4, true)] | ||
| [TestCase("1.234", 5, 3, true)] | ||
| public void IsValid_WhenValidNumbers_ReturnTrue(string value, int precision, int scale, bool onlyPositive) |
There was a problem hiding this comment.
Стоит полностью указывать название метода на который ориентирован тест. В данном случае корректнее будет назвать его IsValidNumber_WhenValidValue_ReturnTrue.
| [TestCase(8, 4, "12.34", true)] | ||
| [TestCase(5, 4, "11.234", true)] | ||
| [TestCase(7, 3, "1.234", true)] | ||
| [TestCase(8, 4, "+123,4567", true)] | ||
| [TestCase(4, 2, "+12.34", false)] | ||
| [TestCase(6, 2, "1.234", false)] | ||
| [TestCase(5, 4, "-12.3", true)] | ||
| [TestCase(8, 4, "-1234.567", true)] | ||
| [TestCase(12, 4, "-123.4567", true)] | ||
| [TestCase(6, 3, "-12,345", true)] | ||
| [TestCase(4, 1, "-123,4", false)] | ||
| [TestCase(10, 2, "-123.456", false)] |
There was a problem hiding this comment.
Тут тоже присутствуют дубли, а также постоянно меняются значения, из-за чего не понятно что именно проверяем
| [TestCase(1, -1, false)] | ||
| [TestCase(1, 2, false)] | ||
| [TestCase(1, 1, false)] | ||
| public void Constructor_WhenInvalidArgs_Throws(int precision, int scale, bool onlyPositive) |
There was a problem hiding this comment.
А ещё касательно названий тестов. Есть разные способы составления названия, здесь используется сейчас <название метода>When<условие><Результат>. У нас в команде используется к примеру другая семантика - Should_<ожидаемый результат в snake case>_when<условие в snake case>, тестируемый метод мы обычно не указываем, т.к. тестовые файлы поделены по методам класса.
Так что можешь в целом почитать/посмотреть как в проектах шарповых идёт именование тестов и выбрать себе предпочтительный способ, если текущий кажется не сильно удобным, главное чтобы он был стандартизированным и удобным тебе.
| ClassicAssert.IsFalse(new NumberValidator(3, 2, true).IsValidNumber("-1.23")); | ||
| ClassicAssert.IsFalse(new NumberValidator(3, 2, true).IsValidNumber("a.sd")); | ||
| [TestCase(-1, 2, true, TestName = "precision is negative")] | ||
| [TestCase(0, 0, false, TestName = "precision is zero")] |
There was a problem hiding this comment.
У тебя ниже есть тест "scale equals precision", однако если взглянуть на этот тест, может появится впечатление что они проверяют одно и то же. Когда мы просматриваем тесты, мы не знаем в каком порядке чередуются проверки в коде, поэтому следует писать тест так, чтобы он падал только на необходимой для нас проверке, то есть здесь желательно передавать scale, который больше нуля.
| [TestCase("-1.23", 3, 2, true, TestName = "negative when positive only")] | ||
| [TestCase("1.2a", 3, 2, true, TestName = "invalid character in fraction")] | ||
| [TestCase("-123", 4, 0, true, TestName = "negative integer when positive only")] | ||
| [TestCase("-1.23", 4, 2, true, TestName = "negative fractional when positive only")] |
There was a problem hiding this comment.
Дубль для теста "negative when positive only"?
| [TestCase("", 1, 0, false, TestName = "empty string")] | ||
| [TestCase(null, 1, 0, false, TestName = "null value")] | ||
| [TestCase("01.23", 3, 2, true, TestName = "leading zeros")] | ||
| [TestCase("+0.00", 3, 2, true, TestName = "sign with zero")] |
There was a problem hiding this comment.
Непонятное название тесткейса. Фактически падаем не из-за того что знак указан вместе с нулём, а потому что precision == 3, а целая и дробная часть в сумме равны 4, потому как туда входит знак
|
|
||
| [TestCase("", 1, 0, false, TestName = "empty string")] | ||
| [TestCase(null, 1, 0, false, TestName = "null value")] | ||
| [TestCase("01.23", 3, 2, true, TestName = "leading zeros")] |
There was a problem hiding this comment.
Не из-за этого падаем. Если установить precision = 5, то тест упадёт. Стоит добавить его в валидные случаи, такие числа могут быть валидными
| validator.IsValidNumber(value).Should().BeTrue(); | ||
| } | ||
|
|
||
| [TestCase("", 1, 0, false, TestName = "empty string")] |
There was a problem hiding this comment.
Можно к этим тесткейсам добавить случаи, когда перед числовым значением есть пробелы, после значения пробелы и случай когда до и после они есть
| [TestCase(null, 1, 0, false, TestName = "null value")] | ||
| [TestCase("01.23", 3, 2, true, TestName = "leading zeros")] | ||
| [TestCase("+0.00", 3, 2, true, TestName = "sign with zero")] | ||
| [TestCase("-1.23", 3, 2, true, TestName = "negative when positive only")] |
There was a problem hiding this comment.
Если посмотреть в дебаге, то до желаемой проверки мы не добираемся, тест проходит не из-за причин указанных в названии
| [TestCase("01.23", 3, 2, true, TestName = "leading zeros")] | ||
| [TestCase("+0.00", 3, 2, true, TestName = "sign with zero")] | ||
| [TestCase("-1.23", 3, 2, true, TestName = "negative when positive only")] | ||
| [TestCase("1.2a", 3, 2, true, TestName = "invalid character in fraction")] |
There was a problem hiding this comment.
стоит добавить случаи когда они в целой части, а также когда просто внутрь строка без чисел попадает. Можно закинуть также случаи со строкой со специальными символами
| } | ||
|
|
||
| [TestCase("123", 2, 1, false, TestName = "integer exceeds precision")] | ||
| [TestCase("-12", 2, 1, false, TestName = "negative integer exceeds precision")] |
There was a problem hiding this comment.
А зачем здесь scale отличный от нуля, если проверка целой части?
| [TestCase(1, -1, false, TestName = "scale is negative")] | ||
| [TestCase(1, 2, false, TestName = "scale greater than precision")] | ||
| [TestCase(1, 1, false, TestName = "scale equals precision")] | ||
| public void Constructor_WhenInvalidArgs_ThrowsArgumentException(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
2 participants
No description provided.