Подобного рода фразы можно услышать от абсолютно разных людей: фанатиков разработки, продавцов различных утилит, пользователей удобных тулз. Слышит их и мой менеджер, когда мне хочется поэкспериментировать с чем-то новеньким.
Правда, инструкция к инструменту обычно не содержит раздел «Противопоказания», не указываются ситуации когда НЕ стоит применять утилиту. Между тем, подобный раздел мог бы сэкономить тонны времени на неудачные эксперименты.
Сегодня я пошвыряю камни в огород Code Coverage (CC). Достаточно полезная метрика, под которой лежат несколько скудно документированных граблей.
«Есть ложь, есть наглая ложь
есть статистика».
Создатели SonarQube это великолепно понимают, недаром у SonarQube с десяток CC метрик. Я буду перебивать статистику используя CC от DevExpress, там метрика лишь одна.
Проблема 1. Типичный тест не того. Протестируем метод с кучей проверок аргумента:
public static Hash Example1_IfThen(int arg)
{
if(arg == 0) { throw new ArgumentException("0 argument"); }
if (arg == 1) { throw new ArgumentException("1 argument"); }
if (arg == 2) { throw new ArgumentException("2 argument"); }
return new Hash(new Hash(arg + 42) + 13);
}
...
[TestMethod, ExpectedException(typeof(ArgumentException))]
public void Example1_IfThen_0()
{ Program.Example1_IfThen(0); }
[TestMethod, ExpectedException(typeof(ArgumentException))]
public void Example1_IfThen_2()
{ Program.Example1_IfThen(2); }
[TestMethod, ExpectedException(typeof(ArgumentException))]
public void Example1_IfThen_1()
{ Program.Example1_IfThen(1); }
Метод покрыт тестами на 83%, чего обычно достаточно для авто-билда. Технически спорить не о чем, большая часть кода покрыта тестами, но основной сценарий тестами не затронут. Тестами покрыта наиболее простая часть кода, не наиболее важная.
Проблема 2. Замеряем актуальный код, вместо необходимого.
public static int Example2_IncompleteLogic(IEnumerable<int> arg)
{
return arg
.Where(elem => elem != 2)
.Where(elem => elem != 3)
.Count();
}
...
[TestMethod]
public void OneTestToCoverAll()
{
// Arrange - collection with one element '4'
var arg = new List<int> { 4 };
// Act
var result = Program.Example2_IncompleteLogic(arg);
// Assert
Assert.AreEqual(1, result);
}
Тестируемый метод не содержит проверки на null аргумент, однако покрытие — 100%. Иногда люди забывают: Code Coverage — это метрика покрытия кода, не метрика закрытия требований; если в методе не достает логики (метод недостаточно сложен для решения своей задачи) — CC это не покажет.
100% покрытия не гарантируют работоспособности программы. Доводя до абсурда: пустой метод элементарно покрывается на 100%. Непустой метод покрывается на 100% тестами без Assert-ов.
Проблема 3. Оптимизм. Немного иное проявление предыдущей проблемы. Как видно, один тест покрывает 100% кода. Попробуем переписать наш метод, избавившись от LINQ (для улучшения производительности).
var result = 0;
foreach(var elem in arg)
{
if(elem == 2 || elem == 3) { continue; }
else { result++; }
}
return result;Получаем лишь 73% покрытия. Функциональность не изменилась, метрика упала. Мало того, что 100% покрытия не гарантируют работоспособности программы, эти 100% могут быть фейковыми. Вывод:
Побочное наблюдение: в данном случае мы можем просто иметь косяк в технической реализации, в теории анонимный метод elem => elem != 2 можно заменить на elem => if (elem != 2) return true else return false;, что пофиксит покрытие оригинального метода до 73%. Правда, такой подход потребует усложнения удобного сейчас UI.
Следствие: Используемый инструмент может не обладать всей желаемой функциональностью. Тривиальная вещь, не менее от того верная.
Проблема 4. Передача ответственности.
public static void MakeEverythingWell()
{
OuterLib.MakeEverythingWell();
} Метод покрывается на 100% одним тестом. При этом покрытие OuterLib библиотеки лежит на совести того, кто её добавил. Или обновил. Года три назад, до введения CC. До увольнения.
Приходится снова констатировать факт: мало того, что 100% покрытия не гарантируют работоспособности программы, эти 100% могут быть фейковыми.
Помимо чисто кодовых моментов есть несколько претензий именно к обработке результатов CC
Претензия 0, всем известная. 100% покрытия. Нет 100% покрытия — нет одобрения билда. Проблема в том, что первые проценты покрытия получить относительно просто, а вот последние… Особенно, когда часть кода генерируется. Или недостижима (поскольку создана для Васи, который будет её юзать через два дня). Или просто теоретически достижима, а пример подбирать\высчитывать пару недель (такое бывает при работе с математикой). Короче, большинство команд (из тех кто вообще интегрирует CC в CI) останавливаются на 60\70\80 процентах необходимого покрытия.
Претензия 1, спорная. Покрытие мертвого кода. На моей памяти схожая проблема особо ярко проявилась в ходе проверки Mirand-ы коллегами из PVS. Комментарии довольно эмоциональны, но часть споров касалась мертвого кода: часть найденных диагностик указывала на (заброшенные) плагины, но не на ядро.
Возникает вопрос: нужен ли CodeCoverage для мертвого кода? С одной стороны, мертвый код это проблема, и привлечение внимания к нему приветствуется. С другой стороны, мертвый код не влияет на продакшн, так стоит ли позволять ему влиять на CC метрику?
Претензия 2. Важность кода. Расширение проблемы 1. В моем проекте есть два примечательных контроллера: «оплата» и «переговорка». «Оплата» критична для клиента, и я вполне согласен с требованием «80% покрытия», «переговоркой» же пользуются 1.5 анонимуса. В год. И она не менялась уже два года. Вопрос: для чего писать тесты к полумертвой функциональности? Лишь для получения 80% бейджа одобрения автосборки?
Претензия 3, невозможная. Метрика как ачивка. Это когда никто не проверяет что именно покрыто. Помните байки про оплату за линии кода? Мне доводилось слышать про людей, которые творили ненужный кода для лучшего покрытия.
Претензия 4. Метрика «за бесплатно». Когда руководство скидывает требование «покрывайте код на 80%», и разработчики безропотно соглашаются. Проект при этом — одноразовый. Или прототип. Или дедлайн на носу. Или имеется здоровенный
Покрытие кода тестами требует времени! Если покрытие еще и замерять — время на тесты может и возрасти (хотя может и упасть). Так что если команда не успела сдать проект в срок, но зато достигла 80% покрытия — вина может поделиться между руководством и разработчиками. Вопрос линии раздела вины поднимать не стоит, ибо холивар.
Под конец. Еще раз замечу: СС — метрика полезная, хоть и с сюрпризами. Она реально помогает с контролем кода, если нет слепого стремления к цифрам в отчетах.
Комментарии (27)
alexeykuzmin0
10.01.2018 13:35Интересно, существуют ли тулзы для вычисления code coverage, которые могут игнорировать не-покрытие фрагмента кода, если рядом с ним есть сигнальный комментарий. Казалось бы, в таком случае правило «весь код должен быть покрыт на 100% либо иметь разумную причину быть не покрытым, выраженную в комментарии» решает большую часть проблем, указанных в статье.
imanushin
10.01.2018 13:55+1Обычно ведь покрытие тестами кода должно говорить не то, что код покрыт на 60%, а то, что в 40% кода тесты вообще не смогли зайти. Или по-другому: разработчик не способен написать такие интеграционные или модульные тесты, которые бы зашли в эти 40% (за приемлемое время).
То есть правильная характеристика — не покрытие кода, а скорее "мера непокрытого кода".
Ну и конечно — адекватность тестов должна проверяться отдельно на code review, тут уже никто не спорит. Накрутки тестов и покрытие должны исключаться и удаляться.
Oxoron Автор
10.01.2018 14:14Еще лучше: проверять адекватность тестов\CC еще в процессе разработки. Во время CodeReview проблемно проверить какие именно участки код покрыты.
Code Review позволяет прикинуть какие сценарии коммитер протестировал, но только прикинуть. CC инструмент же заодно подсвечивает те строки метода, которые не покрыты.
Несомненно, предварительный просмотр кода CC утилитой не отменяет CodeReview, однако и CodeReview предварительного просмотра полностью не заменит.
vba
10.01.2018 14:56Не забывайте что есть другие подходы, например Property Based Testing или Mutation Testing, которые друг друга дополняют и могут заменить классический ААА путь написания тестов. Но защиты от
сумасшедшего индусадурка нет.
schroeder
10.01.2018 15:03Любой процент ппокрытия не говорит вообще ничего, потому что эта метрика никак не связана с качеством тестов.
Пример:
if(a > 5){
…
} else {
…
}
Вы пишете тест где а = 10 и а = 3. Анализ покрытия скажет вам, что код протестен на 100%. Но говорит ли этот факт о том, что условие действительно корректно протестено? Конечно нет, ибо если вдруг поменяем условие на а >=5 тесты все еще будут зелеными, хотя поведение программы изменилось.
Поэтому можно говорить только о том, что покрытие в 70% наверное лутше чем в 50% и не более того.
Что бы улутшить ситуацию, надо тестить сами тесты. Например через mutation testing. На хабре есть статьи на эту тему, очень рекомендую.
RPG18
10.01.2018 15:21+1Анализ покрытия скажет вам, что код протестен на 100%
Это необразованный человек так интерпретирует результаты анализатора. Анализ покрытия говорит, что все ветки кода исполнялись хотя бы раз. Ничего более.
Oxoron Автор
10.01.2018 15:26Вы пишете тест где а = 10 и а = 3.
Наш код стоит тестировать примерами a=5 и a=4, как два граничных случая. Соответственно, появится красный тест при изменении условия на a >= 5.
Любой процент ппокрытия не говорит вообще ничего, потому что эта метрика никак не связана с качеством тестов.
Вы несколько категоричны. Метрика действительно может иметь расхождение с качеством приложения\тестов, однако она предоставляет первичную оценку, что уже неплохо. Иначе бы CC не использовали. Плюс, CC — это не только метрика. В случае .NET есть CC утилита, подсвечивающая посещенные\непосещенные тестами куски кода, что весьма помогает при написании тестов.
Что до мутационного тестирования — это (интересная) тема отдельных статей на хабре.
andy128k
10.01.2018 18:55+1Мутацинное тестирование (и/или фаззинг) это приятное дополнение к покрытию, а не замена.
RationalBot
10.01.2018 17:24+3Я бы посоветовал относиться к покрытию кода как температуре тела — 36.6 не говорит, что человек здоров, но большие отклонения от нормы обычно говорят о серьезных проблемах.
Нужно ли мерить покрытие кода авто-тестами? Мое мнение — обязательно. И в первую очередь не ради магической цифры, а для анализа результатов.
Да, как и любую другую метрику ее можно скомпрометировать.
Да, метрика является линейной — не может отличить полезный код от мало- или бесполезного. Но если фокусироваться на анализе, то смотреть можно на покрытие отдельных компонент или библиотек. При желании можно как-то разметить код и получить нелинейную метрику (примерно как раскладка багов по северити).
Да, метрика не показывает отсутствующие ветки кода или покрытие граничных условий. Примите как ограничение. Покрытие требований избавлено от этих недостатков? Вы все граничные условия прописываете в требования?
Я бы выделил следующие сильные стороны измерения и анализа покрытия кода по сравнению с покрытием требований:
1. тестовое покрытие можно мерить при отсутствии требований или других артефактов.
2. метрика инструментальная, т.е. ее получение обычно относительно недорого (в плане трудозатрат).
3. Т.ч. процесс измерений автоматизирован, то исключен человеческий фактор — труднее манипулировать результатом.
4. Т.к. процесс относительно дешев, то можно мерить часто и отслеживать тренды.
И если смотреть на тренды, а не на абсолютные цифры, то легко ответить на данные вопрос " Нужно ли покрывать мертый код?" — Нет, не нужно. Если он реально мертвый, то нужно выбросить. Если он редкоиспользуемый и мы не хотим его покрывать, но тренд все равно будет растущий (при условии, что новый код покрыт на достаточном уровне).
И опять же, любую полезную метрику можно превратить во вредный KPI, т.ч. я бы не стал на этом моменте фокусироваться. Гораздо интереснее сопоставлять сильные и слабые стороны разных метрик оценки тестового покрытия.
SamSol
11.01.2018 01:39СС — это технологическая метрика. Она не много говорит о качестве программного продукта. Зато кое-что говорит о качестве процесса его разработки!
SergeyEgorov
11.01.2018 08:34На мой взгляд создатели Agile-манифеста вполне исчерпывающе высказались на эту, и похожие темы.
ApeCoder
11.01.2018 09:39(поскольку создана для Васи, который будет её юзать через два дня).
А почему юнит тест не может это покрыть?
Oxoron Автор
11.01.2018 10:39Функциональность в приватном методе. Ни один из публичных конкретно эту ветку пока не использует.
Практика не самая лучшая, мало отличается от мертвого кода, но изредка допустима.
Иногда у меня в таких приватных методах зашиты исключения на тот случай, если кто-либо когда-либо попробует-таки использовать метод неправильно. То есть, все текущие методы используют приватный метод корректно, но в теории кто-то может и косякнуть в будущем. Также довольно спорная практика, однако в ядре приложения иногда спокойнее перестраховаться.ApeCoder
11.01.2018 10:59Если это большой метод, то почему нельзя его вынести в отдельный класс или написать test-specific descendant для тестирования или сделать публичным, если это маленький метод почему нельзя чтобы коллега его добавил при разработке.
Oxoron Автор
11.01.2018 11:52Вы во многом правы, технически протестировать можно. Правда, при этом мы можем получить другие проблемы. Например, получившийся тест с потомком будет иметь смысл в течении пары дней (если у нас сценарий с Васей через пару дней). Плюс, создание потомка не самая популярная стратегия тестирования. Наконец, подобный подход предполагает что мы тестируем деталь реализации, а не публичный контракт.
Остальные сценарии (перенести в класс, сделать публичным, оставить Васе) тоже имеют право на жизнь, однако могут привнести свои сложности.
В итоге мы получим некоторое количество геморроя лишь для «100% покрытия».
Возвращаясь к оригинальной фразе (часть кода… недостижима (поскольку создана для Васи, который будет её юзать через два дня).) — вы правы, с недостижимостью я косякнул.ApeCoder
11.01.2018 12:33Фактически вы разрабатваете что-то что используется Васей как отдельный модуль. То есть есть high cohension внутри этого метода и low coupling с Васиным кодом. Сам по себе факт того, что это надо это признак для рефакторинга. Тесты нужны и для того, чтобы выявлять такие моменты.
dididididi
11.01.2018 10:40Покрытие тестами — это метрика, того как на фирме относятся к качеству кода.
Наличие на предприятии директора по качеству, ОТК и сертификатов качества, еще не говорит, что продукт качественный(директор — разгильдяй, сертификаты липовые), но по крайней мере на этом заводе знают слово «качество»)))
goodnickoff
11.01.2018 11:59Для меня тесты — это не только показатель работоспособности кода, но и инструмент для написания более архитектурно грамотного, качественного кода. Следуя TDD проблемные места видны заранее, еще до того как был реализован класс.
По этой причине низкое покрытие тестами может быть сигналом о том, что присутствуют архитектурные проблемы. На практике я заметил, что практически всегда тот код, который нельзя/сложно протестировать — код низкого качества.
Поэтому я стараюсь достигать как можно больших значений CC, пытаясь не "читерить".
andy128k
Проблема №1 прекрасно иллюстрирует, что любая планка ниже 100% приводит к тому, что покрывается то что легче, а не то что нужно.
Есть ещё и такое мнение.
Oxoron Автор
Помечать код как «Не анализировать покрытие» — подход заслуживающий права на жизнь. К сожалению, им тоже можно злоупотребить. Доводя до абсурда — можно пометить весь код «неанализируемым».
Но статья отличная, спасибо за линк.
andy128k
Пометка будет иметь объяснение и коммитера.
Oxoron Автор
Несколько недостатков по-прежнему остается.
Например, 'commited by Vasya, «Will test later, deadline»'. Или проекты где анализ внедряется после 5 лет разработки: в наличии миллион строк кода, покрытие — 50%. Метить непокрытый код проблемно, придется реагировать на изменения.
Но в целом вариант очень даже интересный, с явными преимуществами перед 80% покрытия.