Чтобы не заставлять вас бегать к предыдущей статье, приведу правило «Не тестируем алгоритм методов» еще раз, как оно там звучало:
Тут неудачно подобрано название правила, но лучшего пока не придумал. Среди «мокистов» (это те, кто мокает в тестах) есть те, кто проверяет количество вызовов определенных методов, верифицирует сам вызов и пр. Другими словами, занимается проверкой внутренней работы методов. Это так же плохо, как и тестирование приватных. Разница только в уровне применения такой проверки. Такой подход опять дает множество хрупких тестов, из-за чего TDD некоторыми не воспринимается нормально.
Есть следующий код хендлера:
public class SomeEventHandler
{
public SomeEventHandler(IDatabaseCommands dbCommands,
IEventValidator validator,
IMessagingLogger messagingLogger)
{
// skipped
}
public HandlerResult Handle(EventPayload payload)
{
if (Validator.IsOurEvent(payload))
if (Validator.IsValid(payload))
{
var evt = Mapper.Map<Event>(payload);
try
{
using (var tran = new TransactionScope())
{
DbCommands.SaveEvt(evt);
MessagingLogger.Received(payload);
tran.Complete();
}
}
catch (Exception ex)
{
return MessageHandlerResult.Fatal;
}
}
else
{
var error = Validator.GetErrors();
MessagingLogger.InvalidEvent(payload, error);
return MessageHandlerResult.Fatal;
}
return MessageHandlerResult.Success;
}
}
Необходимо протестировать работу метода Handle(). Вопрос стоит в том, чтобы убедиться, что методы DbCommands и MessagingLogger были вызваны.
Подход «мокиста»
Он передал бы в конструктор класса моки соответствующих интерфейсов, а после проверил бы вызвались или нет соответствующие методы: SaveEvt(), Received() или InvalidEvent(). Код выглядел бы примерно так:
public void Should_save_valid_data_and_log_to_messaging_events()
{
var builder = new EventPayload {
// skipped
};
var validator = Mock.Of<IEventValidator>();
var dbCommands = new Mock<IDatabaseCommands>();
var messagingLogger = new Mock<IMessagingLogger>();
var handler = new SomeEventHandler(dbCommands, validator, messagingLogger);
var result = handler.Handle(payload);
// assertions
Assert.Equal(MessageHandlerResult.Success, result);
dbCommands.Verify(m => m.SaveEvt(It.IsAny<Event>(), Times.Once())
messagingLogger.Verify(m => m.Received(It.IsAny<EventPayload>(), Times.Once())
}
Подход «немокиста»
Он создал бы fake objects и проверил бы совершилось ли событие в целом, а не вызов метода. В этом случае код был бы примерно следующим:
public void Should_save_valid_data_and_log_to_messaging_events()
{
var builder = new EventPayload {
// skipped
};
var validator = Mock.Of<IEventValidator>();
var dbCommands = new FakeDatabaseCommands();
var messagingLogger = new FakeMessagingLogger();
var handler = new SomeEventHandler(dbCommands, validator, messagingLogger);
var result = handler.Handle(payload);
// assertions
Assert.Equal(MessageHandlerResult.Success, result);
Assert.True(dbCommands.IsEventSaved);
Assert.True(messagingLogger.IsEventRegistered);
}
А методы fake-objects выглядели бы так:
public void SaveEvt(Event evt)
{
IsEventSaved = true;
}
При этом IsEventSaved был бы объявлен только в fake объекте.
Плюсы и Минусы
Первый подход прост и быстр, но если необходимо менять методы, вызывать один, вместо другого в той же ситуации, то тесты необходимо было бы править.
Второй подход приводит к созданию дополнительных сущностей, а выигрыш получается только в ситуации с заменой методов. В этом случае, возможно, даже ничего не придется менять ни в Fakes, ни в тестах. Еще один плюс, правда, более идеалистический, в том, что немокист делает тест так, чтобы он не знал о внутреннем устройстве тестируемого метода. Поэтому, лично я, если время позволяет, делаю тесты на fakes.
Комментарии (29)
cs0ip
13.11.2016 13:58+1Я немного не уловил разницу в этих двух подходах. В обоих случаях создается некая заглушка. В первом случае заглушку генерит Moq, во втором вы пишите её руками. Если в коде что-то меняется так, что первый вариант нужно переписать, то с вероятностью близкой в 1, второй вариант тоже надо будет переписать. Так в чем же разница?
ETman
13.11.2016 16:27Разница действительно незначительная. Она в том, что тест не знает о внутренней реализации тестируемого метода. Это важно для легкости тестов и понятности когда. Далее вы проверяете fake объектом не конкретную реализацию, а следствие (эффект) вызванный работой тестируемого метода.
Лично я и тот и другой использую подход и не вижу ничего критичного в замене одного другим. Второй случай мне кадется более показательным для описания логики метода.
lair
13.11.2016 14:04+3Необходимо протестировать работу метода Handle(). Вопрос стоит в том, чтобы убедиться, что методы DbCommands и MessagingLogger были вызваны.
[...]
[мокист] передал бы в конструктор класса моки соответствующих интерфейсов, а после проверил бы вызвались или нет соответствующие методы: SaveEvt(), Received() или InvalidEvent().
dbCommands.Verify(m => m.SaveEvt(It.IsAny<Event>(), Times.Once())
messagingLogger.Verify(m => m.Received(It.IsAny<EventPayload>(), Times.Once())
[...]
[немокист] создал бы fake objects и проверил бы совершилось ли событие в целом, а не вызов метода
[...]
А методы fake-objects выглядели бы так:public void SaveEvt(Event evt) => IsEventSaved = true;Так вот.
Во-первых, в обоих случаях у вас используются стабы (а не моки или фейки). Это согласно следующему определению Фаулера:
Stubs provide canned answers to calls made during the test, usually not responding at all to anything outside what's programmed in for the test. Stubs may also record information about calls, such as an email gateway stub that remembers the messages it 'sent', or maybe only how many messages it 'sent'.
(по классификации Мезароса это Test Spy: "A Test Spy is an object that can act as an observation point for the indirect outputs of the SUT.")
А из того, что оно одинаково называется, можно догадаться, что в реальности это, на самом деле, один и тот же подход. И это, в общем-то, тоже вытекает из фразы Фаулера в той же статье
The classical TDD style [который Фаулер противопоставляет "мокистскому"] is to use real objects if possible and a double if it's awkward to use the real thing. So a classical TDDer would use a real warehouse and a double for the mail service.
В нашем случае оба объекта, на которых мы делаем верификацию — это те, которые "неудобно использовать", поэтому, согласно Фаулеру, представители обоих подходов будут здесь использовать test doubles (что у вас и произошло).
Реальная разница между двумя подходами в вашей статье состоит в том, что первый проверяет требования как они были сформулированы (напомню: "убедиться, что методы DbCommands и MessagingLogger были вызваны"), а второй проверяет некоторое допущение о нужном бизнес-требовании.
Если вы хотите, чтобы этот код был менее "мокистским" и более "классическим", вам надо переформулировать требования — например, так: "убедиться, что данные сохранены и сообщение отправлено", — затем переписать оба test double так, чтобы они максимально соответствовали оригиналу, но использовали легкодоступное хранилище (например, оба складывали полученное в очередь), а затем в качестве проверки анализировать это хранилище. Вот тогда — да, вы получите тестирование вносимого эффекта.
Потому что в чем основная ошибка (обоих) ваших тестов? Они не тестируют, что именно передано в метод — а это, очевидно, нужно для корректного теста. Когда вы начнете это делать, первый код изменится незначительно (вместо
It.IsAnyпоявится нормальный предикат), а вот второй придется менять радикально.
funca
13.11.2016 16:33Test Doubles удобно рассматривать в разрезе ролей, которую они выполняют в тестах сайд-эффектов:
Stubs — предоставляют заданные значения для входов SUT (Indirect Inputs),
Mocks — дают возможность для верификации выходов SUT (Indirect Outputs),
Fake — реализуют штуки, которые в тесте не важны, но без которых SUT не работает (null-objects, dummy и т.п.).
В этом смысле оба варианта, предложенных в статье, предназначены для проверки Indirect Outputs и реализуют Mock. Первый использует программную генерацию (ad-hoc), второй написан лапками (hard coded).lair
13.11.2016 16:39Когда мне нужны детали, я предпочитаю классификацию Мезароса, в которой то, что описано в статье, называется test spy (и является частным случаем test stub), а мок — это то, что само проверяет indirect outputs.
Но в общем случае я вообще не вижу смысла в этом разделении, и предпочитаю использовать термин "мок" для любого test double вообще.
ETman
13.11.2016 16:36Очень позновательно о терминах. Спасибо. Про требования именно так.
На счет этого дам пояснения:
Потому что в чем основная ошибка (обоих) ваших тестов? Они не тестируют, что именно передано в метод
На мой взгляд именно так должен выглядеть самый первый тест на данную логику. По мере накопления багов или в ходе предпродакшн тестирования появятся тесты более специфияные. Но для работы данной логики такие детали, как передача конкретного значения может быть упущена. Это сделает тесты легкими и легко пишущимися/поддерживающимися. Спустя 3 месяца там 100% будет и тест на параметры и на какие-то значения, но зачем сразу?lair
13.11.2016 16:41+1Спустя 3 месяца там 100% будет и тест на параметры и на какие-то значения, но зачем сразу?
Затем, что тест "что-то вообще передали" весьма бессмысленен. Обычно важно, что именно было передано (особенно если в хэндлере есть перепаковка/трансформация).
И да, даже через три месяца стоимость добавления теста на конкретные значения все еще будет такой, как я ее описал.
ETman
13.11.2016 17:03С практической т.з. соглашусь. В случае проверки значений я бы не использовал второй подход, т.к. сделать нормально и потом поддерживать это потребует времени. Moq уже дает то, что необходимо.
С идеалистической я бы сделал иначе.
Во-первых, стоит ответить на вопрос: зачем тестировать значения? Если это необходимо, то вероятно существует логика трансформации в тестируемом методе. Эту логику можно вытащить и тестировать отдельно. Т.о. используя данный компонет для трансформации (маппинга) вы будете уверены, что он выдает на выходе всегда правильный результат. Если нет, вы добавите еще один случай для проверки этого маппера.
Во-вторых, в случае использования компонента-маппера внутри тестируемого метода, вам уже не надо будет заботиться о конкретных значениях. Это следует из допущения, что маппер работает так, как надо, а логики трансформации в тестируемом методе нет.
Например, в случае, приведенном в статье, есть код с AutoMapper. Его было бы неплохо вытащить в отдельный компонент и покрыть его тестами. Не сам AutoMapper, а враппер, который внутри использует AutoMapper. И тогда я был бы уверен, что передав некое значение, я всегда в методах DbCommands получу ему соответствующее. Как-то так.
Но опять же. Я не хочу категорично выступать за такой подход, т.к. следует по конкретной задаче смотреть, имея в голове представление о работе тестируемого кода.lair
13.11.2016 17:41Во-первых, стоит ответить на вопрос: зачем тестировать значения?
Чтобы знать, что код делает то, что от него ожидается.
Эту логику можно вытащить и тестировать отдельно.
Вытащить куда? Зачем? Он у вас и так вытащен:
Mapper.Map<Event>(payload);. Но вам же все равно надо протестировать что вы засовываете именно результат транформации, а не что-то другое.
Т.о. используя данный компонет для трансформации (маппинга) вы будете уверены, что он выдает на выходе всегда правильный результат.
… но (как и сказано выше), я не буду уверен, что я (а) подаю на вход транформации нужные данные и (б) использую результат транформации правильным образом.
в случае использования компонента-маппера внутри тестируемого метода, вам уже не надо будет заботиться о конкретных значениях.
Надо-надо. Смотрите выше.
Например, в случае, приведенном в статье, есть код с AutoMapper. Его было бы неплохо вытащить в отдельный компонент и покрыть его тестами. Не сам AutoMapper, а враппер, который внутри использует AutoMapper.
Избыточно. У AutoMapper уже давно есть свой собственный интерфейс, который можно так же вбрасывать и мокать, и не нужны никакие врапперы.
И тогда я был бы уверен, что передав некое значение, я всегда в методах DbCommands получу ему соответствующее.
Нет, не будете. Смотрите выше.
ETman
15.11.2016 10:09Вы правы и я с вами полностью согласен. Так будет правильнее, хотя писать тесты будет сложнее. Я даже уверен, что они станут менее поддерживаемыми, т.к. придется больше прописывать различных нюансов, как значения передаваемых типов. Мой тест не покажет проблему, если вдруг между маппером и следующим возовом DbCommands параметр приравнять NULL.
В данном случае, я оставляю эту часть проверки ручному тестированию. Думаю, программист в здравом уме не будет просто так вставлять NULL и не проверять данную модификацию никак. А закрыть все абсолютно дыры программно все равно не получится. Поэтому тут вопрос о балансе затрачиваемых ресурсов и результата, который дают тесты. Чрезмерно сильно проверять код там, где это не надо нет необходимости. У нас есть голова, чтобы знать и помнить о том, как он работает. А тест должен продокументировать основной ход выполнения, за счет его проверки, и проверить различные краевые случаи.
Т.е. в целом я согласен, но против чрезмерно дотошного тестирования юнит-тестами.lair
15.11.2016 12:01+2Так будет правильнее, хотя писать тесты будет сложнее. Я даже уверен, что они станут менее поддерживаемыми, т.к. придется больше прописывать различных нюансов, как значения передаваемых типов
Нет и нет. Если использовать Moq, то там есть прекрасный метод с автовыводом типов (предположим, что
commands— это то, что возвращает маппер):
dbCommands.Verify(m => m.SaveEvt(commands), Times.Once()); messagingLogger.Verify(m => m.Received(payload), Times.Once());
Кода стало меньше, чем с
It.IsAny, и явные указания типов пропали.
Если же использовать самописный test double, то типы прописывать в любом случае (и в любом случае при нормальном рефакторинге они изменятся автоматически).
Мой тест не покажет проблему, если вдруг между маппером и следующим возовом DbCommands параметр приравнять NULL.
Не только. Например, он не покажет проблему, если рачительный программист решит заменить автоматический маппинг ручным из соображений производительности и не замапит пару важных полей. Или он не покажет проблему, если один программист вынесет маппинг в общий private-метод, а другой в этом методе напишет логики.
В данном случае, я оставляю эту часть проверки ручному тестированию.
У ручного тестирования есть (как минимум) одна крупная проблема: его нельзя запускать тогда, когда мне нужно, и столько, сколько мне нужно. Поэтому чем меньше в моей жизни ручного тестирования, тем эта жизнь спокойнее.
Думаю, программист в здравом уме не будет просто так вставлять NULL и не проверять данную модификацию никак.
Вы зря так думаете.
У нас есть голова, чтобы знать и помнить о том, как он [код] работает.
Спасибо, но нет. Я не хочу знать и помнить, как работает код во всей системе на сотни и тысячи KLOC.
Т.е. в целом я согласен, но против чрезмерно дотошного тестирования юнит-тестами.
Вопрос, как определить "чрезмерно дотошное тестирование". Ну и да, я, когда мне что-то лень (или неудобно) покрывать юнит-тестами, пишу интеграционные (иногда даже сквозные).
ETman
15.11.2016 14:27Я восхищаюсь вашим уровнем погружения в данную область. Очень интересные и ценные комментарии.
funca
13.11.2016 16:55Необходимо протестировать работу метода Handle(). Вопрос стоит в том, чтобы убедиться, что методы DbCommands и MessagingLogger были вызваны.
Требование к Indirect Outputs какое-то мутное. Если по аналогии сформулировать требование к выходу (Direct Output) самого метода Handle(), то получится что-то типа: «Необходимо протестировать работу метода Handle(). Вопрос стоит в том, чтобы убедиться, что метод возвращает результат.».
Тот факт, что мы просто должны дергать за ручки back door систем, не очень осмысленный, так же как мало смысла в проверке, что метод возвращает «неважно что». При верификации выходов SUT (Direct или Indirect) в первую очередь стоит проверять, что выполняются контракты по значениям на этих самых выходах. А для этого и требования должны формулироваться соответствующим образом.lair
13.11.2016 17:02Требование к Indirect Outputs какое-то мутное
Оно не мутное, оно, гм, заранее завязано на реализацию (т.е., если следовать определениям из самого поста — оно заранее "мокистское").
При верификации выходов SUT (Direct или Indirect) в первую очередь стоит проверять, что выполняются контракты по значениям на этих самых выходах.
Можете привести пример?
funca
13.11.2016 17:37+1При тестировании SUT через API («front door») проверяются связи между значениями на прямых входах и значении на выходе (direct inputs и direct outputs). В данном случае при подаче на вход валидного значения Payload ожидается конкретный результат MessageHandlerResult.Success на выходе:
public void Should_save_valid_data() { var result = handler.Handle(validPayload); Assert.Equal(MessageHandlerResult.Success, result); }
Такого же уровня детализации стоит придерживаться и при формулировании требований в отношении Indirect Output при тестировании «back door»:
public void Should_log_to_messaging_events_on_saving_valid_data() { var result = handler.Handle(validPayload); messagingLogger.Verify(m => m.Received(It.Is(validPayload)), Times.Once()) }lair
13.11.2016 17:43Такого же уровня детализации стоит придерживаться и при формулировании требований в отношении Indirect Output при тестировании «back door»:
Вот только никакого контракта тут нет. Вы проверяете, что вы запихнули в логгер то же, что передали в хэндлер.
funca
13.11.2016 17:55Я так понимаю ответственность этого метода. А как бы вы описали контракт?
lair
13.11.2016 17:57Ответственность метода — не то же самое, что его контракт.
(собственно, контракты не могут распространяеться на indirect inputs/outputs)
funca
13.11.2016 18:12Можете как-то аргументировать это утверждение?
lair
13.11.2016 18:55Если вы позволите контракту распространяться на indirect in/out, контракт станет зависимым от реализации (а в пределе — станет идентичным реализации). При этом смысл контракта как раз абстрагировать реализацию.
Контракт — это публичная информация о компоненте; например, "сохраняет событие в персистентное хранилище". То, что при этом компонент (а) логгирует это событие и (б) делает сохранение через три метода (добавить, сохранить, закоммитить транзакцию) — это детали реализации, которые потребителя не интересуют.
Я обычно представляю себе это так: если бы я вытащил из компонента интерфейс, то контракт — это тот набор условий, который я вправе ожидать от этого интерфейса вне зависимости от реализации.
Saffron
14.11.2016 22:54Друзья, не надо ссориться. Пишите оба теста. Одни тегируйте как те, что проверяют публичный интерфейс, другие как те, что проверяют внутренний интерфейс.
chaetal
19.11.2016 19:03Есть следующий код хендлера:
Надеюсь, я правильно понимаю, что буковки «TDD» в первую строку заметки попали случайно и отношения к ней не имеют?
<…>
Необходимо протестировать работу метода
hVostt
Вроде как суть в подходе «мокиста» не просто проверить, что были совершены вызовы, но и проверить, что в эти вызовы было передано. Часто внутри используется несколько сервисов, одни что-то возвращают, чтобы передать результат другим сервисам. Именно это и надо проверить, если эти вызовы являются обязательными для гарантии правильной работы алгоритма. При разработке и рефакторинге можно забыть вызвать нужные методы, что не повлияет на работу алгоритма, например, запись в лог. Проблема «замены метода» в контексте юнит-тестов не является проблемой, так как если придётся переписывать тесты, это последнее что надо будет переписывать, и тесты надо будет переписывать отнюдь не из-за замены метода, а потому что поменялся тестируемый код. Использовать фейки только лишь в угоду более удобному рефакторингу, значит не понимать саму суть тестов, как я думаю.
ETman
Хочу уточнить правильно ли я Вас понял.
1) Вы утверждаете, что верифицируете сам факт вызова методов в купе со значениями передаваемых параметров.
2) Если кто-то создает классы, чтобы удобнее тестирвоать, то такой человек не понимает тестирование воообще.
Это то, что вы говорите?
Если в первом случае да, то мне интересно всегда ли Вы так пишете или только в некоторых случаях?
hVostt
1) Я говорю о том, что алгоритм тестируется проверкой результата работы алгоритма. Верифицирование фактов вызова методов и переданных значений является проверкой реализации алгоритма в дополнение к проверке результата. Т.е. если в алгоритм были переданы определённые интерфейсы, значит они должны определённым образом использоваться, иначе их наличие становится лишним. Тесты проверяют, что это так.
2) Нет. Если человек пишет тесты так, как будто это код приложения, применяя различные принципы кодирования, в том числе DRY, то это не правильный подход. Тесты не должны быть абстрактными классами, чтобы максимально адаптироваться, совсем наоборот. Изменение и рефакторинг кода должно провоцировать пересмотр тестов, если они в этом случае ломаются — это хорошо. Подозрительно, если они не ломаются.
Я стараюсь максимально протестировать каждый аспект алгоритмов. Если мы подставляем интерфейс, значит мы должны его мокать. Стараюсь избегать фейк-объектов в разумных пределах, так как это усложняет тесты, делая их похожим на код. Надо смотреть код фейков, а они в свою очередь могут использовать другие фейки и т.д. Так быть не должно. Каждый отдельный тест должен быть по моему мнению изолирован от других тестов. Хотя соблазн «улучшить» код тестов велик. Код тестов должен быть простым и понятным, даже если он будет уродлив с точки зрения программного кода.
ETman
Понятно, и я полностью согласен. Даже если в терминах есть разница, я все равно не вижу сильной разницы в подходах. Тесты должны легко писаться, читаться, а создание новых сущностей помогать их читать, а не усложнять.