Conversation
| @@ -8,24 +9,57 @@ namespace HomeExercise.Tasks.NumberValidator; | |||
| public class NumberValidatorTests | |||
| { | |||
| [Test] | |||
| { | ||
| [Test] | ||
| public void Test() | ||
| [TestCase(-1, 2, true)] |
There was a problem hiding this comment.
В идеале хотелось бы ещё пояснения в виде текста увидеть. TestName = ...
| [TestCase(-1, 2, false)] | ||
| [TestCase(1, -1, false)] | ||
| [TestCase(1, 1, false)] | ||
| public void ThrowArgumentException_AfterCallingConstructor(int precision, int scale, bool onlyPositive) |
There was a problem hiding this comment.
Кажется что добавлять onlyPositive в аргументы TestCase скорее лишнее, тк он никак не влияет на проверяемое состояние
There was a problem hiding this comment.
Да, вы правы. Исправил. Я исходил из той мысли, что тесты не знают, как реализован метод, соответственно, человек, который написал метод, мог его написать так, что onlyPositive влиял бы на тест. (При одних и тех же данных, кроме onlyPositive тест показывал бы разный результат)
There was a problem hiding this comment.
Поверять все аргументы есть смысл когда тестируешь логику как "чёрный ящик", но это скорее про Интеграционные тесты. Unit-тесты это скорее "белый ящик", в котором тебе нужно проверить тонкости работы конкретных строчек кода, а не логики в целом. В таком кейсе ты знаешь как код устроен внутри и с какими аргументами взаимодействует. Добавление "лишних" аргументов только спутает читателя кода. Unit-тесты не обязаны покрывать "будущие" сценарии, когда кто-то решит в контейнере проверять ещё один аргумент - они проверяют что-то очень конкретное и нынешнее. Когда добавят ещё проверок в контейнер, тогда и стоит расширять тест
| [TestCase(3, 2, false, "-0.0")] | ||
| [TestCase(4, 2, true, "+1.23")] | ||
| [TestCase(4, 2, true, "+12")] | ||
| public void IsTrue_AfterCallingIsValidNumber(int precision, int scale, bool onlyPositive, string number) |
There was a problem hiding this comment.
Вкусовщина, но я бы предпочёл читать тесты вида [ТестируемыйМетод]_[Результат]_[Сценарий]/[ТестируемыйМетод]_[Сценарий]_[Результат]
Если у класса будет несколько методов с одинаковым типом результата, то будет тяжело читать как по мне. Обычно хочется прочитать начало и уже понять о чём речь
There was a problem hiding this comment.
Например тут IsValidNumber_ReturnsTrue_WhenCorrectConditions/etc
| } | ||
|
|
||
| [Test] | ||
| [TestCase(3, 2, true, "0000")] |
There was a problem hiding this comment.
Не хватает проверок из начала метода IsValidNumber
There was a problem hiding this comment.
Ещё немного вариаций с числами можно добавить
| } | ||
|
|
||
| [Test] | ||
| [TestCase(17, 2, true, "0.0")] |
| // Какие недостатки у такого подхода? | ||
| ClassicAssert.True(AreEqual(actualTsar, expectedTsar)); | ||
|
|
||
| // Ответ от студента: Если тест упадёт, не будет конкретно понятно, в чём проблема, ведь мы просто получим false. |
There was a problem hiding this comment.
Если мы к примеру захотим убрать или добавить поле в Person, то нужно будет переписать тест, в моём же решении ничего переписывать не нужно. Также для теста написан отдельный метод сравнения Person, если же появится новый класс со своими полями в классе Person, то придётся писать ещё один метод сравнения, а также переписывать сравнение Person.
| ClassicAssert.AreEqual(expectedTsar.Parent.Height, actualTsar.Parent.Height); | ||
| ClassicAssert.AreEqual(expectedTsar.Parent.Parent, actualTsar.Parent.Parent); | ||
|
|
||
| actualTsar.Should().BeEquivalentTo(expectedTsar, options => options.ExcludingMembersNamed("Id").AllowingInfiniteRecursion()); |
There was a problem hiding this comment.
На подумать:
А можно сделать так чтобы при изменении названия поля(с помощью IDE), например Id => identifier, здесь не пришлось ничего менять?
There was a problem hiding this comment.
Да, поменять "Id" на nameof(Person.Id).
|
|
||
| actualTsar.Should().BeEquivalentTo(expectedTsar, options => options.ExcludingMembersNamed("Id").AllowingInfiniteRecursion()); | ||
|
|
||
| // Комментарий от студента: |
| [TestCase(1, -1, false)] | ||
| [TestCase(1, 1, false)] | ||
| public void ThrowArgumentException_AfterCallingConstructor(int precision, int scale, bool onlyPositive) | ||
| [TestCase(-1, 2, TestName = "Precision should be <= 0")] |
There was a problem hiding this comment.
TestName немного противоречит логике. Слово should как будто бы отсылает к реальной логике, а не к конкретным входным данным твоего кейса. Ты проверяешь негативные кейсы - то есть в реальности нужно наоборот этого избегать. В таких случаях лучше описать не каким должен быть(should) а каким не должен быть(can't/should not)
There was a problem hiding this comment.
То есть по твоим тестам хочется ещё понимать логику кода, который ты тестируешь. Сейчас условия приходится инвертировать, что усложняет чтение.
Если хочется описать именно с точки зрения входных данных, то можно просто Precision <= 0, но это кажется малоинформативным
| [TestCase(20, 2, true, "++123")] | ||
| public void IsFalse_AfterCallingIsValidNumber(int precision, int scale, bool onlyPositive, string number) | ||
| [TestCase(3, 2, true, null, TestName = "Null")] | ||
| [TestCase(3, 2, true, "", TestName = "Empty string")] |
There was a problem hiding this comment.
Обычно WhiteSpace тоже хорошо бы проверять
| [TestCase(3, 2, true, "", TestName = "Empty string")] | ||
| [TestCase(3, 2, true, "0.", TestName = "Fractional part missing")] | ||
| [TestCase(3, 2, true, ".0", TestName = "Integer part is missing")] | ||
| [TestCase(3, 2, true, "0000", TestName = "Integer")] |
There was a problem hiding this comment.
Здесь не хватает объяснения, что int-овая часть превышает размер
| [TestCase(3, 2, true, "+000", TestName = "Positive signed integer")] | ||
| [TestCase(3, 2, true, "-00", TestName = "Negative integer")] | ||
| [TestCase(3, 2, true, "00.00", TestName = "Fractional number")] | ||
| [TestCase(3, 2, true, "+0.00", TestName = "Signed fractional number")] |
There was a problem hiding this comment.
Тут тоже не хватает объяснения, что знак тоже имеет вес
| [TestCase(3, 2, true, "00.00", TestName = "Fractional number")] | ||
| [TestCase(3, 2, true, "+0.00", TestName = "Signed fractional number")] | ||
| [TestCase(3, 2, true, "-0.0", TestName = "Negative fractional number")] | ||
| [TestCase(17, 2, true, "0.000", TestName = "Very long fractional part")] |
There was a problem hiding this comment.
Она не просто very long, она именно больше конкретной переменной
| [TestCase(3, 2, true, "-00", TestName = "Negative integer")] | ||
| [TestCase(3, 2, true, "00.00", TestName = "Fractional number")] | ||
| [TestCase(3, 2, true, "+0.00", TestName = "Signed fractional number")] | ||
| [TestCase(3, 2, true, "-0.0", TestName = "Negative fractional number")] |
There was a problem hiding this comment.
Тут, как и выше Negative integer всё таки важно, что onlyPositive = true. А твоё название как будто смещает акцент с этой переменной к тому, что число просто отрицательное. Хотелось бы в названии видеть все факторы, которые влияют на результат
2 participants
@FnaTikJK