Как стать автором
Обновить

Комментарии 35

  1. Замокать MessageQueue.Receive()
  2. Вернуть из замоканного метода сообщение, которое вызовет сработку нового флага.
  3. Вызвать MessageProcessorService.Start()
  4. Проверить, что обработка была выполнена правильно (судя по всему, ProcessMessage имеет побочные эффекты)
  5. ?????
  6. Вы великолепны.
Так просто не получится, потому что основной код выполняется параллельно и завершится позже выполнения assert-ов в тесте. Вам нужно после 3) ждать завершения созданных потоков, но в приведенном примере, без изменений вы это сделать не можете.
Замокать конструктор CancellationTokenSource, у вас будет токен на момент начала теста. Выполнить `Start()`, дождаться `Receive()`, после этого протухнуть токен, дождаться очистки очереди задач, проверить сайд-эффекты.
Можно проще, как я написал. Зачем так извращаться? Это должен быть не интеграционный тест, а юнит-тест. Простой и быстрый.
Вы выставляете наружу детали внутренней реализации, пусть даже прикрывая это благозвучным термином «internal». Ваш публичный интерфейс должен изначально быть написан таким образом, чтобы реализацию в любой момент можно было выбросить, полностью переписать с нуля, и ни один тест при этом не сломался, т.к. тестируется не конкретная реализация, а то, что с ее помощью будет достигнуто — а как конкретно, не особо важно.

И да, TDD заключается в том, что сначала пишется тест, а потом под него код. Вы же пытаетесь натянуть сову на глобус, написав сначала ущербный код, а потом пытаясь его протестировать, запрещая при этом его изменять. Если бы ваш сервис принимал делегат для `MessageQueue.Receive` и существующий экземпляр токена отмены, проблем бы с тестированием не было бы никаких, независимо от потоковой модели вашего сервиса.
Возможно я недостаточно описал, что в примере я не обсуждаю ситуацию из разряда «если бы». Как должно быть, понятно и об этом написано очень много других статей. Вы аппелируете к идеальному коду, когда есть возможность всё сделать, как надо. А я рассказываю о более реальной ситуации, когда есть говно-код, и все равно надо с ним работать и его тестировать. Это, конечно, не тот идеальный TDD, о котором идет речь у классиков, но это тоже написание тестов перед тем, как пишется сам функционал. Отличие в том, что вместо «писать сразу на имеющийся public» следует сесть и подумать о том, стоит ли овца выделки. Если тест для публичного метода, написан через workaround'ы и рефакторинг, сделаный под этот тест, то это плохой тест. Думаю, если он упадёт, еще и дебаг понадобится, чтобы понять что пошло не так.

Если же получится тест, который прост в поддержке (т.е. легко найти в нем проблему, если он упал), легок в обращении (т.е. если он перестал быть актуальным, его можно удалить, а вместо него написать другой), и при этом на его написание тратится 10 минут, то это хороший тест. И это ближе к TDD.

Относительно internal. Да, какая-то логика становится видна для тестов. Но это не страшно, т.к. тесты легкие и понятные. Если начали падать, то понятно почему. Если стали неактуальны — удаляем и пишем другие. Кроме того, internal — это еще не public. Internal не является хинтом, что этот метод должен быть использован везде, где можно. Кроме того, обычно в команде на этапе Code Review пресекаются неправльные использования. Да, это не идеальный TDD, но и громоздить код в настройках юнит-тестов публичных методов, и заниматься рефакторингом, по сути, ради рефакторинга, нет необходимости.
Хотя, если все-таки рефакторинг сделали нормальный, то тест на публичный может быть тоже нормальным. Тут от многих факторов зависист.
Если тест для публичного метода, написан через рефакторинг, сделаный под этот тест, то это плохой тест.

Нет, неправда. В этом случае плох отнюдь не тест, а код, который мы тестируем. Модульные тесты описывают частные случаи использования публичного контракта объекта в изолированной среде. Если нужный нам частный случай не может быть протестирован, значит, объект не решает поставленную задачу и тестируемый код надо менять. TDD — оно именно про разработку через тесты, а не про "давайте напишем тесты к старому коричневому коду". Не нужно путать термины :)

А я рассказываю о более реальной ситуации, когда есть говно-код, и все равно надо с ним работать и его тестировать.

Вы Физерса не читали?

Читал. К чему вопрос?

К тому, что описанное в вашем посте точно попадает под описания из книги.


Задача: внедрить новый флаг бизнесс-логики в обработку сообщений.

  • Chapter 8: How Do I Add a Feature?

Тестируем ProcessMessage

  • 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



Я, собственно, некоторое время не мог понять, почему мне ситуация из поста кажется настолько тривиальной — вот именно поэтому: она описана и обсуждена больше десяти лет назад.

Есть мнение что плохой код сложно тестировать. В данном случае код плохой и он был плохим ДО добавления флага.

Почему есть метод Start() но нет метода Stop()? Зачем странно используемый lockObject?

Если всё это исправить(ну и многое другое) и сделать аккуратно и красиво то после 3) вызывается синхронный Stop() после которого проверяются все побочные эффекты
Согласен, код плохой. Но вот он такой. Это часто в долгоживущих проектах, где код писался 10ю разными разработчиками. А тестировать все равно надо. Если переписать и исправить, то будет лучше. Но в реальности, это редко когда возможно. К сожалению.
Ну тогда не стоит удивляться что плохой код можно тестировать только с помощью костылей.

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

Я думаю что у каждого есть свой говнокод, который без костылей не протестировать. Применять ли тут паттерн «паблик морозов» я не знаю. Стоит ли рассказывать как тестировать говнокод с помощью костылей я тоже не знаю)
Реалии большинства проектов такие, что там костыль на костыле и костылем погоняет. Можно не обсуждать. А я вижу проблему, что слишком много внимания уделяется идеальному коду и коротким проектам, где с нуля создается идеальная архитектура и идеально пишутся тесты. И всё что не так: говнокод и костыли. Хотя этого 80% (а то и больше) от всего общего количества кода. Мне хочется донести, что бояться писать тесты не надо. Но идеалистический подход и отношение к этому приводят к тому, что многим сложно в голове уместить идею юнит-тестирования. И подходят к ним, скорее, как к еще одному продакшн коду. Когда цель их в другом.
1. Это каким интересно образом?
Дальше смысла писать нет. Статья про тестирование нетестируемого кода, просто за подобный код надо руки отрывать, а не предлагать мокать то, к чему нет доступа.
Допустим, руки автору кода оторвали, а дальше что делать?
Всё переписывать с нуля? А если бизнес не согласен это оплачивать, то в своё свободное время?
Всё зависит от сложности кода, но исходя из данного примера, будет быстрее переписать код и сделать для него тесты, чем извращаться с тестами для данного примера.
Наверняка этот пример сильно урезан, чтобы показать суть. В реальных кейсах будут портянки кода килобайт по 150 в одном классе.
так и есть.

Я не специалист в c#, прошу меня запардонить, но разве в c# нельзя заменить системный класс своим на этапе компиляции? Т.е. сказать "то, что вам ранее было известно как System.Random, это теперь MySuperRandom"?


Ну ладно, пусть нельзя на этапе компиляции — можно ж наверное тогда перегрузить системную assembly своей? Что-то типа LD_PRELOAD, только для .net? Заменить все системные классы пустыми Proxy-подобными обертками, у которых нет своей логики, кроме как перенаправлять вызовы либо на оригинальные реализации, либо, если стоит флажок, дергать класс-делегат.


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


Да, естественно, это unsafe, но это же тест, а не продовый код. Если тест упадет, никто не умрет, тесты для того и предназначены.

Microsoft Fakes. Но про TDD после этого можно забыть.

НЛО прилетело и опубликовало эту надпись здесь

Потому что когда у вас есть возможность влезть куда угодно и подменить что угодно, вы совсем иначе думаете о структуре кода, нежели когда у вас такой возможности нет. Памятуя о максиме "тестируемый дизайн — хороший дизайн", если вы уберете давление со стороны тестируемости, вы понизите качество кода (ну или по крайней мере, драйвер в сторону качества).

В Python и JavaScript есть возможность влезть куда угодно и подменить что угодно прямо из коробки, но я что-то не слышал, чтобы от этого страдало качество тестов.

Я вроде бы про качество тестов ничего не говорил.

Недостаточно информации для ответа на ваш вопрос. Коль это новый флаг, значит предыдущие как-то тоже тестировали? Есть ли аналог или общий интрефейс типа GetFlagValue для других флагов?
На вскидку, я бы не стал делать ProcessMessage pubiс. Судя по InitializeReceiveLoop он не самодостаточен.
Вам надо написать первый тест. Других нет.
НЛО прилетело и опубликовало эту надпись здесь
Мне нравится ваше решение. Но есть нюанс. Такие изменения слишком большие для задачи. Т.е. как задача по техническому долгу она отличная и, когда будет одобрена, оно будет примерно так и решено. Но сейчас надо внести минимум изменений. Я умышлено помещаю ситуацию в условия, когда у вас времени не вагон, и надо все сделать так, чтобы на это можно было опираться в дальнейшем и это не вызывало головной боли у других разработчиков.

Как вы объясните зачем нужны такие большие изменения ради одного флага?
НЛО прилетело и опубликовало эту надпись здесь

Давайте по шагам.


  1. Ваш MessageProcessorService нарушает SRP, хоть и слегка — в нем одновременно есть логика работы с очередью и логика обработки сообщений.


  2. Ваш MessageProcessorService неудобен для тестирования, потому что в нем есть асинхронный код, который тестировать всегда неудобно.



Решение второй проблемы — сделать ProcessMessage видимым для тестов, не важно, как именно (зависит от доступного вам инструментария), и тестировать только его.


Решение обеих проблем — вынести обработку сообщений в отдельный класс (надо показывать, как?) и тестировать его стандартным способом без каких-либо проблем. В качестве бонуса гарантируем себе отсутствие лишней связности между очередью и обработкой сообщения.


Задача: внедрить новый флаг бизнесс-логики в обработку сообщений. Этот флаг имеет нетривиальную логику. Как будете внедрять и тестировать флаг?

Что значит "внедрить новый флаг"? Это неверная постановка задачи, особенно в терминах кода, который вы показываете. Нужно добавить новую ветку выполнения внутрь ProcessMessage на основании чего-то в полученном сообщении? Или что?


(поэтому, кстати, и ваше "я внёс бы несколько изменений" выглядит бессмысленным — мне, например, совершенно непонятно, что такое "метод GetFlagValue для логики флага", и зачем там вообще метод)


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

Это ожидание неверно. "Только один метод" может быть исправлен только при соблюдении SRP (у вас это не так), и даже тогда не обязательно. А юнит-тест будет иметь отношение только к изменяемой логике в том случае, если есть юнит-тесты на логику до изменения. У вас это так?


если метод можно использовать только внутри основного класса, где он может быть вызван из любого другого метода, то делайте его protected internal.

Нет никаких оснований ни для protected, ни для internal.


PS Ни тег TDD, ни "TDD — это долго" тут ни при чем: в посте нет ни следа TDD.

Спасибо. В первой половине я рассматривал именно решение второй проблемы (как вы и сказали). Про идеальный вариант вы тоже написали (вынос в отдельный класс).

Со в торой частью согласен. Надо поработать над качеством изложения, т.к. много осталось «за кадром».
Можете начать с того, чтобы поменять заголовок статьи? и tdd из тегов убрать? Чтобы быть честным с читателями. Потому как описываемый вами пример не имеет отношения ни к tdd ни к проблеме тестирования публичных контрактов.
А самое простое решение поставленной проблемы имеет классическое описание: вместо мутной логики в приватном методе делаем публичный класс, в который помещаем ту же самую логику и пишем столько тестов, сколько захотим. Приватный метод превращается в банальное обращение к этому классу и уже не требует собственных тестов.
Реальная же проблема, которая наблюдается в вашем примере, и присутствует в куда более жёстких проявлениях во всякий древних проектах — это макаронный код, где вперемешку логика и работа с внешним состоянием и внешними сервисами. Вот там да, на первый полезный тест можно пару недель убить…
Для того, чтобы проиллюстрировать проблему, придумал пример.

Какую проблему вы иллюстрируете?


Но сейчас никто не станет его переписывать (или рефакторить), т.к. это приведет к большим затратам времени на правку и тестирование того, что и так работает.

… и тем не менее, именно этот вариант решения является основным (и, более того, не предполагает "больших затрат времени" ни на правку, ни на тестирование).


Так какую же проблему вы рассматриваете?

Зарегистрируйтесь на Хабре , чтобы оставить комментарий

Публикации

Истории