Conversation
| using NUnit.Framework; | ||
| using FluentAssertions; | ||
| using NUnit.Framework; | ||
| using NUnit.Framework.Legacy; |
There was a problem hiding this comment.
using NUnit.Framework.Legacy; не используется
There was a problem hiding this comment.
если убрать - альтернативное решение не сможет использовать ClassicAssert
There was a problem hiding this comment.
Не в том классе пометил - это к NumberValidatorTests.cs относилось, сорри.
| ClassicAssert.IsFalse(new NumberValidator(17, 2, true).IsValidNumber("0.000")); | ||
| ClassicAssert.IsFalse(new NumberValidator(3, 2, true).IsValidNumber("-1.23")); | ||
| ClassicAssert.IsFalse(new NumberValidator(3, 2, true).IsValidNumber("a.sd")); | ||
|
|
There was a problem hiding this comment.
Пустая строка после каждого [Test], давай уберём, а перед атрибутом, наоборот, добавим.
|
|
||
| public void NumberValidator_ThrowExc_AfterAdditionNotPositivePrecision() | ||
| { | ||
| var action1 = () => new NumberValidator(-1, 2, true); |
There was a problem hiding this comment.
Давай вынесем action1 и action2 в атрибуты TestCase
| var action2 = () => new NumberValidator(0, 2, true); | ||
|
|
||
| action1.Should().Throw<ArgumentException>() | ||
| .WithMessage("precision must be a positive number"); |
There was a problem hiding this comment.
Давай вынесем "precision must be a positive number" в константу. Иначе, если захотим исправить сообщение, придётся не забыть исправить в двух местах.
| var action = () => new NumberValidator(1, 2, true); | ||
|
|
||
| action.Should().Throw<ArgumentException>() | ||
| .WithMessage("precision must be a non-negative number less or equal than precision"); |
There was a problem hiding this comment.
То же самое - "precision must be a non-negative number less or equal than precision" в константу.
Вопрос на засыпку: тебя в смысле этого сообщения ничего не смущает? :)
There was a problem hiding this comment.
scale вместо первого precision. Я думал может поменять, но не стал, ибо по заданию тот файл не трогаем
There was a problem hiding this comment.
Ага. Здесь мораль в том, что если есть что-то за пределами твоей задачи, что недорого поправить (а в этом случае недорого), то лучше поправить сразу. В противном случае получается, что мы добавили проверку конкретно этой строки в тесты и если её потом исправят в исходном классе, то неизвестно, сколько еще дополнительных правок придётся делать в тестах.
| } | ||
| [Test] | ||
|
|
||
| public void NumberValidator_ThrowExc_AfterAdditionPositivePrecisionEqualScale() |
There was a problem hiding this comment.
Тест полностью дублирует предыдущий. Если бы у тебя был один тест с разными тест-кейсами, то ты бы сразу увидел, что продублировал тест-кейс, давай объединим в один.
Кроме того, я вот читаю часть названия "...AfterAddition..." и чет не совсем понимаю, про что это?
А еще у тебя класс с тестами и так называется NumberValidatorTests и в целом встречается, когда в именовании теста пишут название тестируемого класса\модуля, но, кмк, будет лаконичнее написать просто Should_ThrowException_.... Да, слово exception я бы полностью написал.
| public void NumberValidator_NotThrow_WhenValidParameters() | ||
| { | ||
|
|
||
| var action1 = () => new NumberValidator(1, 0, true); |
There was a problem hiding this comment.
Выноси разные параметры для action в тест-кейсы
|
|
||
| public void IsValidNumber_IsValid_AfterAdditionValidNumber() | ||
| { | ||
| var testNum1 = new NumberValidator(4, 2, true).IsValidNumber("12.34"); |
There was a problem hiding this comment.
Ты в тестах несколько нарушаешь структуру AAA - по-дефолту обычно Arrange отделяют пустой строкой от Act, а Act - пустой строкой от Assert. В целом это не критично и можно писать и так (и люди пишут), НО! Когда тесты становятся посложнее проще их читать, когда у них эта структура соблюдена.
|
|
||
| public void IsValidNumber_IsNotValid_AfterAdditionNegativeNumberWhenOnlyPositiveTrue() | ||
| { | ||
| var testNum1 = new NumberValidator(4, 2, true).IsValidNumber("-2.34"); |
There was a problem hiding this comment.
Пиши тесты единообразно - если ты в предыдущих тестах отделял Assert пустой строкой, то и здесь делай так же.
|
|
||
| [Test] | ||
|
|
||
| public void IsValidNumber_IsValid_AfterAdditionNegativeNumberWhenOnlyPositiveFalse() |
There was a problem hiding this comment.
IsValidNumber_IsValid... - кмк, понятнее будет IsValidNumber_ReturnFalse....
|
|
||
| actualTsar.Should().BeEquivalentTo(expectedTsar, options => options | ||
| .Excluding(t => t.Id) | ||
| .Excluding(t => t.Parent.Id)); |
There was a problem hiding this comment.
Дополнительный вопрос: а что будет, если начнут добавлять деда, прадеда и т.п?
There was a problem hiding this comment.
Тест упадёт, так как у деда начнёт сравниваться Id. Вместо этого отключу сравнение id у всех объектов класса Person
There was a problem hiding this comment.
Вместо этого отключу сравнение id у всех объектов класса Person
А покажи, как?
There was a problem hiding this comment.
actualTsar.Should().BeEquivalentTo(expectedTsar, options => options
.Excluding(t => t.Id)
.Excluding(t => t.Name == "Id" && t.DeclaringType == typeof(Person)));
There was a problem hiding this comment.
В принципе уже норм. Но можно докрутить решение, чтобы было достаточно только второго Excluding.
There was a problem hiding this comment.
можно просто удалить первый, он же и так Person, значит не будет учитываться, тупанул
There was a problem hiding this comment.
Нельзя, т.к. если поле "Id" переименуют, то оно снова начнёт сравниваться. Пока у тебя есть первая строчка, ты сразу глядя на код поймёшь, в чём проблема (т.к. он тупо не скомпилируется, если в тесте не поменять название поля вместе с объявлением внутри класса, ну IDE обычно сейчас сразу во всех местах меняют).
| | ||
| using FluentAssertions; | ||
| using NUnit.Framework; | ||
| using NUnit.Framework.Legacy; |
2 participants
@Dimques