Skip to content

Якшибаев Данил#47

Open
gogy4 wants to merge 3 commits intokontur-courses:masterfrom
gogy4:master
Open

Якшибаев Данил#47
gogy4 wants to merge 3 commits intokontur-courses:masterfrom
gogy4:master

Conversation

@gogy4
Copy link

@gogy4 gogy4 commented Oct 28, 2025

@isaev_io

…Assertion.

Отрефакторил NumberValidatorTests.cs с использованием FluentAssertion и TestCase
[TestCase(1, -1)]
[TestCase(3, 3)]
[TestCase(3, 4)]
public void Constructor_ShouldThrow_WhenArgsInvalid(int precision, int scale)

This comment was marked as resolved.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тут не увидел исправления + добавились еще "InvalidArgs_NegativeOrScaleTooBig"


[TestCase(1, 0, true)]
[TestCase(10, 5, false)]
public void Constructor_ShouldNotThrow_WhenArgsValid(int precision, int scale, bool onlyPositive)

This comment was marked as resolved.


#region Валидация чисел

public static IEnumerable ValidNumbers => new[]

This comment was marked as resolved.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Еще, у таких методов лучше явно указывать возвращаемый тип, а не обычный IEnumerable. Это, в каких-то случаях, поможет тебе на этапе разработки понять, что ты допустил ошибку и вернул не то, что надо (хоть и редкость, но когда прогон проходит дольше 5-10 минут, это становится ощутимо)
Помимо этого посмотри в сторону ленивости. Генерилки не должны быть жадными, а скорее ленивыми и простыми, чтобы можно было дешево использовать циклы и пр.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Остался вопрос с использованием
Где-то еще нужны эти методы, кроме данного класса?

.IsValidNumber(input)
.Should()
.BeTrue(
$"Тест '{TestContext.CurrentContext.Test.Name}' провален: " +

This comment was marked as resolved.

$"'{input}' должно быть валидным для precision={precision}, scale={scale}");
}

public static IEnumerable InvalidNumbers => new[]

This comment was marked as resolved.

new TestCaseData(3, 2, true, null, "значение null недопустимо").SetName("Invalid_Null")
};

[Test, TestCaseSource(nameof(InvalidNumbers))]

This comment was marked as resolved.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Немного не понял вопроса

Мы же помечаем Test когда пишем тесты, чтобы NUnit запускал эти методы

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test нужен для простых кейсов, когда данные ты генерируешь внутри самого теста, при этом чаще всего это будут юниты
Когда ты помечаешь тесты этим атрибутом, семантически ожидается, что метод будет вызываться один раз для одних параметров

А TestCaseSource или TestCase уже более сложные атрибуты, которыми помечаются переиспользуемые тесты, в которые мы передаем данные извне
Они хорошо подходят для интеграционных или функциональных тестов

В данном случае лучше подходит TestCaseSource, т.к. данные ты передаешь извне и использование Test + TestCaseSource излишне, достаточно использование последнего

.Should()
.BeFalse(
$"Тест '{TestContext.CurrentContext.Test.Name}' провален: " +
$"'{input ?? "null"}'. {expectedMessage}. " + $"Параметры: precision={precision}, scale={scale}");

This comment was marked as resolved.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Исправлю

public void Test()
#region Конструктор

[TestCase(-1, 2)]

This comment was marked as resolved.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Потому что TestCaseSource берет из коллекции, и там достаточно много тесткейсов, а у конструктора их пару штук. Думаю если бы я создавал отдельные коллекции для 3 элементов, то засорился бы код

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Потому что TestCaseSource берет из коллекции, и там достаточно много тесткейсов, а у конструктора их пару штук. Думаю если бы я создавал отдельные коллекции для 3 элементов, то засорился бы код

Понял, что у меня также создается коллекция для четырех тесткейсов.

@@ -1,31 +1,46 @@
using NUnit.Framework;

This comment was marked as resolved.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Вопрос хороший)

По хорошему нужно было разбить на 2 коммита, но подумал, что нужно одним коммитов, т.к. одна домашка

А так понял, в будущем буду разделять по коммитам

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Можешь описать, вкратце, почему лучше делить решение по коммитам, пожалуйста?
В чем преимущества такого подхода, недостатки?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Каждый коммит отражает логически завершённый шаг, проще понять как развивалось решение, что и как менялось.

Легче делать код ревью по каждому коммиту.

Иногда сложно заранее разбить на разные коммиты

Иногда бесмыссленно для мелких задач.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Да, в целом верно
Еще разбивка по коммитам дает тебе возможность удобно откатывать изменения, если они были ошибочны
Помимо этого легче подтягивать изменения в другие ветки через чери-пики и пр.
В общем, преимуществ много, но они не всегда нужны, ты прав
Советую попроходить игру . Там показывается и рассказывается все, что нужно для работы с гитом, думаю будет полезно, если раньше не проходил ее

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants