Conversation
Yrwlcm
left a comment
Yrwlcm
left a comment
There was a problem hiding this comment.
В целом задача сделана хорошо, но есть что доработать. Старался в ревью не писать сразу как поправить, а немного на подумать :) Но вообще по любому комменту пиши - обсудим или более явно подскажу
Еще есть удобная практика в ревьюшках, на комменты которые поправил тыкать эмодзи какой-нибудь. Так проще отслеживать, как по чеклисту, предлагаю попробовать, но можно и не пробовать
| //Fluent Assertions | ||
| actualTsar.Should() | ||
| .BeEquivalentTo(expectedTsar, options => options | ||
| .Excluding((IMemberInfo memberInfo) => memberInfo.Name == "Id")); |
There was a problem hiding this comment.
Круто, что нашел правильное решение! Его можно еще чуть чуть доработать, если воспользуемся шарповым оператором, который умеет названия переменных доставать) Тогда этот тест сам будет актуализироваться ренеймингом через IDE
|
|
||
| // Какие недостатки у такого подхода? | ||
| /* | ||
| Недостатки: | ||
| 1. При падении теста нет возможности отследить причину, так как вывод теста это True либо False | ||
| 2. Нет обработки рекурсии, приложение может упасть с ошибкой переполнения стека при циклических ссылках Parent. | ||
| 3. При измении Person (удаление/добавление свойств), придется модифицировать метод | ||
|
|
||
| Мое решение лучше тем, что: | ||
| 1. В случае не прохождения теста, выводит все причины | ||
| 2. Отсутствие дублирования кода | ||
| 3. Обработка рекурсии под капотом: рекурсия выполняется до 10 уровней, далее тест падает с ошибкой, что есть | ||
| циклическая зависимость | ||
| 4. Person расширяем: нет необходимости в модификации метода | ||
| */ |
| [TestCase(1, 0, true, TestName = "OnlyPositiveIsTrue")] | ||
| [TestCase(1, 0, false, TestName = "OnlyPositiveIsFalse")] | ||
| [TestCase(int.MaxValue, 0, false, TestName = "PrecisionMaxValue")] | ||
| public void Constructor_WhenValidArguments_ReturnVoid(int precision, int scale, bool onlyPositive) | ||
| { | ||
| var func = () => new NumberValidator(precision, scale, onlyPositive); | ||
|
|
||
| func.Should().NotThrow(); | ||
| } |
There was a problem hiding this comment.
Эти тесты кажется не очень информативные. Сценарии которые они покрывают, уже тестируются например в тесте IsValidNumber_WithValidArguments_ReturnsTrue. Предлагаю их удалить
There was a problem hiding this comment.
В тесте IsValidNumber_WithValidArguments_ReturnsTrue у меня упор идет на Value, предполагается, что остальные аргументы верные. В этом же тесте я проверяю precision, scale, onlyPositive, что при верных аргументах все будет работать, как задумано. Например, если их уберем, потом спустя время как-то поменяется код и упадет тест в IsValidNumber_WithValidArguments_ReturnsTrue, но не по вине Value, а в TestName будет что-то про Value, что тогда? Достаточно будет просто дебагом пройтись и следовательно подобные тесты в Constructor_WhenValidArguments_ReturnVoid это излишне?
There was a problem hiding this comment.
В этом же тесте я проверяю precision, scale, onlyPositive, что при верных аргументах все будет работать, как задумано
Скорее в этом тесте проверяется что у нас NumberValidator не падает на этапе инициализации в правильных сценариях. А корректность самой логики мы проверяем в других тестах
Моя идея скорее в том, что если мы сломаем конструктор, то кажется и все тесты из IsValidNumber_WithValidArguments_ReturnsTrue упадут.
Я бы предложил на это посмотреть так: могут ли у нас быть сценарии, при которых эти тесты упадут, а все остальные, которые мы уже написали, будут зелеными?
- И если да - то лучше найти пример и как раз его покрыть тестом.
- А если нет, то тогда эти тесты дублируют какой-то сценарий и их можно не запускать. Вроде мелочь, но лучше когда у тебя упало условно 10 тестов, а не 13 :)
И как раз по такой же логике можно понять, почему Constructor_WithInvalidArguments_ThrowsArgumentException должны остаться. Если мы неправильно инициализируем NumberValidator он должен упасть, но во всех других тестах, мы его инициализируем правильно и это не проверяем
но не по вине Value, а в TestName будет что-то про Value, что тогда
Это правильная мысль, но в контексте этого теста она не очень подходит. Потому что если у нас валидатор падает на этапе создания объекта, то нам тесты об этом напишут например вот такой ошибкой, что исключение свалилось в конструкторе. А другую причину мы бы тут и не отловили, тест только конструктор покрывает
System.ArgumentException : ...
at HomeExercise.Tasks.NumberValidator.NumberValidator..ctor(Int32 precision, Int32 scale, Boolean onlyPositive) in ...\2. NumberValidator\NumberValidator.cs:line 18
Достаточно будет просто дебагом пройтись
Ага, нужно будет понять почему тест ведет себя не так как задумывалось и либо поправить тест, либо поправить код
| [TestCase(-1, 2, false, TestName = "PrecisionIsNegative")] | ||
| [TestCase(1, 2, false, TestName = "ScaleGreaterThanPrecision")] | ||
| [TestCase(1, 1, false, TestName = "ScaleEqualsPrecision")] | ||
| [TestCase(1, -1, false, TestName = "NegativeScale")] | ||
| public void Constructor_WithInvalidArguments_ThrowsArgumentException(int precision, int scale, bool onlyPositive) |
There was a problem hiding this comment.
Предлагаю тут еще докинуть тест на precision 0, тоже краевой сценарий
| [TestCase(1, -1, false, TestName = "NegativeScale")] | ||
| public void Constructor_WithInvalidArguments_ThrowsArgumentException(int precision, int scale, bool onlyPositive) | ||
| { | ||
| var func = () => new NumberValidator(precision, scale, onlyPositive); |
There was a problem hiding this comment.
Совсем придираюсь, но думаю нужно переменную более информативно назвать, func слишком общее понятие :)
| [TestCase(3, 2, true, "+1.23", ExpectedResult = false, TestName = "PositiveValue")] | ||
| [TestCase(4, 2, true, "-1.23", ExpectedResult = false, TestName = "NegativeValue")] | ||
| [TestCase(6, 3, true, "-1 .2 3", ExpectedResult = false, TestName = "NumberWithEmptySpace")] |
There was a problem hiding this comment.
Кажется под негативные сценарии уже есть тест, давай унесем эти тесткейсы туда. И после этого получится еще одну клевую штуку сделать, чтобы побороться с избыточностью информации в тесткейсах
There was a problem hiding this comment.
Тесты перенес, но что за клевая штука?) Тоже нужна подсказка
There was a problem hiding this comment.
Если мы разделили тесты на те которые ожидают только true и те которые ожидают только false, мы можем явно не прописывать ExpectedResult = true в каждом тесте, а написать это один раз в теле теста)
| [TestCase(30, 2, true, null, ExpectedResult = false, TestName = "NullValue")] | ||
| [TestCase(30, 2, true, "Hello world!", ExpectedResult = false, TestName = "Text")] | ||
| [TestCase(30, 2, true, "0+0", ExpectedResult = false, TestName = "InvalidSignPosition")] | ||
| [TestCase(4, 2, true, "+-0.0", ExpectedResult = false, TestName = "DuplicateSigns")] |
There was a problem hiding this comment.
Этот тесткейс может и по другой более очевидной причине падать, давай сделаем его однозначным
There was a problem hiding this comment.
Можно еще подсказку? Не понимаю, какая более очевидная причина может быть и что значит, сделать его однозначным? Т.е должна быть одна причина, из-за которой тест упадет? Или эту группу тестов можно объединить в один?
There was a problem hiding this comment.
Еще раз глянул реализацию NumberValidator-а, скорее моя формулировка не совсем правильная, сори. Моя идея была в том, что у нас в этом тесте и таком же ниже, подается на вход 5 символов, а precision 4 символа.
В теории мы могли бы такие случаи отсекать еще до сверки с регуляркой, тогда бы мы не проверяли, что именно в двойных символах проблема. Но в реализации мы в любом случае засовываем в регулярку, так что скорее я ввёл тебя в заблуждение
| [TestCase(30, 2, true, "a.sd", ExpectedResult = false, TestName = "Letters")] | ||
| [TestCase(30, 2, true, "1.sd", ExpectedResult = false, TestName = "MixedNumericAndLetters")] | ||
| [TestCase(30, 2, true, "1.\n", ExpectedResult = false, TestName = "MixedNumericAndControlCharacter")] | ||
| [TestCase(4, 2, true, "+\n.10", ExpectedResult = false, TestName = "MixedNumericAndPositiveLeadingControlCharacter")] |
There was a problem hiding this comment.
Этот тесткейс может и по другой более очевидной причине падать, давай сделаем его однозначным
| [TestCase(6, 3, true, "-1 .2 3", ExpectedResult = false, TestName = "NumberWithEmptySpace")] | ||
| [TestCase(30, 2, true, "922337203685477580711.0", ExpectedResult = true, TestName = "VeryLargeWholePartNumber")] | ||
| [TestCase(31, 30, true, "0.922337203685477580711", ExpectedResult = true, TestName = "VeryLargeFractionalPartNumber")] | ||
| public bool IsValidNumber_WithValidArguments_ReturnsTrue(int precision, int scale, bool onlyPositive, string value) |
There was a problem hiding this comment.
Не покрыли тестами одну важную функциональность, которую закладывал человек, писавший регулярку в NumberValidator-е, посмотри её внимательно
| [TestCase(30, 2, true, "+++", ExpectedResult = false, TestName = "MultipleSigns")] | ||
| [TestCase(30, 2, true, " ", ExpectedResult = false, TestName = "WhitespaceOnly")] | ||
| [TestCase(30, 2, true, ".12", ExpectedResult = false, TestName = "MissingIntegerPart")] | ||
| public bool IsValidNumber_WithInvalidArguments_ReturnsFalse(int precision, int scale, bool onlyPositive, string value) |
There was a problem hiding this comment.
И еще забыли покрыть сценарии с разным onlyPositive
2 participants
@Yrwlcm