В программировании и в TDD, в частности, есть хорошие принципы, которых полезно придерживаться: DRY и тестирование через public-методы. Они неоднократно оправдали себя на практике, но в проектах с большим legacy-кодом могут иметь «тёмную сторону». Например, ты можешь писать код, руководствуясь этими принципами, а потом обнаружить себя разбирающим тесты, охватывающие связку из 20+ абстракций с конфигурацией, несоизмеримо превосходящей по объему тестируемую логику. Эта «тёмная сторона» пугает людей и тормозит использование TDD в проектах. Под катом я рассуждаю, почему тестировать через public-методы плохо и как можно уменьшить проблемы, возникающие из-за этого принципа.

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

Идея, лежащия в основе принципа, звучит хорошо: тестировать нужно поведение, а не реализацию. А значит, тестировать нужно только интерфейс класса. На практике это не всегда так. Чтобы представить суть проблемы представьте, что у вас есть метод, рассчитывающий стоимость работников занятых на сменной работе. Это нетривиальная задача, если речь идет о сменной работе, т.к. у них есть чаевые, бонусы, выходные, праздники, корпоративные правила и пр. и пр. Этот метод внутри выполняет очень много операций и использует другие сервисы, которые ему дают информацию о праздниках, чаевых и пр. Т.о. при написании для него юнит-теста необходимо создать конфигурацию для всех используемых сервисов, если проверяемый код находится где-то в конце метода. При этом, сам проверяемый код может использовать только частично, или вообще не использовать конфигурируемые сервисы. И есть уже некоторые юнит-тесты, написанные подобным образом.

Минус 1: Оверконфигурация юнит-тестов


Теперь вы хотите добавить реакцию на новый признак, который имеет нетривиальную логику и используется так же где-то в конце метода. Характер флага таков, что он является частью сервисной логики и, в тоже время, не является частью интерфейса сервиса. В приведенном случае, данный код актуален только для этого public-метода, и может быть, вообще, вписан внутрь старого метода.

Если в проекте принято правило тестировать всё только через public-методы, то разработчик может, скорее всего, просто скопирует какой-то существующий юнит-тест и немного его подправит. В новом тесте всё так же будет конфигурация всех сервисов для запуска метода. С одной стороны, принцип мы соблюли, но, с другой стороны, получили юнит-тест с оверконфигурацией. В дальнейшем, если что-то сломается, или потребует изменения конфигурации, придется делать мартышкину работу по корректировке тестов. Это нудно, долго и не приносит ни радости, ни видимой пользы клиенту. Казалось бы, следуем правильному принципу, но оказываемся в той же ситуации, от которой хотели уйти, отказываясь от тестирования private-методов.

Минус 2: Неполное покрытие


Дальше может вмешаться такой человеческий фактор, как лень. Например, private-метод с нетривиальной логикой флага может выглядеть так, как в этом примере.

private bool HasShifts(DateTime date, int tolerance, bool clockIn, Shift[] shifts, int[] locationIds)
{
    bool isInLimit(DateTime date1, DateTime date2, int limit)
        => Math.Abs(date2.Subtract(date1).TotalMinutes) <= limit;

    var shiftsOfLocations = shifts.Where(x => locationIds.Contains(x.LocationId));

    return clockIn
        ? shiftsOfLocations.Any(x => isInLimit(date, x.StartDate, tolerance))
        : shiftsOfLocations.Any(x => isInLimit(date, x.EndDate, tolerance));
}

Данный метод требует 10 проверок, чтобы покрыть все случаи, 8 из них существенные.

Расшифровка 8 важных кейзов
  • shiftsOfLocations — 2 значения — есть или нет
  • clockIn — 2 значения — true или false
  • tolerance — 2 разных значения

Итого: 2 x 2 x 2 = 8

При написании юнит-тестов для проверки этой логики, разработчику придется написать не меньше 8 больших юнит-тестов. Я сталкивался со случаями, когда конфигурация юнит-теста занимала 50+ строк кода, при 4 строках непосредственного вызова. Т.е. только, примерно, 10% кода несёт полезную нагрузку. В этом случае велик соблазн сократить объем работы за счет написания меньшего количества юнит-тестов. В итоге из 8 остается, например, только два юнит теста, для каждого значения clockIn. Такая ситуация приводит к тому, что либо, опять же, нудно и долго надо писать все необходимые тесты, создавая конфигурацию (Ctrl+C,V работает, куда же без него), либо метод остаётся только частично покрытым. У каждого варианта есть свои неприятные последствия.

Возможные решения


Кроме принципа «тестироват поведение», еще есть OCP (Open/closed principle). Применяя его правильно, вы можете забыть что такое «хрупкие тесты», тестируя внутреннее поведение модуля. Если вам понадобится новое поведение модуля, вы напишете новые юнит-тесты для нового класса-наследника, в котором будет изменено нужное вам поведение. Тогда вам не надо будет тратить время на перепроверку и корректировку существующих тестов. В этом случае данный метод можно объявить, как internal, или protected internal, и тестировать, добавив InternalsVisibleTo к сборке. В этом случае ваш IClass интерфейс не пострадает, а тесты будут наиболее локаничными, не подверженными частым изменениям.

Другой альтернативой может быть объявление дополнительного класса-хелпера, в который можно вытащить наш метод, объявив его, как public. Тогда и принцип будет соблюдён, и тест будет лаконичным. На мой взгляд, такой подход не всегда себя оправдывает. Например, некоторые могут решить вытаскивать даже один метод в один класс, что приводит к созданию кучи классов с одним методом. Другой крайностью может быть сваливание таких методов в один класс-хелпер, который превращается в GOD-helper-class. Но этот вариант с хелпером может быть единственным, если рабочая сборка подписана строгим именем, а тестовую сборку вы подписать не можете, по каким-то причинам. InternalsVisibleTo будет работать когда сразу обе сборки либо подписаны, либо нет.

Итог


И в итоге, из-за совокупности подобных проблем страдает идея TDD и юнит-тестов, т.к. желания писать объемные тесты и поддерживать их нет ни у кого. Буду рад любым примерам того, как четкое следование данному принципу приводило к проблемам и снижению мотивации писать тесты у команды разработчиков.

Комментарии (29)


  1. Mikluho
    11.08.2018 02:06
    +4

    Э…

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

    Т.е. вы не только противоречите общепринятым нормам, но ещё и просите у общественности помощи в свержении норм???

    Есть многое, что вам можно ответить на этот призыв… Но, пожалуй, хватит одного: «чёткое следование принципам» нередко оказывается итальянской забастовкой :)

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

    А касательно именно тестов — у вас проблема в другом месте. Вы имеете blackmagic box, который в дебрях своих что-то там делает. Такое не тестируют юнит-тестами. Первое, что нужно сделать, так это опубликовать поведение этого ящика. Должно получится некоторое количество открытых контрактов, которые вместе реализуют бизнес-задачу. Да, если у там и правда сложнозависимый алгоритм — под каждый сценарий придётся писать конфигурацию. Но философия TDD в том, что эта конфигурация есть описание предусловий сценария, а результат теста есть соответствие поведения ожиданиям. И если уж совсем следовать канонам — набор тестов есть ТЗ. Меняются требования — меняется ТЗ — меняются тесты — корректируется код.

    И это если не говорить о том, что бы у вас был TDD, описанная проблема была бы совсем невозможна…
    Да, легаси без тестов несовместимо с TDD. Даже просто unit-тесты прикрутить — надо постараться, но оно того порой стоит. А если не стоит — не надо ругать TDDЮ, надо выбирать подходящий инструмент — например интеграционные автотесты.


    1. ETman Автор
      11.08.2018 10:50

      Т.е. вы не только противоречите общепринятым нормам, но ещё и просите у общественности помощи в свержении норм???

      Я говорю, что принцип хороший, но при чрезмерно педантном отношении к нему, может приводить к проблемам.

      В случае с legacy-кодом есть проблема с написанием юнит-тестов, связанная с конфигурацией окружения. Я согласен, что полезно иметь такой тест, в котором будет понятно, как, собственно, использовать этот метод. При создании такого теста получается, своего рода, и описание работы модуля.

      Но когда у вас есть 10 таких тестов, и 5 из них падает с NullRef exception, в том месте, где не должно этого было быть, вам остается только сидеть и дебажить сами тесты. Это в корне, на мой взгляд, не соответствует идее TDD, согласно которой тесты должны быть простыми и ясными.


      1. Mikluho
        11.08.2018 11:34
        +1

        Я говорю, что принцип хороший, но при чрезмерно педантном отношении к нему, может приводить к проблемам.
        Чрезмерное отношение всегда плохо, везде.
        Как я уже написал, TDD и legacy не совместимы в стартовой позиции, ибо в TDD тесты бы уже были. Их нет — у вас другая проблема. Для её решения есть другие методологии. Если кратко — рефакторинг вас спасёт.


      1. ZelAnton
        11.08.2018 12:25

        Чтобы такого не происходило надо писать тесты к тестам


  1. Beyondtheclouds
    11.08.2018 02:39
    +5

    Это очень вредный подход.

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

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

    А проблема с копипастом может решаться хэлпером для создания минимальной дефолтной конфигурации.


    1. ETman Автор
      11.08.2018 11:11

      Конкретная реализация может быть сложной и приводить к большому усложнению тестирования публичных методов. Если вы стараетесь не изменять существующие классы, а больше руководтствоваться OCP, то проблема, о которой вы пишете, резко снизится. «При изменеии внутренней структуры» тесты далжны быть легко выкинуты и написаны заного. В этом и состоит задача тестов и TDD: быть простыми и легкими, как в понимании, так и в обращении.

      Хелперы — это палка с двумя концами. Один помогает, другой бьет по башке. Когда у вас есть лес классов из хелперов и непонятно, какой в вашем случае подходит, вы просто напишете новый. В итоге где-то в тестах что-то начинает валиться и вы тратите время на долгий и нужный дебаг тестов. Что опять, на мой взгляд, не соответвует идее TDD.

      Но я не говорю тут об интеграционных тестах.


      1. Beyondtheclouds
        11.08.2018 11:42

        >«При изменеии внутренней структуры» тесты далжны быть легко выкинуты и написаны заного.

        непонятна цель написания тестов в таком случае. При измении кода они не помогут вам найти ошибку.

        >Хелперы — это палка с двумя концами. Один помогает, другой бьет по башке.

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

        Возможно вам стоит начать с наведения порядка в проекте. Думаю что много аспектов станет проще, не только тестирование.


        1. ETman Автор
          11.08.2018 11:46

          Цель — обеспечить качество кода, когда изменения делаются не в самом тестируемом классе, а в используемых им других классах.

          То, что вы описываете с хелперами, это как написание еще одного продакшн кода. Потом понадоятся тесты для хелперов, чтобы проверить, что они правильно работают.


          1. Beyondtheclouds
            11.08.2018 12:38

            >Цель — обеспечить качество кода, когда изменения делаются не в самом тестируемом классе, а в используемых им других классах.

            1. У вас получается что качество кода «используемых им других классов.» обеспечивается тестами написанными у тех кто от этих классов зависит. Это достаточно странная логика.

            2. На автономные классы без зависимостей по такой логике тесты можно не писать.

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


  1. VolCh
    11.08.2018 10:18

    Можно тестировать непубличные методы в каких-то ситуациях, но при этом не за счёт снижения покрытия публичных. То есть в ситуации 2^3 тестов не заменяем их на 2•3, а дополняем, получая 14 тестов. Быстро при таком подходе приватные методы начинаешь тестировать только при реальной необходимости.


    1. ETman Автор
      11.08.2018 11:03

      А если рассмотреть вариант с другой стороны:

      • есть метод, не являющийся методом интерфейса класса, но выполняющий нетривиальную логику.
      • покрываешь его тестами так, чтобы быть уверенным в его 100% работе в известных случаях.
      • в публичном методе есть логика вызова этого метода, которую ты покрываешь (и только)

      Знаете технику Sprout Method? Это оно и есть.

      Ваш основной public-метод генерирует множество входящих значений параметров для непубличного метода. Вы проверятете не каждое значение, а то, что это множество правильно передается в непубличный метод. А непубличный метод вы проверяете тпо всему множеству передаваемых значений (по каждому важному). В итоге вам не надо писать 8 тестов для непубличного метода и 8 тестов для публичного. Для публично, в зависимости от логики, может оказаться достаточным, например, только 2.


      1. Mikluho
        11.08.2018 11:43

        есть метод, не являющийся методом интерфейса класса, но выполняющий нетривиальную логику. покрываешь его тестами так, чтобы быть уверенным в его 100% работе в известных случаях.
        А если этот метод сделать публичным или превратить в прокси к публичному классу/методу — то и заморочки с непубличностью пропадут. У вас снова в тестах будут только публичные методы :)
        Кстати, по вашей ссылке «Sprout Method» как раз про это.
        Иначе говоря — не надо тестировать приватное. Надо сделать публичным и тогда тестировать.


      1. Beyondtheclouds
        11.08.2018 11:45

        >В итоге вам не надо писать 8 тестов для непубличного метода и 8 тестов для публичного.

        Если у вас полностью покрыт публичный метод, и у вас есть правило покрывать все публичные методы. Непонятно зачем вообще в таком случае проверять приватный.


        1. VolCh
          11.08.2018 15:03

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


  1. AntonLozovskiy
    11.08.2018 10:51

    рефлексия наша все в тестах


    1. ETman Автор
      11.08.2018 10:53

      ну, только если «рефлексия» в смысле «опять не понятно почему этот юнит-тест падает»? ;)


  1. aamonster
    11.08.2018 11:05
    +2

    Теоретически, если вам понадобилось тестировать "промежуточную точку", дёргая приватный метод — было бы неплохо разрезать класс по этой промежуточной точке, тестируя публичные интерфейсы обрезков.


  1. DmitriyDev
    11.08.2018 11:46

    Похоже возникло недопонимание с тестами.
    Разработчик должен писать тесты не потому что этого хочет другой разработчик/менеджер, а потому что это надо разработчику. Все сы люди, все мы человеки. Мы пока ещё часто ошибаемся и у нас не идеальная память. Если мне через пол года надо будет править код, то тесты мне помогут с примерами использования и с большой долей вероятности защитят от ошибки.
    ИМХО, считаю что не надо заставлять писать тесты, а надо тбъяснять зачем это и показывать пример.
    И да, в моём понимании юнит тесты должны быть максимально глупыми. (Обычно в тестах забываю про наследования, дженерики… и тупо максимально всё забиваю ручками)


  1. lair
    11.08.2018 11:46
    +3

    Этот метод внутри выполняет очень много операций и использует другие сервисы, которые ему дают информацию о праздниках, чаевых и пр.

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


    Данный метод требует 10 проверок, чтобы покрыть все случаи, 8 из них существенные. [...] При написании юнит-тестов для проверки этой логики, разработчику придется написать не меньше 8 больших юнит-тестов.

    Это еще почему?


    Я сталкивался со случаями, когда конфигурация юнит-теста занимала 50+ строк кода, при 4 строках непосредственного вызова. Т.е. только, примерно, 10% кода несёт полезную нагрузку.

    Простите, а что в тестировании этого метода — конфигурация, а что — полезная нагрузка?


    1. avost
      11.08.2018 15:07
      +1

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

      Ноп. Проблема в том, что код, как положено, разбит на простые методы и есть метод, который всё собирает. Этот метод публичный. Простые методы — приватные. Итоговая логика объективно сложная и чтобы протестировать отдельные части, реализуемые отдельными приватными методами надо изрядно извернуться. Это не проблема кода, это именно проблема и ограничение юнит-тестирования. Согласно канону, конечно можно всё растащить по утилитным классам, но это усложнение программы только ради следования канонической теореме тестирования. По-моему, это как раз то место, где следование канону несёт один сплошной вред и никакой пользы.
      Вообще, лично мне кажется разумным было бы ввести в язык специальную аннотацию, помечающую метод как, вроде бы приватный, но, в то же время, открытый для тестирования. Пока для таких случаев некоторые используют пустую аннотацию, вроде @TheReasonTheMethodISPublikIsOnlyForTesting :). Костыль, конечно, но что делать. Рефлексия — ещё более кривой костыль.


      1. VolCh
        11.08.2018 15:18

        > чтобы протестировать отдельные части, реализуемые отдельными приватными методами надо изрядно извернуться.

        Обычно это говорит ещё и том, что извернуться надо будет, чтобы и просто штатно использовать этот публичный метод.


      1. lair
        11.08.2018 21:51
        +1

        Это не проблема кода, это именно проблема и ограничение юнит-тестирования.

        Неа. Юнит-тестирование вам не запрещает это делать (более того, в каком-то смысле — стимулирует, потому что приватный метод — тоже юнит). Природа рекомендации "тестируйте публичный интерфейс" в другом — этот подход требует меньше переписывания при рефакторинге. Внутреннее разбиение метода на приватные методы — детали его реализации, и может меняться раз в десять минут. Каждый раз тесты переписывать? А откуда брать критерии на каждый метод?


    1. xyzuvw
      11.08.2018 16:23
      +1

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

      Насколько я понял, автор имел в виду не переусложненный код, а простой код, задействующий большое число internal-классов, которые не хочется выносить в публичный API. Например, у вас есть библиотека, которая содержит один-единственный публичный класс DbBackupper с одним простым методом CreateBackup(string path), который делает резервную копию БД. Для реализации этого метода в библиотеке есть куча internal-классов, про которые пользователь знать не должен, и поэтому эти классы объявлены internal. Как быть?


      1. ETman Автор
        11.08.2018 16:23

        хороший пример проблемы.


      1. mayorovp
        11.08.2018 17:51

        Но ведь у internal классов тоже бывают публичные методы…


      1. Mikluho
        11.08.2018 18:48

        публичный класс DbBackupper с одним простым методом CreateBackup(string path), который делает резервную копию БД.
        Это плохой пример. Чтобы проверить бэкап нужен или инфраструктурный или интеграционный тест.
        Более реалистичный пример — метод, который данные из БД тащит. У него под капотом и подключение к БД, и инициализация команды/запроса, и трансформация данных, и может что-то ещё. При этом полезная нагрузка — это инициализация параметров запроса/команды и трансформация данных. Правильный подход для покрытия тестами — отсечь всю инфраструктурщину, переложив её на кастомный (если надо) адаптер, оставив в изначальном методе вызов трёх методов: PrepareQuery, GetData, TransformData. GetData тестируется интеграционно, а в модульных тестах заменяется на заглушку. Два других метода имеют вполне понятные контракты, которые покрываются тестами. Более того, при таком рефакторинга запросто можно вытащить кучу однотипных методов, которые сольются в один… и станет в коде чище :)


      1. lair
        11.08.2018 21:53

        Да очень просто быть: у интернал классов тоже есть "публичный" интерфейс, просто он "публичен" в рамках библиотеки. Дальше методика решения зависит от среды, в .net это решается InternalsVisibleTo.


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


  1. SergejSh
    12.08.2018 04:57

    1) Методики TDD и BDD предполагают что вы сами пишете код и в таком случае код получается тестируемый (как я понимаю в данном случае не так).
    2) Лично я всегда следую принципу с минимальными усилиями получить максимальный результат. Исходя из этого считаю допустимым в тесте через рефлексию установить значение приватного поля. Но это скорее исключение чем общее правило.


    1. VolCh
      12.08.2018 10:26

      2) Это должно быть не просто исключением, а документированным и обоснованным исключением. Как минимум в коде модуля пометить это приватное поле комментарием типа «used by reflection in tests»