Pull to refresh

Comments 41

И как приятно видеть такой пост от додопиццы после тех сказочных статей о найме к вам. Это очень здорово!
Вообще говоря, паттерн builder и рождаемый им DSL штука спорная. Я вообще считаю builder антипаттерном потому, что он заставляет помнить ещё и птичий язык DSL, который уникален для данного билдера. Взять хоть пример из статьи — это же очевидно что. Please() надо вызывать в конце цепочки With, а не перед. И другого билдера с Please нет и не будет.
Я думаю, тут нет однозначного ответа использовать или нет. Я считаю, что использование билдеров несёт больше плюсов чем минусов. Более того, с минусами сам ни разу не сталкивался. Возможно у вас другой опыт и это интересно.
Расскажите плз, как вам мешала необходимость изучения структуры билдеров?

Смущает нарушение коммутативности в DSL в неочевидных местах:


var order = Create.Order
.Dated(3.May(2019))
.With(Pizza.Pepperoni.CountOf(1).For(500))
.With(Pizza.Margarita.CountOf(1).For(650))
.Please();


Вернет не то же самое, что


var order = Create.Order
.With(Pizza.Pepperoni.CountOf(1).For(500))
.With(Pizza.Margarita.CountOf(1).For(650))
.Dated(3.May(2019))
.Please();


Или такое поведение было задумано изначально?

Справедливое замечание. Спасибо, поправил.
Новые тесты становится писать легко. Не нужно 2 часа настраивать себя на юнит-тестирование, просто берешь и пишешь.

Можно просто брать и писать, как будто билдеры уже есть, а потом пусть студия создаст недостающие методы.

Да, так и надо. Всё, кроме extension'ов студия и райдер смогут создать.

Я считаю, что самых лучших результатов можно добиться, если использовать AutoFixture вместе с кастомным DSL, а не вместо него, но это за скоупом статьи.

Не совсем понимаю в чем соль доморощенного DSL, зачем время тратить на классы строители? Я бы вас понял если бы вы писали в эпоху Java6 на ней самой. Классы строители как шаблон изжили себя в среде .NET.

Я думаю, это скорее зависит от языка, чем от фреймворка.
В случае С# property и collection initializers устраняют некоторую потребность в строителях, но все равно есть потребность в бизнес-специфичных алгоритмах типа "создай заказ готовый для исполнения" когда можно убрать под капот какие-то детали создания, не важные для конкретного теста.


Например, тут я бы убрал дату внутрь билдера:


var order = Create.Order
     .With(Pizza.Pepperoni.CountOf(1).For(500))
     .With(Pizza.Margarita.CountOf(1).For(650))
     .Please();

  var service = Create.PizzeriaService.Please();

  service.AcceptOrder(order);

  service.VerifyReserveIngredientsWasCalledWith(order);

Но, внезапно выясняется, что property и collection initializers работают только при создании объекта.


Т.е. написать new Order{ Date = Today } можно, а CreateOrder(){Date = Today} нельзя.


Для записей есть proposal


CreateOrder() with {Date = Today}
Да, рекорды это круто. Скорее бы хотя бы в работу взяли)

Вы привели достаточно простой пример строителя, но в настоящей жизни нам пришлось бы иметь дело со строителем на 3 или 4 десятка строк больше. Иметь такое чудо в коде теста, на мой взгляд, не приемлемо. Тест это не только тест, но и живая документация. Так что смысла в доморощенных строителях не вижу, тем более что с AutoFixture все можно персонализировать и если это делать то в отдельном вспомогательном методе.


  // Arrange
  var order = CreateDualOrder();  
  var service = CreatePizzeriaService();
  // Act
  service.AcceptOrder(order);
  // Assert
  service.VerifyReserveIngredientsWasCalledWith(order);

В книжке Art of unit testing, предлагается скрывать детали не относящиеся к конкретному тесту и показывать связь между исходными данными и результатами. В получившемся результате скрыто почти все (из кода теста непонятно, что и сколько должно быть зарезервировано). Фактически и CreateDualOrder и VerifyReserveIngredientsWasCalledWith внутри используют какие-то константы но как они друг с другом связаны, неясно.


Я бы написал так:


var ingridient  = CreateIngridient();
var product = CreateProduct().WithIngridient(ingridient,  AmountOfIngridientInProduct).Please();
var order = CreateOrder().WithLine(product, AmountOfProductInOrderLine).Please();

var service = CreateService();
service.AcceptOrder(order);

Assert.Equals(AmountOfProductInOrderLine * AmountOfIngridientInProduct, service.GetReservedAmount(ingridient));

AutoFixture не пользовался, интересно, как там с воспроизводимостью падений. Например, в данном случае вполне возможно, что результат будет работать с точностью до округлений (каковые будет проверять другой тест).

По мне так все равно куча ненужного шума:


CreateProduct().WithIngridient(ingridient,  AmountOfIngridientInProduct).Please();

Лучше


CreateProduct(ingridient: "olive",  amount: 10);

В книжке Art of unit testing ...

Кстати в той же книге предлагается награждать описанием проверки типа:


Assert.Equals(AmountOfProductInOrderLine * AmountOfIngridientInProduct, service.GetReservedAmount(ingridient), "Ибо так потребно");

Не совсем уверен что service.GetReservedAmount(ingridient) и AmountOfProductInOrderLine * AmountOfIngridientInProduct уместны в секции Assert, по книге аrt of unit testing.

Лучше

Возможно — но тогда придется все сочетания поддерживать в одном методе.


Кстати в той же книге предлагается награждать описанием проверки типа

С моей точки зрения, когда в тесте ровно один ассерт и название достаточно говорящее, это не надо. Кстати.


Не совсем уверен что service.GetReservedAmount(ingridient) и AmountOfProductInOrderLine * AmountOfIngridientInProduct уместны в секции Assert, по книге аrt of unit testing.

Почему?

Возможно — но тогда придется все сочетания поддерживать в одном методе.

Ну это уж вам решать как структурировать код, можно и не в одном методе.


С моей точки зрения, когда в тесте ровно один ассерт и название достаточно говорящее, это не надо.

Здесь страдает не читаемость кода теста, а ошибки в ходе прогона тестов.


Почему?

Ну хотя бы потому что:
Секция 8.3.4 Separating asserts from actions
For the sake of readability, avoid writing the assert line and the method call in the same statement.

Ну это уж вам решать как структурировать код, можно и не в одном методе.

Я имел ввиду, что у вас метод уже создает что-то — соответственно сочетать два таких метода вместе не удастся. Метод не умеет модифицировать уже готовый ордер, например.


Здесь страдает не читаемость кода теста, а ошибки в ходе прогона тестов.

Я тоже про них.
Обычно названия метода-теста выводятся при ошибках, так, что если мы видим что тест проверяет зарезервированность И ошибка в том, что количество отличается от заданного можно сказать что проверяется зарезервированное количество.


Секция 8.3.4 Separating asserts from actions

Я думаю, имелось ввиду action — это то, что мы проверяем, а assert — то, как мы проверяем. В моем коде action это AcceptOrder. Я посмотрел несколько примеров из книжки, там используются выражения и вызовы методов в assert.

Я имел ввиду, что у вас метод уже создает что-то — соответственно сочетать два таких метода вместе не удастся. Метод не умеет модифицировать уже готовый ордер, например.

Да блин возможности по композиции в .NET почти безграничны, зачем вам метод createAll.


Обычно названия метода-теста выводятся при ошибках…

А ну да обрубок вроде Method_ShouldWorkAsExpected_WhenGetsRightValue заменяет полноценную фразу на человеческом языке(xUnit не в счет)


Я думаю, имелось ввиду action

Да но ваш service это объект тестирования, так что это сбивает с толку.

Да блин возможности по композиции в .NET почти безграничны, зачем вам метод createAll.

ну вот вы сделали CreateProduct(ingridient: "olive", amount: 10) потом в другом месте сделали CreateSpecialProduct() а потом с третьем месте понадобилось CreateSpecialProduct(ingridient: "olive", amount: 10) — как вы их скомпозируете? Придется рефакторить.


А ну да обрубок вроде

Method_ShouldWorkAsExpected_WhenGetsRightValue — так это и есть фраза на человеческом языке с точностью до написания. Мы ж все равно ее пишем, чем нам написать еще раз облегчает дело?


Да но ваш service это объект тестирования, так что это сбивает с толку.

Как сбивает? Я же обращение к методу с целью проверки результатов поместил внутрь assert — наоборот, в этом случае Act отделен он Assert если вынести GetReservedAmount в отдельную переменную, придется писать комментарий
// Assert
перед GetReservedAmount. Нет?

Вы, конечно, извините, но это фигня какая-то. Первое что приходит в голову — кто-то перечитал паттернов, в особенности билдера.
Теперь больше конкретики:
1) Написав такой DSL вы тестируете в итоге работу DSL в первую очередь, а не систему.
2) Тесты позволяют в том числе под другим углом посмотреть на удобство кода и открытость его для изменений и тестирования, DSL — уменьшает этот момент.
3) Для лучшей читаемости есть комментарии и cucumber(в том числе для .net)
4) Debug такого теста может быть очень увлекательным, DSL имеет свойство становиться сложнее со временем.
  1. Это не так.
  2. То, что удобно использовать при разработке бизнес-логики не обязано быть удобным для тестов и DSL как раз помогает сгладить это несоответствие.
  3. Мы стараемся писать комментарии только в самых крайних случаях. А использовать BDD фреймворки в юнит-тестах мне представляется оверхедом посильнее DSL.
  4. Дебаг любого теста со сложным сетапом не самое приятное занятие. Согласен, DSL явно не поможет его упростить, но и не сильно усложнит.
по-моему, структуры в духе 4.May(2019) — это уже перебор. Они не имеют никакого преимущества перед new DateTime(2019, 5, 4). Если хотите, чтобы читалось без IDE и людьми из бизнеса — можно new DateTime(year:2019, month:5, day:4). А если вместо 5 хотите май — можно объявить константу. Просто extension-методы так и названы, потому-что они должны расширать функциональность определенного класса. Ну или хотя-бы создавать обертку, а вот пихать себя в DateTime — это определенно не функциональность класса int. Более того, существует возможность в попытке создать эту дату записать ее не как 4.May(2019), а 2019.May(4) и все сломать.
У нас в команде тоже нет консенсуса по этому вопросу) В статье оно приведено для ознакомления. Кому-то нравится, кому-то нет.
new DateTime(2019, 5, 4)
Где здесь месяц, а где день? Если не помнишь конструктор наизусть — не очевидно. А если код смотрят люди из разных стран с разными локалями (например, смотрит аналитик из США), то может быть совсем печально.
но в 4.May(2019) тоже самое. Ну и специально для этого я добавил версию DateTime(year:2019, month:5, day:4)
Ну, тащемто, в 4.May(2019) очевидно где месяц :)
простите, имел в виду не день и месяц, а день и год в момент написания int.May(int)

Год, месяц, день. Как в формате ISO 8601, в JavaScript, в SQL, в Java. И потом, это часть базовой библиотеки, и с датами приходится сталкиваться достаточно часто, чтобы порядок аргументов в конструкторе запомнился. А вот в 4.May(2019) — это как раз локальный российский формат, людям из разных стран с разными локалями он не поможет.

1) Builder не нужен в WhenAcceptOrder_AddIsCalled и WhenAcceptOrder_ReserveIngredientsIsCalled, конструирование его излишне и не помогает в достижении цели теста. Тут достаточно мока order.
2) Не понятно, зачем нужен PizzeriaServiceTestable? Для доступа к protected методам? А стали они protected только ради тестов? Если вам нужно верифаить protected/private методы, возможно, вы что-то делаете не так.
  1. Не понял, почему не помогает? Ордер — это, кстати, не мок, а тестовые данные.


  2. Конкретно здесь Testable нужен только для того, чтобы завернуть в себя ассёрты на моках и создание самих моков. Protected методов тут нет.



Но, например для рефакторинга легаси можно его использовать и для вызова protected. Пользы будет больше чем зла.

То ли пример неудачный, то ли я чего не понял, но тесты выглядят слишком синтетическими. То есть — а что, собственно, они должны сигнализировать?
Если тест не прошёл, значит «что-то не так»?
  • либо какая-то (неизвестно какая) задейстованная функция была как-то изменена (например, Create.Order перестал принимать две пиццы подряд в этом формате). Т.е. так-то всё везде работает (переписали уже), нужно теперь ещё тесты переписывать.
  • либо у вас поменялось меню и Pizza.Pepperoni или Pizza.Margarita теперь не существуют или называются по-другому
  • либо в самом деле «что-то не так»

Если тест прошёл, значит «всё как надо»? Нет:
  • проверяется только факт вызова Add и ReserveIngredients, а не результат их работы. То есть внутри этих функций можно хоть весь код удалить и всё будет продолжать тестироваться на ура
  • проверяется только на этом конкретном заказе. Т.е. WhenAcceptOrder_ReserveIngredientsIsCalled() выглядит обманчиво — звучит так, как будто при любом приёме заказа должны (и будут) резервироваться ингредиенты. И даже если сейчас это так, то через год у вас в товарах могут добавиться электронные книги. Т.е. тест будет зелёный, а логика не проверена.


Не лучше ли, когда тест просто проверяет, что «при заказе пиццы Пепперони зарезервировано 150 г. сыра»? При этом Пепперони заменяется на «номер один в текущем меню» а 150 и «сыр» на соответсвующее количество и ингредиент из БД для этого номера. И без разницы, вызовом каких функций и как это происходит.

Можно ли накосячить при написании DSL? Да, разумеется.
Можно ли накосячить в сложном сетапе юнит-теста? Да, разумеется.
Разница только в том, что в первом случае косяк придется править только в одном месте, а во втором во всех тестах, использующих этот сетап.


По поводу того, что проверяется только вызов метода: это называется "тест на поведение". О том, что следует писать тесты на состояние, а не на поведение я упоминал в статье.


Ну и в целом, если юнит-тесты проходят, это еще далеко не значит, что у вас всё хорошо. Зато если они падают, значит всё точно плохо :)

Если тест не прошёл, значит «что-то не так»?

Вы же не знаете, какое приложение человек пишет. В данном случае, понятно, что человек пишет компилятор c#. С-но, он написал на c# код с вызовом определенных методов, и теперь проверяет, что данный код, будучи скомпилированным и запущенным, действительно вызывает эти методы.
Просто название для теста придумано неудачное, вот и все.


Кстати, в чем проблема была написать просто:


[Fact]
public void AcceptOrder_Successful()
{
  var order = new Order(DateTime.Now); 
  service.AcceptOrder(order);

  orderRepositoryMock.Verify(r => r.Add(order), Times.Once);
  ingredientsRepositoryMock.Verify(r => r.ReserveIngredients(order), Times.Once);
}

Или ф-я AcceptOrder занимается валидацией заказа и не пропустит такой заказ? Так надо тогда вынести валидацию в отдельную ф-ю и мокнуть ее, чтобы она сделала valid на order.
И валидацию проверять отдельным тестом самой ф-и валидации, т.к. иначе п.2 (сфокусированность) нарушено.

Успех выполнения операции состоит из кучи маленьких успехов. Это и запись в базу, и корректный возвращенный результат, и публикация сообщения в MQ. Это технически можно записать в один тест, но принцип «один тест — одна проверка» существует не просто так. Удобно, когда по одному взгляду на результат прогона (не на детали упавших тестов, а именно на результат прогона) видно, что именно идет не так. Например, кто-то нарушил формат сообщения в MQ, в прогоне может быть удобнее увидеть не просто: AcceptOrder_Successful failed, а скорее AcceptOrder_EventPublished failed. Все остальные два-три-десять тестов, которые тоже проверяют успех при этом вполне могут быть зелеными. Я сразу по прогону вижу, какая часть моего, возможно, здоровенного метода сломалась.
Мне вот, кстати, поэтому нравится семантика Should. Когда тестовый класс заканчивается на слово Should и содержит тесты иногда только на один метод класса. Если у меня в классе типа AuthenticationService имеется два нетривиальных метода Authenticate и Logout, например, то мне будет не очень удобно наблюдать огромный тестовый класс, где черт ногу сломит. Вместо этого можно сделать тестовые классы AuthenticationService.AuthenticateShould и LogoutShould и в каждой фикстуре держать по пяток проверок, которые реально нужно осуществлять. Может получиться что-то типа.
public class AuthenticateShould
{
    [Fact]
    public async Task ReturnSuccess_WhenTokenCorrect()
    {
         var actual = await Authenticate("correct_token");
         actual.Result.Should().Be(AuthenticationResult.Success);
    }

    [Fact]
    public async Task ProlongateSessionForHour_WhenTokenCorrect()
    {
        currentMomentSetup.Returns(new DateTime(2019, 01, 01, 10, 05, 20));
        await Authenticate("correct_token");
        sessionRepository.Verify(r => r.RefreshSession(It.Is(userId), It.Is(sessionId), It.Is(new DateTime(2019, 01, 01, 11, 05, 20)), Times.Once);
        sessionRepository.VerifyNoOtherCalls();
    }

    [Fact]
    public async Task UpdateUserActivity_WhenTokenCorrect()
    {
        currentMomentSetup.Returns(new DateTime(2019, 02, 03));
        await Authenticate("correct_token");
        userRepository.Verify(r => r.Update(It.IsUpdateBuilder<User>().WithField(u => u.LastActivityDate, new DateTime(2019, 02, 03))), Times.Once);
        userRepository.VerifyNoOtherCalls();
    }
}

Вот мне интересно про VerifyNoOtherCalls — это не приводит к излишней хрупкости теста? Т.е. фактически мы проверяем не только что он делает что-то но и то что он не делает ничего еще — если в будущей появится новое требование мы должны будем переделывать все такие тесты. Что из этого получается на практике?


Еще хотелось бы увидеть общий подход к организации тестов — на какие куски делите, и какими инструментами отслеживаете структуру (сколько уровней категоризации и так далее и насколько устраивает то, что получилось)?

Я стараюсь использовать VerifyNoOtherCalls только когда используются какие-то внешние зависимости, которые чаще всего приводят к проблемам — базы данных здесь в приоритете, так что репозитории, по-моему, лучше как-то контроллировать.
Если возникнет необходимость дважды вызывать репозиторий, то тогда конечно придется этот тест переписывать, но по-моему лучше держать архитектуру такой, чтобы этого не случалось, если есть возможность.

По поводу структуры — предпочитаю не повторять структуру неймспейсов из тестируемого проекта, вместо этого просто держу все в папках с названиями классов, а внутри делю по тестовым классам по методам или даже подметодам. В случае с примером выше, можно даже целый тестовый класс назвать AuthenticationService.AuthenticateWithCorrectTokenShould. Критерия по разделению строго нет, просто слежу за тем, чтобы тестовые классы не становились слишком жирными. Для этого еще отдельно обращаю внимание на неиспользуемые зависимости, стараюсь, чтобы их было поменьше. Это, как мне кажется, помогает придерживаться SRP, а в итоге вместо обычных для индустрии медиаторов типа CommentaryService, где сложена вся логика работы с комментариями, у меня есть CreateCommentaryService, DeleteCommentaryService, LikeCommentaryService и ReplyCommentaryService. Все классы строго отвечают за бизнес-процессы, очень легко покрываются тестами, которые даже не всегда надо разносить по разным файлам (как можно понять по названиям, в них редко бывает больше одного метода).

Пока вроде бы устраивает, что получается. В основном даже не в самих тестах, а в том, как меняется код, чтобы повысить свою тестабильность.
Итак, я хочу сделать так, чтобы в моём тестовом методе не было ни одного мока.

PizzeriaServiceTestable проверяет неявные выходные данные (indirect output) тестируемой системы. Фактически, это мок, просто специализированный.

Sign up to leave a comment.