-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Галичев Артем #13
base: master
Are you sure you want to change the base?
Галичев Артем #13
Conversation
…овые случаи на разные тестовые методы. Создал класс TestNumberValidatorBuilder для удобного создания класса NumberValidator
@@ -1,4 +1,5 @@ | |||
using NUnit.Framework; | |||
using FluentAssertions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Привет) давай сначала общие рекомендации:
-
Именование коммитов) Имена должны быть емкими (и желательно на английском языке, как стандарте в ИТ индустрии) у меня в github desktop'e они даже не влезают в строчку имени и уезжают в описание
-
Содержание коммитов должно быть атомарным. Один коммит - одно логическое изменение. Например, в коммите
NumberValidatorTests. Написал недостающие тесты, разделил разные тестовые случаи на разные тестовые методы. Создал класс TestNumberValidatorBuilder для удобного создания класса NumberValidator
ты уже сам видишь, что завел много различных независимых изменений, которые просто завернул в один коммит и отправил. Рекомендую стараться разделять независимое. Зачем это нужно? Чтобы по истории коммитов, понимать, что произошло. Или уметь ревертить только "плохие" части. Или уметь их черри-пикать. И тд
Прямо сейчас что-либо делать с коммитами не надо. Это рекомендации на будущее
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Также могу порекомендовать отмечать мои сообщения какой-нибудь эмодзи, чтобы отмечать проработанное и не запутаться перед отправкой на следующую итерацию ревью)
@@ -35,6 +27,14 @@ public void CheckCurrentTsar_WithCustomEquality() | |||
new Person("Vasili III of Russia", 28, 170, 60, null)); | |||
|
|||
// Какие недостатки у такого подхода? | |||
/* | |||
* 1) Если у класса изменять поля (удалять или добавлять), то также придется изменять |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
ClassicAssert.AreEqual(expectedTsar.Parent.Height, actualTsar.Parent.Height); | ||
ClassicAssert.AreEqual(expectedTsar.Parent.Parent, actualTsar.Parent.Parent); | ||
|
||
actualTsar.IsEqual(expectedTsar); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Можешь рассказать, почему вынес в экстеншн эту проверку?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Я подумал что без экстеншн в проверке будет избыточная информация из за настройки проверки. Избыточная информация замедляла бы скорость понимания всего теста. Можно было вынести и в приватный метод тестового класса, но я посчитал что через экстеншн проверка читается проще, как обычное английское предложение.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Понял, корректно)
Могу только немного со стороны накинуть, что пока использование единственное и сама проверка не супер сложная (всего шесть строк, когнитивная нагрузка низкая).ю то можно оставлять прямо в тесте. А вот при дублировании или усложнении проверки - да, вынести в метод или экстешн хорошее повышение читаемости)
{ | ||
var builder = new TestNumberValidatorBuilder().WithPrecision(-1); | ||
|
||
Assert.Throws<ArgumentException>(() => builder.Build()); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
actual.Should().BeFalse(); | ||
} | ||
|
||
[Test] |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
@@ -0,0 +1,34 @@ | |||
namespace HomeExercise.Tasks.NumberValidator; | |||
|
|||
public class TestNumberValidatorBuilder |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
||
public NumberValidator Build() | ||
{ | ||
return new NumberValidator(precision, scale, onlyPositive); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
[TestCase(5, -1, TestName = "Should_Throw_ArgumentException_If_Scale_Is_Negative")] | ||
[TestCase(2, 5, TestName = "Should_Throw_ArgumentException_If_Scale_Greater_Precision")] | ||
[TestCase(2, 2, TestName = "Should_Throw_ArgumentException_If_Scale_Equal_Precision")] | ||
public void Should_Throw_Exception(int precision, int scale) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Assert.DoesNotThrow(() => new NumberValidator(1, 0, true)); | ||
var act = () => new NumberValidator(10, 5); | ||
|
||
act.Should().NotThrow<ArgumentException>(); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
||
var actual = numberValidator.IsValidNumber(value); | ||
|
||
actual.Should().BeFalse(); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
[TestCase(2, 2, TestName = "Should_Throw_ArgumentException_If_Scale_Equal_Precision")] | ||
public void Should_Throw_Exception(int precision, int scale) | ||
{ | ||
var act = () => new NumberValidator(precision, scale); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
@dmitgaranin