Conversation
pakapik
left a comment
pakapik
left a comment
There was a problem hiding this comment.
Если покажется, что замечаний много, то не переживай - во-первых, нет, во-вторых, это нормально, т.к. ревью для того и делается 🙃
There was a problem hiding this comment.
Хорошо. Ещё, как вариант в борьбе с дублированием кода, 2 предыдущих теста можно было бы объединить в один с именем NumberValidator_WhenPassInvalidArguments_ShouldThrowsArgumentException, подтягивая данные из TestCaseSource . Подобный подход хорош тем, что на один тест можно клепать много входных данных, проверяя n сценариев сразу. При этом можно манипулировать параметрами входных данных всякими SetXXX и Object Mother / Test Data Buider. Из минусов можно назвать, что тесту приходится давать чуть более общее имя (которое, впрочем, можно уточнить через SetName) и придумывать какое-то название для TestCaseSource, но это мелочи по сравнению с тем, какого объёма кода иногда можно избежать. Это просто к сведению, переделывать не нужно.
Буду оставлять ещё nit: замечания, которые цепляют глаз только у меня. Ты можешь принимать их к сведению, но не обязана что-то с этим делать - исправлять или даже отвечать.
There was a problem hiding this comment.
Ожидаемый результат ShouldReturnFalse не соответствует секции Assert.
nit: к NumberValidator в названии теста я бы добавил ctor: NumberValidatorCtor, чтоб подчеркнуть, что тестируется именно конструктор, а не класс.
There was a problem hiding this comment.
nit: в лекции видел, что у вас есть упоминание System Under Test. По некоторым гайдам/книгам/статьям, именно так и стоит называть тестируемый объект - sut. Если бы секция Arrange была больше, то из nit это превратилось в момент, который стоит поправить.
There was a problem hiding this comment.
Откровенно говоря, я не понял условия 🙃
Почему PassOnlyPositive - IsFalse ?
There was a problem hiding this comment.
nit: лично я не вижу смысла добавлять название тестируемого метода в название теста по той причине, что названия методов, бывает, меняются. А про тесты вспоминают, уже когда те начинают падать по иным причинам.
Обычно я делаю так: [Test][TestOf(nameof(NumberValidator.IsValidNumber))] public void ShouldBe[что-то]_When[что-то].
cs/HomeExercises/ObjectComparison.cs
Outdated
cs/HomeExercises/ObjectComparison.cs
Outdated
There was a problem hiding this comment.
Казалось бы мелочь, но после // принято ставить пробел - так читается проще 🙃
cs/HomeExercises/ObjectComparison.cs
Outdated
There was a problem hiding this comment.
nit: numbers множественное число, does следовало бы заменить на do - does используется только для he/she/it. Обращаю внимание только по той причине, что можно было сократить название длинного метода на пару буковок🙃
There was a problem hiding this comment.
Оставлю ссылку на этот комментарий, и ещё раз скажу, что тесты, которые выкидывают исключение, можно объединить в один, указав для них TestCaseSource
There was a problem hiding this comment.
Тут тоже нужно подумать в сторону TestCaseSource. Вызывать внутри теста другой тест - плохая практика.
Под обобщением создания корректного валидатора я подразумевал немного иное. Подумай, как можно получать в методе / группе методов корректный валидатор, не вызывая непосредственно конструктор?
Но опустим пока этот момент. Сейчас тебе надо сделать так, чтоб все твои 8 тестов, о которых мы говорили, превратились в один, для которых бы использовался один или несколько TestCaseSource.
There was a problem hiding this comment.
Очевидно, что слетело форматирование. Если пользуешься rider, то в нем можно прожать ctrl + alt + L / ctrl + alt + shift + L, код отформатируется. Можно так же настроить автоформатирование (можешь погуглить) на каждое сохранение кода ctrl + s - очень удобно, кстати, рекомендую. Если в vs работаешь, то не подскажу, но всё легко гуглится.
There was a problem hiding this comment.
Всё тело метода можно упростить до одной строчки, не забудь про expression body
There was a problem hiding this comment.
Помимо уже озвученных замечаний InvalidArgumentTestCases и ValidArgumentTestCases стоит разделить пустой строкой друг от друга, но это тоже обычно автоформатируется.
There was a problem hiding this comment.
Тоже как вариант обобщения. Иногда SetUp очень удобен, но он запускается для каждого теста - в том числе для тестов, где ты проверяешь конструктор.
Помимо прочего, не беря во внимание SetUp (т.е. вообще не юзаем SetUp), хотел бы услышать ответ на вопрос - что может пойти не так, если использовать в тест-классе поле, тем более, если это поле-sut? Представь, что у тебя не несколько тестов, а 100-200 штук, и в каждом по своему разумению используется поле numberValidator?
По тому, как я бы хотел видеть создание numberValidator, дам подсказку - тебе нужно сделать обертку над созданием numberValidator, своего рода фабрику (не в ортодоксальном её понимании), какой-то мини-член класса, который отвечал бы только за что, что возвращал бы корректный валидатор)
К слову, поля обычно начинается с нижнего подчеркивания: _numberValidator. Это позволяет не использовать конструкцию this внутри класса, да и в целом сразу видно, что это поле, а не просто локальная переменная. Это тоже где-то настраивается в настройках среды. В Rider Settings (ctrl + alt + s) → Editor → Code Style → C#. И тут куча всяких настроек, можешь попробовать настроить под себя + выставить в подсказки мои замечания, чтоб среда сама тебе всё подсказывала в будущем. Но поле numberValidator тебе тут вообще не нужно)
There was a problem hiding this comment.
Ещё один минус SetUp - это непонятно, в каком состоянии находится сейчас numberValidator. Очевидно, что он проинициализирован, но с какими параметрами? Сейчас название теста не соответствует проверяемым кейсам - ValidArgumentTestCases как источник данных, но при этом WhenPassInvalidArguments_ShouldReturnFalse.
cs/HomeExercises/ObjectComparison.cs
Outdated
There was a problem hiding this comment.
Ранее не обратил внимание, но объясни ещё, пж, про 52 строку - в чем тут потенциальная опасность?
There was a problem hiding this comment.
Тут тоже можно до одной строки упростить.
There was a problem hiding this comment.
Ранее об этом не говорил, но класс тоже можно пометить атрибутом TestFixture, причем туда же можно по желанию так же впихнуть TestOf: [TestFixture(TestOf = typeof(NumberValidator))]
There was a problem hiding this comment.
[TestFixture] дублировать не надо, хватит одной записи - первой или второй (на мой взгляд более предпочтительной)
There was a problem hiding this comment.
nit: очень странный перенос аргументов. Мб, у тебя по ширине строки это форматируется, не знаю. Обычно пишут либо в одну строку:
(int precision, int scale, bool onlyPositive, string number, bool expectedResult),
либо переносят вообще всё:
(
int precision,
int scale,
bool onlyPositive,
string number,
bool expectedResult)
There was a problem hiding this comment.
new TestCaseData(3,2,true,"0.0", true).Returns().SetName("WhenNumberIsValid")
А bool expectedResult можешь убрать. Соответственно, тебе придется возвращать результат вызова IsValidNumber.
There was a problem hiding this comment.
Все ещё неверное название теста. У тебя в источнике есть кейсы, которые проверяют, что IsValidNumber вернет true + туда подаются валидные аргументы.
There was a problem hiding this comment.
Не надо смешивать секцию Act и Assert. Отдельно напиши в var actualResult = correctValidator.IsValidNumber(number);, затем отдельно Assert.AreEqual(expectedResult, actualResult). Впрочем, когда ты переделаешь с учетом замечаний сверху, тебе вообще не понадобится писать Assert.
Но совет про не смешивать всё ещё актуальный (во всех смыслах🙃🙃🙃)
There was a problem hiding this comment.
Давай добавим чуть больше данных для проверок - null, спец.символы - /r , /n, просто символы %$# и т.д. Число с каким-то символом? Символ с каким-то числом? Строка из пробелов?
Что будет, если разделитель не точка, а запятая? А если разделителей несколько? А если в строке только разделитель?
Как валидатор ведет себя с очень большими числами? С очень большой точностью? А все вместе? Умеет ли он понимать числа в экспоненциальном формате?
А может, валидатор считает верным число в другой системе счисления? Иррациональные числа?
Возвращает ли валидатор на один и тот же входной параметр один и тот же результат? Для этого можно использовать атрибут [Repeat()].
А может, валидатор как-то меняет свой стейт? Что будет если подать сначала одно число, затем второе, а затем снова первое? Кстати, для того, чтоб обозначить, что метод ничего не изменяет (что-то внутри себя / входные параметры), то его можно пометить атрибутом [Pure] из неймспейса System.Diagnostics.Contracts или JetBrains.Annotations. В данном случае [Pure] применяться должен к IsValidNumber, раз он внутри себя не делает ничего такого криминального.
Тесты как документация кода должны отвечать на все эти вопросы и даже больше) Так что сама тоже подумай над тем, каких бы ещё тестов добавить
2 participants
@pakapik