Conversation
There was a problem hiding this comment.
Реформатируй, пожалуйста, код перед отправкой на проверку
Первый раз - не страшно, но наставник на следующих проверках имеет право не проверять твое решение, пока код не отреформачен
Здорово, что поделил на коммиты, молодец)
Со мной можно спорить по комментам ревью, не стесняйся, если есть что сказать
|
|
||
| namespace HomeExercises | ||
| { | ||
| [TestFixture] |
There was a problem hiding this comment.
Можешь объяснить какую роль выполняет этот атрибут? Почему он нужен в этой ситуации?
There was a problem hiding this comment.
Этим атрибутом помечается класс, содержащий тесты.
Насколько я знаю, ему можно передавать параметры, которые прокинуться в конструктор класса.
Таким образом можно автоматически создавать несколько классов тестов с разными настройками (действие похоже на атрибут TestCase для метода).
Использование этого атрибута не является обязательным, но я решил его добавить, чтобы подчеркнуть то, что NumberValidatorTests содержит тесты. Хотя это понятно и из названия класса :)
There was a problem hiding this comment.
я думаю, что нет смысла навешивать этот атрибут, когда нет прямой необходимости (например, создания тестового класса с разными параметрами или параллельный запуск тестов на уровне фикстуры), поэтому я бы убрала) Но в целом это не критично
| namespace HomeExercises | ||
| { | ||
| [TestFixture] | ||
| public class NumberValidatorTests |
There was a problem hiding this comment.
Для более простой навигации по проекту лучше разносить классы по отдельным файлам и называть их соответствующе + обычно тесты выносят в отдельный проект, чтобы не выпускать их вместе с основным кодом на рабочую площадку (относится к обеим частям задания)
cs/HomeExercises/ObjectComparison.cs
Outdated
| { | ||
| [Test] | ||
| [Description("Проверка текущего царя")] | ||
| [Description("Проверка текущего царя. Информативная реализация")] |
There was a problem hiding this comment.
Description не понадобится, если дать тесту достаточно понятное название
cs/HomeExercises/ObjectComparison.cs
Outdated
|
|
||
| namespace HomeExercises | ||
| { | ||
| public class ObjectComparison |
There was a problem hiding this comment.
В названии лучше отразить, что это класс с тестами
cs/HomeExercises/ObjectComparison.cs
Outdated
| [Test] | ||
| [Description("Проверка текущего царя")] | ||
| [Description("Проверка текущего царя. Информативная реализация")] | ||
| [Category("ToRefactor")] |
There was a problem hiding this comment.
Кажется, что эта категория не нужна)
| { | ||
| [Test] | ||
| public void Test() | ||
| [Category("NumberValidator.Constructor; Exception")] |
There was a problem hiding this comment.
про категорию аналогично другому комменту
| [TestCase(5, -5, TestName = "Scale < 0")] | ||
| [TestCase(5, 5, TestName = "Precision = Scale")] | ||
| [TestCase(5, 10, TestName = "Precision < Scale")] | ||
| public void Constructor_With_IncorrectArguments_Should_ThrowException(int precision, int scale) |
There was a problem hiding this comment.
Я бы все-таки советовала именование тестов в формате [Метод (или конструктор)]should[что-то]when[условие]
| Assert.IsFalse(new NumberValidator(17, 2, true).IsValidNumber("0.000")); | ||
| Assert.IsFalse(new NumberValidator(3, 2, true).IsValidNumber("-1.23")); | ||
| Assert.IsFalse(new NumberValidator(3, 2, true).IsValidNumber("a.sd")); | ||
| var action = new Action(() => new NumberValidator(precision, scale)); |
There was a problem hiding this comment.
это на самом деле Func а не Action
имхо (можно не исправлять) я бы написала Func<NumberValidator> action = () => new NumberValidator(precision, scale);
| } | ||
|
|
||
| [Category("NumberValidator.Constructor; No exception")] | ||
| [TestCase(10, 5, false, TestName = "Correct parameters")] |
There was a problem hiding this comment.
А какие параметры будут корректными? Мне понравился формат, в котором ты описал некорректные в названиях, тут можно сделать так же)
| [TestCase(10, 5, true, "++3.45", TestName = "Two signs")] | ||
| [TestCase(10, 5, true, "2,,66", TestName = "Two separators")] | ||
| [TestCase(10, 5, true, "", TestName = "Empty string as number")] | ||
| public void IsValid_ShouldReturn_False_When_IncorrectValue(int precision, int scale, |
There was a problem hiding this comment.
Какие еще граничные ситуации проверяют при таком тестировании обычно? (есть еще как минимум 1 очень простой распространенный тесткейс)
…отдельным папкам. Потребовалась реорганизация файлов задания. Удалил тестовый проект, который создал для проверки новой структуры. Это инфраструктурный этап рефакторинга.
2 participants
@elis_shtol