Идея, лежащия в основе принципа, звучит хорошо: тестировать нужно поведение, а не реализацию. А значит, тестировать нужно только интерфейс класса. На практике это не всегда так. Чтобы представить суть проблемы представьте, что у вас есть метод, рассчитывающий стоимость работников занятых на сменной работе. Это нетривиальная задача, если речь идет о сменной работе, т.к. у них есть чаевые, бонусы, выходные, праздники, корпоративные правила и пр. и пр. Этот метод внутри выполняет очень много операций и использует другие сервисы, которые ему дают информацию о праздниках, чаевых и пр. Т.о. при написании для него юнит-теста необходимо создать конфигурацию для всех используемых сервисов, если проверяемый код находится где-то в конце метода. При этом, сам проверяемый код может использовать только частично, или вообще не использовать конфигурируемые сервисы. И есть уже некоторые юнит-тесты, написанные подобным образом.
Минус 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 из них существенные.
- 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)
Beyondtheclouds
11.08.2018 02:39+5Это очень вредный подход.
При тестировании приватных методов, тестируются не контракты, а конкретная реализация.
При изменении внутренней структуры класса(например пару методов удалили, другую пару объединили) не будет никакой возможности узнать не нарушились ли внешние контракты, так как тесты все равно упадут, и все придется внимательно проверять руками.
А проблема с копипастом может решаться хэлпером для создания минимальной дефолтной конфигурации.ETman Автор
11.08.2018 11:11Конкретная реализация может быть сложной и приводить к большому усложнению тестирования публичных методов. Если вы стараетесь не изменять существующие классы, а больше руководтствоваться OCP, то проблема, о которой вы пишете, резко снизится. «При изменеии внутренней структуры» тесты далжны быть легко выкинуты и написаны заного. В этом и состоит задача тестов и TDD: быть простыми и легкими, как в понимании, так и в обращении.
Хелперы — это палка с двумя концами. Один помогает, другой бьет по башке. Когда у вас есть лес классов из хелперов и непонятно, какой в вашем случае подходит, вы просто напишете новый. В итоге где-то в тестах что-то начинает валиться и вы тратите время на долгий и нужный дебаг тестов. Что опять, на мой взгляд, не соответвует идее TDD.
Но я не говорю тут об интеграционных тестах.Beyondtheclouds
11.08.2018 11:42>«При изменеии внутренней структуры» тесты далжны быть легко выкинуты и написаны заного.
непонятна цель написания тестов в таком случае. При измении кода они не помогут вам найти ошибку.
>Хелперы — это палка с двумя концами. Один помогает, другой бьет по башке.
Проблема не в хелперах, а в — «Когда у вас есть лес классов из хелперов и непонятно, какой в вашем случае подходит, вы просто напишете новый.»
Возможно вам стоит начать с наведения порядка в проекте. Думаю что много аспектов станет проще, не только тестирование.
ETman Автор
11.08.2018 11:46Цель — обеспечить качество кода, когда изменения делаются не в самом тестируемом классе, а в используемых им других классах.
То, что вы описываете с хелперами, это как написание еще одного продакшн кода. Потом понадоятся тесты для хелперов, чтобы проверить, что они правильно работают.Beyondtheclouds
11.08.2018 12:38>Цель — обеспечить качество кода, когда изменения делаются не в самом тестируемом классе, а в используемых им других классах.
1. У вас получается что качество кода «используемых им других классов.» обеспечивается тестами написанными у тех кто от этих классов зависит. Это достаточно странная логика.
2. На автономные классы без зависимостей по такой логике тесты можно не писать.
При нормальном подходе к тестам, зависимости в свою очередь должны быть покрыты тестами, так что дополнительные проверки им не нужны.
VolCh
11.08.2018 10:18Можно тестировать непубличные методы в каких-то ситуациях, но при этом не за счёт снижения покрытия публичных. То есть в ситуации 2^3 тестов не заменяем их на 2•3, а дополняем, получая 14 тестов. Быстро при таком подходе приватные методы начинаешь тестировать только при реальной необходимости.
ETman Автор
11.08.2018 11:03А если рассмотреть вариант с другой стороны:
- есть метод, не являющийся методом интерфейса класса, но выполняющий нетривиальную логику.
- покрываешь его тестами так, чтобы быть уверенным в его 100% работе в известных случаях.
- в публичном методе есть логика вызова этого метода, которую ты покрываешь (и только)
Знаете технику Sprout Method? Это оно и есть.
Ваш основной public-метод генерирует множество входящих значений параметров для непубличного метода. Вы проверятете не каждое значение, а то, что это множество правильно передается в непубличный метод. А непубличный метод вы проверяете тпо всему множеству передаваемых значений (по каждому важному). В итоге вам не надо писать 8 тестов для непубличного метода и 8 тестов для публичного. Для публично, в зависимости от логики, может оказаться достаточным, например, только 2.Mikluho
11.08.2018 11:43есть метод, не являющийся методом интерфейса класса, но выполняющий нетривиальную логику. покрываешь его тестами так, чтобы быть уверенным в его 100% работе в известных случаях.
А если этот метод сделать публичным или превратить в прокси к публичному классу/методу — то и заморочки с непубличностью пропадут. У вас снова в тестах будут только публичные методы :)
Кстати, по вашей ссылке «Sprout Method» как раз про это.
Иначе говоря — не надо тестировать приватное. Надо сделать публичным и тогда тестировать.
Beyondtheclouds
11.08.2018 11:45>В итоге вам не надо писать 8 тестов для непубличного метода и 8 тестов для публичного.
Если у вас полностью покрыт публичный метод, и у вас есть правило покрывать все публичные методы. Непонятно зачем вообще в таком случае проверять приватный.VolCh
11.08.2018 15:03Может иметь смысл покрывать приватный, чтобы ошибки в нём выявлялись раньше и лучше локализовались. Особенно, если приватный легкий и не требует тяжёлых зависимостей, а публичный может ио дергать или ещё что-то тяжелое.
AntonLozovskiy
11.08.2018 10:51рефлексия наша все в тестах
ETman Автор
11.08.2018 10:53ну, только если «рефлексия» в смысле «опять не понятно почему этот юнит-тест падает»? ;)
aamonster
11.08.2018 11:05+2Теоретически, если вам понадобилось тестировать "промежуточную точку", дёргая приватный метод — было бы неплохо разрезать класс по этой промежуточной точке, тестируя публичные интерфейсы обрезков.
DmitriyDev
11.08.2018 11:46Похоже возникло недопонимание с тестами.
Разработчик должен писать тесты не потому что этого хочет другой разработчик/менеджер, а потому что это надо разработчику. Все сы люди, все мы человеки. Мы пока ещё часто ошибаемся и у нас не идеальная память. Если мне через пол года надо будет править код, то тесты мне помогут с примерами использования и с большой долей вероятности защитят от ошибки.
ИМХО, считаю что не надо заставлять писать тесты, а надо тбъяснять зачем это и показывать пример.
И да, в моём понимании юнит тесты должны быть максимально глупыми. (Обычно в тестах забываю про наследования, дженерики… и тупо максимально всё забиваю ручками)
lair
11.08.2018 11:46+3Этот метод внутри выполняет очень много операций и использует другие сервисы, которые ему дают информацию о праздниках, чаевых и пр.
А корень проблемы-то — вот. Вы сделали переусложненный метод, и вместо того, чтобы среагировать на переусложненные тесты как на симптом сложного кода и разобраться с кодом, ищете обходные пути.
Данный метод требует 10 проверок, чтобы покрыть все случаи, 8 из них существенные. [...] При написании юнит-тестов для проверки этой логики, разработчику придется написать не меньше 8 больших юнит-тестов.
Это еще почему?
Я сталкивался со случаями, когда конфигурация юнит-теста занимала 50+ строк кода, при 4 строках непосредственного вызова. Т.е. только, примерно, 10% кода несёт полезную нагрузку.
Простите, а что в тестировании этого метода — конфигурация, а что — полезная нагрузка?
avost
11.08.2018 15:07+1А корень проблемы-то — вот. Вы сделали переусложненный метод, и вместо того, чтобы среагировать на переусложненные тесты как на симптом сложного кода и разобраться с кодом, ищете обходные пути
Ноп. Проблема в том, что код, как положено, разбит на простые методы и есть метод, который всё собирает. Этот метод публичный. Простые методы — приватные. Итоговая логика объективно сложная и чтобы протестировать отдельные части, реализуемые отдельными приватными методами надо изрядно извернуться. Это не проблема кода, это именно проблема и ограничение юнит-тестирования. Согласно канону, конечно можно всё растащить по утилитным классам, но это усложнение программы только ради следования канонической теореме тестирования. По-моему, это как раз то место, где следование канону несёт один сплошной вред и никакой пользы.
Вообще, лично мне кажется разумным было бы ввести в язык специальную аннотацию, помечающую метод как, вроде бы приватный, но, в то же время, открытый для тестирования. Пока для таких случаев некоторые используют пустую аннотацию, вроде @TheReasonTheMethodISPublikIsOnlyForTesting :). Костыль, конечно, но что делать. Рефлексия — ещё более кривой костыль.VolCh
11.08.2018 15:18> чтобы протестировать отдельные части, реализуемые отдельными приватными методами надо изрядно извернуться.
Обычно это говорит ещё и том, что извернуться надо будет, чтобы и просто штатно использовать этот публичный метод.
lair
11.08.2018 21:51+1Это не проблема кода, это именно проблема и ограничение юнит-тестирования.
Неа. Юнит-тестирование вам не запрещает это делать (более того, в каком-то смысле — стимулирует, потому что приватный метод — тоже юнит). Природа рекомендации "тестируйте публичный интерфейс" в другом — этот подход требует меньше переписывания при рефакторинге. Внутреннее разбиение метода на приватные методы — детали его реализации, и может меняться раз в десять минут. Каждый раз тесты переписывать? А откуда брать критерии на каждый метод?
xyzuvw
11.08.2018 16:23+1Вы сделали переусложненный метод, и вместо того, чтобы среагировать на переусложненные тесты как на симптом сложного кода и разобраться с кодом, ищете обходные пути.
Насколько я понял, автор имел в виду не переусложненный код, а простой код, задействующий большое число internal-классов, которые не хочется выносить в публичный API. Например, у вас есть библиотека, которая содержит один-единственный публичный класс DbBackupper с одним простым методом CreateBackup(string path), который делает резервную копию БД. Для реализации этого метода в библиотеке есть куча internal-классов, про которые пользователь знать не должен, и поэтому эти классы объявлены internal. Как быть?
Mikluho
11.08.2018 18:48публичный класс DbBackupper с одним простым методом CreateBackup(string path), который делает резервную копию БД.
Это плохой пример. Чтобы проверить бэкап нужен или инфраструктурный или интеграционный тест.
Более реалистичный пример — метод, который данные из БД тащит. У него под капотом и подключение к БД, и инициализация команды/запроса, и трансформация данных, и может что-то ещё. При этом полезная нагрузка — это инициализация параметров запроса/команды и трансформация данных. Правильный подход для покрытия тестами — отсечь всю инфраструктурщину, переложив её на кастомный (если надо) адаптер, оставив в изначальном методе вызов трёх методов: PrepareQuery, GetData, TransformData. GetData тестируется интеграционно, а в модульных тестах заменяется на заглушку. Два других метода имеют вполне понятные контракты, которые покрываются тестами. Более того, при таком рефакторинга запросто можно вытащить кучу однотипных методов, которые сольются в один… и станет в коде чище :)
lair
11.08.2018 21:53Да очень просто быть: у интернал классов тоже есть "публичный" интерфейс, просто он "публичен" в рамках библиотеки. Дальше методика решения зависит от среды, в .net это решается
InternalsVisibleTo.
А этот "единственный публичный класс с одним простым методом" нельзя протестировать юнит-тестом, если у вас нет контроля за внутренностями библиотеки, только интеграционными (и это тоже надо делать, отдельно от юнит-тестов).
SergejSh
12.08.2018 04:571) Методики TDD и BDD предполагают что вы сами пишете код и в таком случае код получается тестируемый (как я понимаю в данном случае не так).
2) Лично я всегда следую принципу с минимальными усилиями получить максимальный результат. Исходя из этого считаю допустимым в тесте через рефлексию установить значение приватного поля. Но это скорее исключение чем общее правило.VolCh
12.08.2018 10:262) Это должно быть не просто исключением, а документированным и обоснованным исключением. Как минимум в коде модуля пометить это приватное поле комментарием типа «used by reflection in tests»
Mikluho
Э…
Т.е. вы не только противоречите общепринятым нормам, но ещё и просите у общественности помощи в свержении норм???
Есть многое, что вам можно ответить на этот призыв… Но, пожалуй, хватит одного: «чёткое следование принципам» нередко оказывается итальянской забастовкой :)
Иначе говоря, цель должна определять необходимый инструментарий. Если цель у вас неверная — то и традиционный инструмент будет казаться кривым.
А касательно именно тестов — у вас проблема в другом месте. Вы имеете
blackmagic box, который в дебрях своих что-то там делает. Такое не тестируют юнит-тестами. Первое, что нужно сделать, так это опубликовать поведение этого ящика. Должно получится некоторое количество открытых контрактов, которые вместе реализуют бизнес-задачу. Да, если у там и правда сложнозависимый алгоритм — под каждый сценарий придётся писать конфигурацию. Но философия TDD в том, что эта конфигурация есть описание предусловий сценария, а результат теста есть соответствие поведения ожиданиям. И если уж совсем следовать канонам — набор тестов есть ТЗ. Меняются требования — меняется ТЗ — меняются тесты — корректируется код.И это если не говорить о том, что бы у вас был TDD, описанная проблема была бы совсем невозможна…
Да, легаси без тестов несовместимо с TDD. Даже просто unit-тесты прикрутить — надо постараться, но оно того порой стоит. А если не стоит — не надо ругать TDDЮ, надо выбирать подходящий инструмент — например интеграционные автотесты.
ETman Автор
Я говорю, что принцип хороший, но при чрезмерно педантном отношении к нему, может приводить к проблемам.
В случае с legacy-кодом есть проблема с написанием юнит-тестов, связанная с конфигурацией окружения. Я согласен, что полезно иметь такой тест, в котором будет понятно, как, собственно, использовать этот метод. При создании такого теста получается, своего рода, и описание работы модуля.
Но когда у вас есть 10 таких тестов, и 5 из них падает с NullRef exception, в том месте, где не должно этого было быть, вам остается только сидеть и дебажить сами тесты. Это в корне, на мой взгляд, не соответствует идее TDD, согласно которой тесты должны быть простыми и ясными.
Mikluho
Как я уже написал, TDD и legacy не совместимы в стартовой позиции, ибо в TDD тесты бы уже были. Их нет — у вас другая проблема. Для её решения есть другие методологии. Если кратко — рефакторинг вас спасёт.
ZelAnton
Чтобы такого не происходило надо писать тесты к тестам