Тестировать только через public-методы плохо

    В программировании и в 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
      +4
      Э…
      Буду рад любым примерам того, как четкое следование данному принципу приводило к проблемам и снижению мотивации писать тесты у команды разработчиков.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

                  При нормальном подходе к тестам, зависимости в свою очередь должны быть покрыты тестами, так что дополнительные проверки им не нужны.
            0
            Можно тестировать непубличные методы в каких-то ситуациях, но при этом не за счёт снижения покрытия публичных. То есть в ситуации 2^3 тестов не заменяем их на 2•3, а дополняем, получая 14 тестов. Быстро при таком подходе приватные методы начинаешь тестировать только при реальной необходимости.
              0
              А если рассмотреть вариант с другой стороны:
              • есть метод, не являющийся методом интерфейса класса, но выполняющий нетривиальную логику.
              • покрываешь его тестами так, чтобы быть уверенным в его 100% работе в известных случаях.
              • в публичном методе есть логика вызова этого метода, которую ты покрываешь (и только)

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

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

                  Если у вас полностью покрыт публичный метод, и у вас есть правило покрывать все публичные методы. Непонятно зачем вообще в таком случае проверять приватный.
                    +1
                    Может иметь смысл покрывать приватный, чтобы ошибки в нём выявлялись раньше и лучше локализовались. Особенно, если приватный легкий и не требует тяжёлых зависимостей, а публичный может ио дергать или ещё что-то тяжелое.
                0
                рефлексия наша все в тестах
                  0
                  ну, только если «рефлексия» в смысле «опять не понятно почему этот юнит-тест падает»? ;)
                  +4

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

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

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


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

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


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

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

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

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

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

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

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

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

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

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

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


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

                                  0
                                  1) Методики TDD и BDD предполагают что вы сами пишете код и в таком случае код получается тестируемый (как я понимаю в данном случае не так).
                                  2) Лично я всегда следую принципу с минимальными усилиями получить максимальный результат. Исходя из этого считаю допустимым в тесте через рефлексию установить значение приватного поля. Но это скорее исключение чем общее правило.
                                    0
                                    2) Это должно быть не просто исключением, а документированным и обоснованным исключением. Как минимум в коде модуля пометить это приватное поле комментарием типа «used by reflection in tests»

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

                                  Самое читаемое