Бабинцев Григорий #21
Conversation
| actualTsar.Should().BeEquivalentTo(expectedTsar, | ||
| options => options.Excluding(person => person.Id). | ||
| Excluding(person => person.Parent.Id)); |
There was a problem hiding this comment.
Давай попробуем выполнить сравнение для такого царя
new Person("Ivan IV The Terrible", 54, 170, 70,
new Person("Vasili III of Russia", 28, 170, 60,
new Person("Ivan III", 65, 170, 80, null)))
Важно обращать внимание на рекурсивные вызовы, потому что в них часто кроются ошибки
There was a problem hiding this comment.
Сравнение с ID будет работать для всех, да. Но если в Person появятся какие-нибудь другие ID в будущем, которые будет нужно сравнивать, есть риск их пропустить. Попробуем сделать обработку конкретно этого свойства Id в person и его parent?
|
|
||
| [Test] | ||
| [Test] |
There was a problem hiding this comment.
Совет на будущее: хорошая привычка пересматривать изменения перед коммитом или пушем, чтобы ненужные изменения не попадали в общий репозиторий. Здесь сыграли злую шутку табы и пробелы, решается просто настройкой IDE
|
|
||
| namespace HomeExercise.Tasks.NumberValidator | ||
| { | ||
| // для каждого теста может быть много тест кейсов, поэтому я вынес их в отдельные коллекции |
There was a problem hiding this comment.
Хорошо что знаешь про возможность выделения тесткейсов в отдельные коллекции. Но здесь это не идёт на пользу
- На самом деле в тестах не должно быть много кейсов. Если значительно растёт их количество, значит что-то не так с тестом и нужно проверить, а сколько действий он проверяет
- Потерялась связь с тем, что вообще означают эти цифры. Если хочется выносить в отдельные коллекции, то нужно делать их generic. Например так. Тогда ещё на этапе компиляции IDE подскажет, если что-то не будет совпадать
public static IEnumerable<(int precision, int scale, bool onlyPositive, string value)> ValidTestCases
|
|
||
| yield return new TestCaseData(17, 2, true, "0"); | ||
|
|
||
| yield return new TestCaseData(4, 2, true, "+1.23"); |
There was a problem hiding this comment.
В этом кейсе одновременно проверяются три сценария
- максимум по цифрам включая знак
- максимум по цифрам
- максимум по дробной части
Я бы предложил разбить его на три независимых
| yield return new TestCaseData(4, 2, true, "+1.23"); | ||
|
|
||
| yield return new TestCaseData(7, 3, false, "+1.00"); | ||
| } |
There was a problem hiding this comment.
Не нашёл тестов на запятую в качестве разделителя и отрицательное значение
| yield return new TestCaseData(1, 0, true); | ||
|
|
||
| yield return new TestCaseData(2, 1, true); |
There was a problem hiding this comment.
Играет ли роль третий параметр в этих тестах?
| ClassicAssert.IsFalse(new NumberValidator(3, 2, true).IsValidNumber("a.sd")); | ||
| } | ||
| [TestCaseSource(nameof(ValidTestCases))] | ||
| public void Should_Be_Valid(int precision, int scale, bool onlyPositive, string value) |
There was a problem hiding this comment.
Замечание по названию. Хорошо что указал глагол и не дублируешь класс. Но потерялся указатель на то, что проверяется. Валидность - понятие довольно растяжимое. Здесь тестируется конкретный метод. Я бы предлодил назвать IsValidNumber_Shoud_BeTrue_When... Дело в том, что подряд идущие Shoud_BeTrue и Shoud_BeFalse сбивают с толку
| } | ||
|
|
||
| [TestCaseSource(nameof(NotValidTestCases))] | ||
| public void Should_Be_NotValid(int precision, int scale, bool onlyPositive, string value) |
There was a problem hiding this comment.
Тоже комментарий по названию метода. Что проверяется и чем должно быть
| } | ||
| } | ||
|
|
||
| public static IEnumerable NotValidTestCases |
There was a problem hiding this comment.
Идейно можно разбить проверки на крупные группы. Например переполнение, странные состояние строки, плохие разделители или формат, несовпадение знаков. Тогда будет проще читать тесты, а количество кейсов для одного теста не увеличится так сильно
| } | ||
|
|
||
| [TestCaseSource(nameof(ThrowArgumentExceptionTestCases))] | ||
| public void Should_ThrowArgumentException(int precision, int scale, bool onlyPositive) |
There was a problem hiding this comment.
А всё ещё IsValidNumber должен вызывать исключение или мы тестируем что-то другое?
There was a problem hiding this comment.
Тут же вообще не используется IsValidNumber
There was a problem hiding this comment.
Да. Но кто-то же бросает исключения. Я бы назвал его с использованием слова Constructor. Например Constructor_Should_ThrowArgumentException
|
Переделал на тест кейсы, потому что при выделении в отдельный класс коллекций с кейсами тесты что-то не запускались и добавил названия тестам |
| actualTsar.Should().BeEquivalentTo(expectedTsar, | ||
| options => options.Excluding(person => person.Id). | ||
| Excluding(person => person.Parent.Id)); |
There was a problem hiding this comment.
Сравнение с ID будет работать для всех, да. Но если в Person появятся какие-нибудь другие ID в будущем, которые будет нужно сравнивать, есть риск их пропустить. Попробуем сделать обработку конкретно этого свойства Id в person и его parent?
| } | ||
| } | ||
|
|
||
| public static IEnumerable NotValidTestCases |
| } | ||
|
|
||
| [TestCaseSource(nameof(ThrowArgumentExceptionTestCases))] | ||
| public void Should_ThrowArgumentException(int precision, int scale, bool onlyPositive) |
There was a problem hiding this comment.
Да. Но кто-то же бросает исключения. Я бы назвал его с использованием слова Constructor. Например Constructor_Should_ThrowArgumentException
| public void Should_Be_NotValid(int precision, int scale, bool onlyPositive, string value) | ||
|
|
||
| [TestCase(3, 2, true, "00.00", TestName = "Number length with fractionl part more than precision")] | ||
| [TestCase(3, 2, true, "-0.0", TestName = "Negative number length with fractionl part more than precision")] |
There was a problem hiding this comment.
А этот тест проверяет не то, что указано в описании. Но сам тест хороший
No description provided.