Conversation
… единый тестовый метод на несколько с TestCase'ами
| .Excluding(t => t.Id) | ||
| .Excluding(t => t.Parent.Id)); |
There was a problem hiding this comment.
t.Parent.Parent.Id учитываться будет :) И т.д по цепочке.
Давай подумаем в сторону того, как исключить сравнение для всех сущностей такого типа
There was a problem hiding this comment.
Поправил, теперь исключаю из сравнения все поля названные Id
| /* | ||
| * 1. В тестовом методе выше есть возможность отследить на каком из этапов сравнения двух экземпляров класса | ||
| * упал тест. Здесь же, используя кастомный метод сравнения AreEqual мы можем проверить только факт полного | ||
| * совпадения всех полей, и не сможем отследить какое именно поле отличается. | ||
| * 2. При добавлении новых полей в класс Person необходимо каждый раз переписывать метод AreEqual | ||
| * Ну и читаемость намного возрастает в сравнении с решением представленным выше, там просто | ||
| * нужно знать что делает BeEquivalentTo и понимать что его поведение не изменится, а здесь нужно разбираться | ||
| * в методе AreEqual который может время от времени меняться в зависимости от полей класса Person | ||
| */ |
There was a problem hiding this comment.
Есть еще недостаток, связанный с самой реализацией AreEquals.
Представь какое-нибудь большое генеалогическое дерево. Справится ли с ним AreEquals вообще?
There was a problem hiding this comment.
Нашел еще недостаток - если Parent у двух царей будет ссылаться друг на друга, то AreEqual войдет в бесконечный цикл проверок на этапе сравнения родителей. Это он, или есть еще какие-то недостатки?
| [TestCase(17, 2, true, "0.0", true)] | ||
| [TestCase(17, 2, true, "0", true)] | ||
| [TestCase(3, 2, true, "00.00", false)] | ||
| [TestCase(3, 2, true, "-0.00", false)] | ||
| [TestCase(3, 2, true, "+0.00", false)] | ||
| [TestCase(4, 2, true, "+1.23", true)] | ||
| [TestCase(3, 2, true, "+1.23", false)] | ||
| [TestCase(17, 2, true, "0.000", false)] | ||
| [TestCase(3, 2, true, "-1.23", false)] | ||
| [TestCase(3, 2, false, "-1.23", false)] | ||
| [TestCase(5, 2, false, "-1.23", true)] | ||
| [TestCase(10, 2, false,null, false)] | ||
| [TestCase(10, 2, false, "", false)] | ||
| [TestCase(10, 2, false, "abc", false)] | ||
| [TestCase(10, 2, false, "1..2", false)] | ||
| [TestCase(10, 2, false, "1.", false)] | ||
| [TestCase(10, 2, false, "1.2.3", false)] | ||
| [TestCase(10, 2, false, "a.sd", false)] |
There was a problem hiding this comment.
Давай представим, что у нас конкретный тест начал падать. Тогда мне нужно будет зайти в реализацию IsValidNumber, соотнести входной результат, и только тогда я пойму проблему.
Давай добавим информативности, чтобы мне было сразу понятно какая логика сломалась
There was a problem hiding this comment.
Добавил TestName каждому тест-кейсу
| [TestCase(10, 2, false, "1.", false)] | ||
| [TestCase(10, 2, false, "1.2.3", false)] | ||
| [TestCase(10, 2, false, "a.sd", false)] | ||
| public void IsValidNumber_ShouldBeAsExpected(int precision, int scale, bool onlyPositive, string number, bool expectedResult) |
There was a problem hiding this comment.
Лучше разделить на два метода: один проверяет успешные результаты, другой проверяет фейлы.
Сейчас это сделано через переменную expectedResult, что значительно снижает читаемость(нужно посмотреть TestCase, понять, что expectedResult соответствует последней переменной, посмотреть в код). И так для каждого TestCase'а.
| { | ||
| var createNumberValidator = () => new NumberValidator(precision, scale, onlyPositive); | ||
|
|
||
| createNumberValidator.Should().Throw<Exception>().Which.Should().BeOfType(expectedExceptionType); |
There was a problem hiding this comment.
А у тебя в конструкторе других типов концептуально и не может возникнуть)
There was a problem hiding this comment.
Согласен, но вдруг у разрабов класса возникнет потребность выбрасывать какие-нибудь другие исключения, например ArgumentOutOfRange вместо ArgumentException, или ArgumentNullException когда строка = null вместо return false в IsValidNumber. Я с этой целью делал так, чтобы если поменяется пул возможных исключений в классе было меньше боли добавлять на это больше тестов
There was a problem hiding this comment.
Я согласен, что такое может быть. Но у нас сама архитектура такого не предполагает.
Конкретно тут сошлюсь на KISS, т.к мы начинаем закладываться под то, чего 99% не будет
| [TestCase(-1, 2, true, typeof(ArgumentException))] | ||
| [TestCase(1, -1, false, typeof(ArgumentException))] | ||
| [TestCase(1, 1, false, typeof(ArgumentException))] |
There was a problem hiding this comment.
Можно еще кейсов накинуть. Например, когда precision=0. Корнер кейсы лучше всегда отдельно рассматривать в тестах.
Также стоит рассмотреть случаи, когда невалидные параметры пересекаются(выполняются оба условия)
There was a problem hiding this comment.
Теперь рассматриваю еще случаи когда:
- precision = 0
- scale > precision
- precision = 0 и scale < 0
- precision и scale < 0
| [TestCase(10, 2, false, "1.", false)] | ||
| [TestCase(10, 2, false, "1.2.3", false)] | ||
| [TestCase(10, 2, false, "a.sd", false)] | ||
| public void IsValidNumber_ShouldBeAsExpected(int precision, int scale, bool onlyPositive, string number, bool expectedResult) |
There was a problem hiding this comment.
Тоже можно еще кейсов накинуть.
Например, у тебя число может быть слишком большим.
Опять же корнер-кейсы, вроде precision заданное и precision самого числа совпадают.
null передать вместо строчки и т.д.
There was a problem hiding this comment.
Добавил еще больше проверок покрывающих корнер-кейсы
| [TestCase(5, 2, false, "123456", TestName = "IsValidNumber_ShouldBeFalse_WhenPrecisionExceededByIntegerPart")] | ||
| [TestCase(3, 0, false, "1234", TestName = "IsValidNumber_ShouldBeFalse_WhenIntegerPartLongerThanPrecision")] | ||
| [TestCase(5, 2, false, "000.000", TestName = "IsValidNumber_ShouldBeFalse_WhenTotalDigitsExceedPrecision")] | ||
| public void IsValidNumber_ShouldBeFalse_WhenParametersInvalid(int precision, int scale, bool onlyPositive, string number) |
There was a problem hiding this comment.
Все равно вижу непокрытые кейсы. Например, вместо точки можно запятую указать. Или другой символ.
Или long.MaxValue + 1. Или число вида ".1".
В общем давай еще подумаем какие мы не покрываем. В строчке у тебя буквально может быть что угодно
There was a problem hiding this comment.
А еще у тебя могут быть неэкранируемые символы, будет работать в этом случае метод? А если японские цифры использовать?
| { | ||
| var createNumberValidator = () => new NumberValidator(precision, scale, onlyPositive); | ||
|
|
||
| createNumberValidator.Should().Throw<Exception>().Which.Should().BeOfType(expectedExceptionType); |
There was a problem hiding this comment.
Я согласен, что такое может быть. Но у нас сама архитектура такого не предполагает.
Конкретно тут сошлюсь на KISS, т.к мы начинаем закладываться под то, чего 99% не будет
| [TestCase(5, 2, false, "123456", TestName = "IsValidNumber_ShouldBeFalse_WhenPrecisionExceededByIntegerPart")] | ||
| [TestCase(3, 0, false, "1234", TestName = "IsValidNumber_ShouldBeFalse_WhenIntegerPartLongerThanPrecision")] | ||
| [TestCase(5, 2, false, "000.000", TestName = "IsValidNumber_ShouldBeFalse_WhenTotalDigitsExceedPrecision")] | ||
| public void IsValidNumber_ShouldBeFalse_WhenParametersInvalid(int precision, int scale, bool onlyPositive, string number) |
There was a problem hiding this comment.
А еще у тебя могут быть неэкранируемые символы, будет работать в этом случае метод? А если японские цифры использовать?
| [TestCase(10, 2, false, "123", TestName = "IsValidNumber_ShouldBeTrue_WhenJapaneseDigitsUsed")] | ||
| [TestCase(10, 2, false, "123.45", TestName = "IsValidNumber_ShouldBeTrue_WhenJapaneseDigitsWithDotUsed")] |
There was a problem hiding this comment.
Я имел в виду какие-нибудь символы типа - 六(6 на кандзи)
Ну и учитывай, что у тебя еще и другие языки есть. Со своими цифрами.
Кейсы можно еще комбинировать - часть арабскими цифрами, часть другими и тд
| [TestCase(5, 0, false, "-1234", TestName = "IsValidNumber_ShouldBeTrue_WhenIntegerWithinPrecisionWithoutFraction")] | ||
| [TestCase(5, 2, false, "999.99", TestName = "IsValidNumber_ShouldBeTrue_WhenPrecisionAndScaleAtUpperBound")] | ||
| [TestCase(20, 5, false, "9223372036854775808", TestName = "IsValidNumber_ShouldBeTrue_WhenNumberExceedsLongMaxValue")] | ||
| [TestCase(50, 10, false, "12345678901234567890.1234567890", TestName = "IsValidNumber_ShouldBeTrue_WhenVeryLongNumberWithinPrecision")] |
There was a problem hiding this comment.
А если такое же число, но условиям валидации не подходит? :)

No description provided.