Пример с запашком
Для того, чтобы проиллюстрировать проблему, придумал пример. Идея его взята из одного реального проекта. В идеале, как кто-то может заметить, класс надо было бы написать иначе. Но сейчас никто не станет его переписывать (или рефакторить), т.к. это приведет к большим затратам времени на правку и тестирование того, что и так работает. Поэтому, это не будет одобрено. При этом изменять код надо и изменения должны быть правильными. И так, вот код. Важная логика сосредоточена в методе ProcessMessage. Задача: внедрить новый флаг бизнесс-логики в обработку сообщений. Этот флаг имеет нетривиальную логику.
Как будете внедрять и тестировать флаг?
using System;
using System.Threading.Tasks;
using System.Threading;
using System.Messaging;
namespace PublicMethodNottrivialSample
{
public class MessageProcessorService
{
private object _lockObject = new object();
private CancellationTokenSource _cancellationSource;
private Action _receiveMessage;
private int _listenerThreads = 5;
public void Start()
{
lock (_lockObject)
{
_cancellationSource = new CancellationTokenSource();
Task.Factory.StartNew(() => InitializeReceiveLoop(_cancellationSource.Token), _cancellationSource.Token);
}
}
private void InitializeReceiveLoop(CancellationToken cancellationToken)
{
_receiveMessage = () =>
{
while (!cancellationToken.IsCancellationRequested)
{
using (MessageQueueTransaction msgTx = new MessageQueueTransaction())
{
try
{
msgTx.Begin();
// получить сообщение
MessageQueue queue = new MessageQueue();
Message message = queue.Receive(msgTx);
// проверить остановлен ли процесс, пока получали сообщение
if (!cancellationToken.IsCancellationRequested)
{
ProcessMessage(message);
}
msgTx.Commit();
}
catch (Exception ex)
{
// some logging
}
}
}
};
for (int n = 0; n < _listenerThreads; n++)
{
StartProcessingLoop(cancellationToken);
}
}
private void ProcessMessage(Message message)
{
// некоторый важный функционал, куда внедряется новый флаг
}
private void StartProcessingLoop(CancellationToken cancellationToken)
{
Task.Factory.StartNew(_receiveMessage, cancellationToken);
}
}
}
В классе видно, что единственный public-метод — это Start(). Его можно протестировать, если изменить сигнатуру, но в этом случае изменится публичный интерфейс. Кроме того, потребуется менять несколько методов, чтобы возвращать запущенные потоки и потом ожидать их завершения в тесте.
Но, как помним, требование касалось только внедрения флага в процесс обработки сообщения, и не предполагало изменение работы механизма получения сообщений. Поэтому, кто бы ни вносил изменения, я ожидал бы, что исправлен будет только один метод, а юнит-тест имел бы отношение только к изменяемой логике. Добиться этого, оставаясь в рамках принципа сложно: тест получится нетривиальным, что повлечет либо отказ от его написания, либо усложненный код. Вот так и начинает подванивать тестирование через публичный метод. И кто-то потом эмоционально напишет: «TDD — это долго», «Заказчик не оплачивает», или «Тесты плохо работают».
Вообще, тестировать такой код надо, но не юнит-тестами, а интеграционными.
«Вам шашечки, или ехать»
Безусловно, написать юнит-тест на изменённую логику необходимо. Считаю, что дилемма, в данном случае, состоит в выборе наименее затратного способа написания теста, а не полезного кода. Тут я имею виду тот факт, что что бы вы ни делали: рефакторинг для public-метода или другое решение — вы будете делать это с целью написать тест, а не решить требование из задачи клиента. В этом случае, целесообразно оценить затраты и эффект. В дополнение к выше указанному решению с рефакторингом существует еще несколько альтернативных решений. Привожу все, ниже обсудим За и Против:
- можно тестировать private-метод
- можно метод сделать публичным
- можно сделать внутренним (internal)
- можно вытащить в отдельный класс, как публичный метод
Если сравнить первые три способа с решением через публичный метод, но все они требуют меньше трудозатрат (с private не факт). Так же, все они, по сути, являются одинаковым решением, с небольшими стилистическими отличиями. Т.к. результат будет получен любым способом, поэтому, на мой взгляд, выбор в пользу одного из этих решений, должен опираться не на технические возможности языка, а на то, что вы хотите показать другим разработчикам:
- если метод можно запускать из любого места в солюшене и он является частью поведения класса, то делайте его публичным и вытягивайте в интерфейс класса;
- если метод можно использовать из любого другого класса, но только внутри данной сборки, то делайте его internal;
- если метод можно использовать только внутри основного класса, где он может быть вызван из любого другого метода, то делайте его protected internal.
Internal-методы можно сделать видимыми сборке с тестами с помощью атрибута InternalsVisibleTo и вызывать, как обычно. Такой подход упрощает написание тестов, а результат будет тем же.
Тестируем ProcessMessage
Вернемся к задаче. Руководствуясь подходом выше я внёс бы несколько изменений:
- сделал бы ProcessMessage публичным
- создал бы новый protected internal метод для логики флага (например, GetFlagValue)
- написал бы тесты для GetFlagValue на все случаи
- написал бы тест для ProcessMessage, чтобы убедиться, что GetFlagValue правильно используется: правильно передаются параметры и он действительно используется
Уточню, что я не стал бы писать юнит-тесты на все случаи использования GetFlagValue в методе ProcessMessage с условием, что я протестировал эти случаи в юнит-тестах GetFlagValue. В случае обнаружения непокрытых случаев, их необходимо дописывать. При этом основной подход остается пржним:
- все случаи покрываются юнит-тестами к GetFlagValue
- юнит-тесты ProcessMessage проверяют правильное использование метода флага
Считаю, что в соответствии с этим можно написать только один юнит-тест для ProcessMessage и несколько для GetFlagValue.
Как-то так. Ваши мнения?
Комментарии (34)
Komesk
19.08.2018 17:45Недостаточно информации для ответа на ваш вопрос. Коль это новый флаг, значит предыдущие как-то тоже тестировали? Есть ли аналог или общий интрефейс типа GetFlagValue для других флагов?
На вскидку, я бы не стал делать ProcessMessage pubiс. Судя по InitializeReceiveLoop он не самодостаточен.
sentyaev
19.08.2018 17:55+1Тут самое место для паттерна Visitor.
Метод ProcessMessage приватный, это нам поможет. Т.к. в процессе рефакторинга интерфейс класс никак не поменяется.
В результате рефакторинга он будет выглядеть так:
private void ProcessMessage(IMessage message) { message.DoProcess(new MessageProcessor()); // Наивно создаю процессор // Можно внедрить процессор или сделать синглтон или еще что-то что придет вам в голову, вариантов масса. }
Итак, собственно рефакторинг.
1. Определим интерфейс IMessage, все сообщения будут его имплементировать.
interface IMessage { void DoProcess(MessageProcessor messageProcessor); }
2. Определим два класса сообщений.
class SimpleMessage : IMessage { public void DoProcess(MessageProcessor messageProcessor) { // double dispatch messageProcessor.Process(this); // вызовет Process(SimpleMessage message) } } class ComplexMessage : IMessage { public void DoProcess(MessageProcessor messageProcessor) { messageProcessor.Process(this); // вызовет Process(ComplexMessage message) } }
3. Определим класс MessageProcessor.
class MessageProcessor { public void Process(SimpleMessage message) { Console.WriteLine("Simple Message"); } public void Process(ComplexMessage message) { Console.WriteLine("Complex message"); } }
Далее нужно слелать так, что бы медотReceive мз MessageQueue возвращал не конкретный класс Message, а интерфейс IMessage.
IMessage message = queue.Receive(msgTx);,
а метод ProcessMessage должет принимать IMessage вместо MessageProcessMessage(IMessage message)
Собственно все.
Суть в том, что от задачи тестирования класса приведенного в статье, мы перешли к тестированию методов из класса MessageProcessor, в которых сосредоточена вся логика по обработке сообщения, в ProcessMessage в классе MessageProcessorService теперь состоит из одной строки и делает message.DoProcess(MessageProcessor), что и тестировать не нужно.ETman Автор
19.08.2018 23:40Мне нравится ваше решение. Но есть нюанс. Такие изменения слишком большие для задачи. Т.е. как задача по техническому долгу она отличная и, когда будет одобрена, оно будет примерно так и решено. Но сейчас надо внести минимум изменений. Я умышлено помещаю ситуацию в условия, когда у вас времени не вагон, и надо все сделать так, чтобы на это можно было опираться в дальнейшем и это не вызывало головной боли у других разработчиков.
Как вы объясните зачем нужны такие большие изменения ради одного флага?sentyaev
20.08.2018 02:16Предложенное решение вносит меньше изменений в существующий код, чем «сделать метод публичным», т.к. интерфейс класса MessageProcessorService совсем не изменяется, соответственно это не может внести ошибок в код его использующий.
Можно сделать еще меньше изменений в существующем коде.
Я писал:
Далее нужно слелать так, что бы медотReceive мз MessageQueue возвращал не конкретный класс Message, а интерфейс IMessage.
IMessage message = queue.Receive(msgTx);
Можно и без этого обойтись:
// получить сообщение MessageQueue queue = new MessageQueue(); Message message = queue.Receive(msgTx); // Используем фабричный метод и адаптер чтобы получить нужый класс из сообщения // MessageAdapter.Create возвращает IMessage var adaptedMessage = MessageAdapter.Create(message) // проверить остановлен ли процесс, пока получали сообщение if (!cancellationToken.IsCancellationRequested) { // Используем адаптированное сообщение ProcessMessage(adaptedMessage); } ...
В результате, не придется менять код класса MessageQueue. Все изменения касаются только деталей реализации класса MessageProcessorService.
К сожалению, сложно понять что вы имели ввиду под «нужно ввести новый флаг», попробую сделать предположение, а если оно будет неправильным, вы уж меня поправьте.
Предположим логика обработки сообщения такая:
private void ProcessMessage(Message message) { // некоторый важный функционал, куда внедряется новый флаг if (message.IsA) { DoA(); } if (message.IsB) { DoB(); } }
Допустим дополнительный флаг этоif (message.IsC) { DoC(); }
Фабричный метод будет таким:
class MessageAdapter { public static IMessage Create(Message message) { // новая логика -> ComplexMessage if (message.IsC) return new CommplexMessage(message); // для обработки текущей логики создаем SimpleMessage return SimpleMessage(message); } }
А дальше весь код из текущей реализации ProcessMessage cut and paste в MessageProcessor.Process(SimpleMessage message) без изменений. В метод MessageProcessor.Process(ComplexMessage message) добавляем только новую логику.
Такие изменения слишком большие для задачи.
Изменения не большие, т.к не изменено ни одного интерйеса/контракта.
Т.е. как задача по техническому долгу она отличная и, когда будет одобрена, оно будет примерно так и решено. Но сейчас надо внести минимум изменений.
Так же нет попытки бороться с техническм долгот, только добавление «нового флага» и покрытие этого функционала тестами, как вы и ставили задачу.
Я умышлено помещаю ситуацию в условия, когда у вас времени не вагон,
Времени на это много не нужно, я сходу придумал решение и написал рабочую версию за пол часа, если посидеть подумать, то можно и лучше решение родить.
и надо все сделать так, чтобы на это можно было опираться в дальнейшем и это не вызывало головной боли у других разработчиков.
С описанным мной подходом можно и тесты добавить, и сколько угодно новых флагов ввести. Головной боли не будет, добавил новый класс для Message и определить новый метов в MessageProcessor для его обработки — все дела.
Ну и самое интересное…
Как вы объясните зачем нужны такие большие изменения ради одного флага?
С одной стороны — я написал тривиальнейший код. Не очень понимаю где вы увидили «большие изменения». С другой стороны — не понятно кому это нужно объяснять?
Звучит так, как-будто вы хотите переложить принятие решения писать плохой код на лида/менеджера/бизнес.
Мы же инженеры, наша задача понять проблему, дать ей оценку и доказать, что делать нужно именно так.
lair
19.08.2018 19:28+2Давайте по шагам.
Ваш
MessageProcessorServiceнарушает SRP, хоть и слегка — в нем одновременно есть логика работы с очередью и логика обработки сообщений.
Ваш
MessageProcessorServiceнеудобен для тестирования, потому что в нем есть асинхронный код, который тестировать всегда неудобно.
Решение второй проблемы — сделать
ProcessMessageвидимым для тестов, не важно, как именно (зависит от доступного вам инструментария), и тестировать только его.
Решение обеих проблем — вынести обработку сообщений в отдельный класс (надо показывать, как?) и тестировать его стандартным способом без каких-либо проблем. В качестве бонуса гарантируем себе отсутствие лишней связности между очередью и обработкой сообщения.
Задача: внедрить новый флаг бизнесс-логики в обработку сообщений. Этот флаг имеет нетривиальную логику. Как будете внедрять и тестировать флаг?
Что значит "внедрить новый флаг"? Это неверная постановка задачи, особенно в терминах кода, который вы показываете. Нужно добавить новую ветку выполнения внутрь
ProcessMessageна основании чего-то в полученном сообщении? Или что?
(поэтому, кстати, и ваше "я внёс бы несколько изменений" выглядит бессмысленным — мне, например, совершенно непонятно, что такое "метод
GetFlagValueдля логики флага", и зачем там вообще метод)
кто бы ни вносил изменения, я ожидал бы, что исправлен будет только один метод, а юнит-тест имел бы отношение только к изменяемой логике.
Это ожидание неверно. "Только один метод" может быть исправлен только при соблюдении SRP (у вас это не так), и даже тогда не обязательно. А юнит-тест будет иметь отношение только к изменяемой логике в том случае, если есть юнит-тесты на логику до изменения. У вас это так?
если метод можно использовать только внутри основного класса, где он может быть вызван из любого другого метода, то делайте его
protected internal.Нет никаких оснований ни для
protected, ни дляinternal.
PS Ни тег TDD, ни "TDD — это долго" тут ни при чем: в посте нет ни следа TDD.
ETman Автор
19.08.2018 23:45Спасибо. В первой половине я рассматривал именно решение второй проблемы (как вы и сказали). Про идеальный вариант вы тоже написали (вынос в отдельный класс).
Со в торой частью согласен. Надо поработать над качеством изложения, т.к. много осталось «за кадром».Mikluho
20.08.2018 00:08+1Можете начать с того, чтобы поменять заголовок статьи? и tdd из тегов убрать? Чтобы быть честным с читателями. Потому как описываемый вами пример не имеет отношения ни к tdd ни к проблеме тестирования публичных контрактов.
А самое простое решение поставленной проблемы имеет классическое описание: вместо мутной логики в приватном методе делаем публичный класс, в который помещаем ту же самую логику и пишем столько тестов, сколько захотим. Приватный метод превращается в банальное обращение к этому классу и уже не требует собственных тестов.
Реальная же проблема, которая наблюдается в вашем примере, и присутствует в куда более жёстких проявлениях во всякий древних проектах — это макаронный код, где вперемешку логика и работа с внешним состоянием и внешними сервисами. Вот там да, на первый полезный тест можно пару недель убить…
lair
20.08.2018 00:08Для того, чтобы проиллюстрировать проблему, придумал пример.
Какую проблему вы иллюстрируете?
Но сейчас никто не станет его переписывать (или рефакторить), т.к. это приведет к большим затратам времени на правку и тестирование того, что и так работает.
… и тем не менее, именно этот вариант решения является основным (и, более того, не предполагает "больших затрат времени" ни на правку, ни на тестирование).
Так какую же проблему вы рассматриваете?
mwizard
MessageQueue.Receive()MessageProcessorService.Start()ETman Автор
Так просто не получится, потому что основной код выполняется параллельно и завершится позже выполнения assert-ов в тесте. Вам нужно после 3) ждать завершения созданных потоков, но в приведенном примере, без изменений вы это сделать не можете.
mwizard
Замокать конструктор CancellationTokenSource, у вас будет токен на момент начала теста. Выполнить `Start()`, дождаться `Receive()`, после этого протухнуть токен, дождаться очистки очереди задач, проверить сайд-эффекты.
ETman Автор
Можно проще, как я написал. Зачем так извращаться? Это должен быть не интеграционный тест, а юнит-тест. Простой и быстрый.
mwizard
Вы выставляете наружу детали внутренней реализации, пусть даже прикрывая это благозвучным термином «internal». Ваш публичный интерфейс должен изначально быть написан таким образом, чтобы реализацию в любой момент можно было выбросить, полностью переписать с нуля, и ни один тест при этом не сломался, т.к. тестируется не конкретная реализация, а то, что с ее помощью будет достигнуто — а как конкретно, не особо важно.
И да, TDD заключается в том, что сначала пишется тест, а потом под него код. Вы же пытаетесь натянуть сову на глобус, написав сначала ущербный код, а потом пытаясь его протестировать, запрещая при этом его изменять. Если бы ваш сервис принимал делегат для `MessageQueue.Receive` и существующий экземпляр токена отмены, проблем бы с тестированием не было бы никаких, независимо от потоковой модели вашего сервиса.
ETman Автор
Возможно я недостаточно описал, что в примере я не обсуждаю ситуацию из разряда «если бы». Как должно быть, понятно и об этом написано очень много других статей. Вы аппелируете к идеальному коду, когда есть возможность всё сделать, как надо. А я рассказываю о более реальной ситуации, когда есть говно-код, и все равно надо с ним работать и его тестировать. Это, конечно, не тот идеальный TDD, о котором идет речь у классиков, но это тоже написание тестов перед тем, как пишется сам функционал. Отличие в том, что вместо «писать сразу на имеющийся public» следует сесть и подумать о том, стоит ли овца выделки. Если тест для публичного метода, написан через workaround'ы и рефакторинг, сделаный под этот тест, то это плохой тест. Думаю, если он упадёт, еще и дебаг понадобится, чтобы понять что пошло не так.
Если же получится тест, который прост в поддержке (т.е. легко найти в нем проблему, если он упал), легок в обращении (т.е. если он перестал быть актуальным, его можно удалить, а вместо него написать другой), и при этом на его написание тратится 10 минут, то это хороший тест. И это ближе к TDD.
Относительно internal. Да, какая-то логика становится видна для тестов. Но это не страшно, т.к. тесты легкие и понятные. Если начали падать, то понятно почему. Если стали неактуальны — удаляем и пишем другие. Кроме того, internal — это еще не public. Internal не является хинтом, что этот метод должен быть использован везде, где можно. Кроме того, обычно в команде на этапе Code Review пресекаются неправльные использования. Да, это не идеальный TDD, но и громоздить код в настройках юнит-тестов публичных методов, и заниматься рефакторингом, по сути, ради рефакторинга, нет необходимости.
ETman Автор
Хотя, если все-таки рефакторинг сделали нормальный, то тест на публичный может быть тоже нормальным. Тут от многих факторов зависист.
worldbeater
Нет, неправда. В этом случае плох отнюдь не тест, а код, который мы тестируем. Модульные тесты описывают частные случаи использования публичного контракта объекта в изолированной среде. Если нужный нам частный случай не может быть протестирован, значит, объект не решает поставленную задачу и тестируемый код надо менять. TDD — оно именно про разработку через тесты, а не про "давайте напишем тесты к старому коричневому коду". Не нужно путать термины :)
lair
Вы Физерса не читали?
ETman Автор
Читал. К чему вопрос?
lair
К тому, что описанное в вашем посте точно попадает под описания из книги.
Chapter 10: I Can’t Run This Method in a Test Harness
[...]
"Fortunately, there is a very direct answer for this question: If we need to test a private method, we should make it public. If making it public bothers us, in most cases, it means that our class is doing too much and we ought to fix it. [...] The real reason we can’t test the [...] class well is that it has too many responsibilities. Ideally, it would be great to break it down into smaller classes using the techniques described in Chapter 20..."
Chapter 20: This Class Is Too Big and I Don’t Want It to Get Any Bigger
Я, собственно, некоторое время не мог понять, почему мне ситуация из поста кажется настолько тривиальной — вот именно поэтому: она описана и обсуждена больше десяти лет назад.
zvorygin
Есть мнение что плохой код сложно тестировать. В данном случае код плохой и он был плохим ДО добавления флага.
Почему есть метод Start() но нет метода Stop()? Зачем странно используемый lockObject?
Если всё это исправить(ну и многое другое) и сделать аккуратно и красиво то после 3) вызывается синхронный Stop() после которого проверяются все побочные эффекты
ETman Автор
Согласен, код плохой. Но вот он такой. Это часто в долгоживущих проектах, где код писался 10ю разными разработчиками. А тестировать все равно надо. Если переписать и исправить, то будет лучше. Но в реальности, это редко когда возможно. К сожалению.
zvorygin
Ну тогда не стоит удивляться что плохой код можно тестировать только с помощью костылей.
Идеальный вариант (если позволяет время и бюджет) сначала переписать этот код и покрыть тестами, затем добавить флаг и покрыть флаг тестами.
Я думаю что у каждого есть свой говнокод, который без костылей не протестировать. Применять ли тут паттерн «паблик морозов» я не знаю. Стоит ли рассказывать как тестировать говнокод с помощью костылей я тоже не знаю)
ETman Автор
Реалии большинства проектов такие, что там костыль на костыле и костылем погоняет. Можно не обсуждать. А я вижу проблему, что слишком много внимания уделяется идеальному коду и коротким проектам, где с нуля создается идеальная архитектура и идеально пишутся тесты. И всё что не так: говнокод и костыли. Хотя этого 80% (а то и больше) от всего общего количества кода. Мне хочется донести, что бояться писать тесты не надо. Но идеалистический подход и отношение к этому приводят к тому, что многим сложно в голове уместить идею юнит-тестирования. И подходят к ним, скорее, как к еще одному продакшн коду. Когда цель их в другом.
AxisPod
1. Это каким интересно образом?
Дальше смысла писать нет. Статья про тестирование нетестируемого кода, просто за подобный код надо руки отрывать, а не предлагать мокать то, к чему нет доступа.
qw1
Допустим, руки автору кода оторвали, а дальше что делать?
Всё переписывать с нуля? А если бизнес не согласен это оплачивать, то в своё свободное время?
AxisPod
Всё зависит от сложности кода, но исходя из данного примера, будет быстрее переписать код и сделать для него тесты, чем извращаться с тестами для данного примера.
qw1
Наверняка этот пример сильно урезан, чтобы показать суть. В реальных кейсах будут портянки кода килобайт по 150 в одном классе.
ETman Автор
так и есть.
mwizard
Я не специалист в c#, прошу меня запардонить, но разве в c# нельзя заменить системный класс своим на этапе компиляции? Т.е. сказать "то, что вам ранее было известно как System.Random, это теперь MySuperRandom"?
Ну ладно, пусть нельзя на этапе компиляции — можно ж наверное тогда перегрузить системную assembly своей? Что-то типа LD_PRELOAD, только для .net? Заменить все системные классы пустыми Proxy-подобными обертками, у которых нет своей логики, кроме как перенаправлять вызовы либо на оригинальные реализации, либо, если стоит флажок, дергать класс-делегат.
Если даже нельзя, остается хардкор — можно ведь пропатчить MSIL для целевого метода на ходу? Типа вот этого сплайсера? Или вот этой библиотеки (которая, как мне кажется, внутри делает то же самое)?
Да, естественно, это unsafe, но это же тест, а не продовый код. Если тест упадет, никто не умрет, тесты для того и предназначены.
lair
Microsoft Fakes. Но про TDD после этого можно забыть.
sentyaev
Спасибо за наводку! Когда-то слышал про это, но не стал пользоваться, нужно будет теперь разобраться.
Кстати, а почему про TDD можно забыть используя это? Что мешает?
lair
Потому что когда у вас есть возможность влезть куда угодно и подменить что угодно, вы совсем иначе думаете о структуре кода, нежели когда у вас такой возможности нет. Памятуя о максиме "тестируемый дизайн — хороший дизайн", если вы уберете давление со стороны тестируемости, вы понизите качество кода (ну или по крайней мере, драйвер в сторону качества).
mwizard
В Python и JavaScript есть возможность влезть куда угодно и подменить что угодно прямо из коробки, но я что-то не слышал, чтобы от этого страдало качество тестов.