Detekt: как статический анализ помогает улучшить код автотестов14.12.2023 10:00

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

И у каждой ошибки есть своя цена. Было бы здорово, если бы о них можно было узнать:

на этапе локальной отладки и, соответственно, быстрее (например, запустив одну команду и получив отчёт) — движение в сторону Fail Fast и сокращения Feedback Loop;
не занимая ресурсы CI сборкой кода, который заведомо придётся исправлять, — Quality Gates;
снимая часть нагрузки с ревьюера и меньше переключая контекст специалистов;
работая с унифицированным кодом и не тратя время на обсуждение мелочей.

Это может касаться как простых ошибок, на которые не хочется тратить время специалистов, так и неочевидных ошибок, у которых иногда непросто определить причину.

Меня зовут Николай, и я инженер в мобильной платформенной команде Яндекс Еды. В этой статье я расскажу, как мы повышаем качество кода автотестов Android-приложения. И в этом нам помогает статический анализ.

Коробочные правила

Прежде всего, коротко расскажу про наш стек: это Kotlin, Kaspresso и Marathon. В качестве основного статического анализатора используем Detekt, который строится на паттерне Visitor и в процессе анализа исходного кода может инспектировать различные языковые конструкции Kotlin: например, функции, переменные и многие другие.

На Хабре уже есть статьи про первичную настройку Detekt (ссылки на них дам в конце), поэтому опустим этот этап и перейдём к рассмотрению нашего опыта.

Итак, в стандартной библиотеке Detekt много правил. Но я приведу те, которые, по моему мнению, могут быть полезны для кодовой базы автотестов:

LongMethod — длинные методы (тестовые). Если тело метода слишком большое, то это может означать, что потенциально его можно разделить на несколько более атомарных методов (тестовых). Например, конкретный тест будет проверять лишь одно состояние экрана приложения.
StringLiteralDuplication — дубликаты строк (тестовых данных). Мы не столь категоричны, как некоторые AQA, и не требуем полностью выносить строки тестовых данных из тестов. Однако как минимум можно вынести повторяющееся в константы private companion objectили общие Kotlin Object.
MagicNumber — «магические числа». Например, ваши тестовые данные приводят к отрисовке списка из множества элементов, и не всегда вам может подойти Kaspresso firstChild или lastChild. И если у вас такой случай, то вынос в приватную константу какого-нибудь индекса 19 повысит читаемость.
Ну и прочие правила, которые применимы как к продуктовому коду, так и к автотестам. Например, unused-параметры функций.

Стандартные правила неплохи, но они не покрывали частые проблемы из нашей кодовой базы автотестов. У нас были целые спринты по их исправлению. Например, мы думали, как починить длительное выполнение части тестов с периодическими простоями или flaky.

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

Примеры наших правил

У вас, конечно, может возникнуть желание покрыть правилами всё и сразу, но более удачным решением будет составить список известных проблем. Вы сможете найти их в комментариях к Pull Request и прочих артефактах совместной работы. В первую очередь можно покрыть то, что соответствует критерию «быстро и просто», а редко встречающиеся проблемы можно отложить до лучших времён.

В первой итерации такого подхода мы создали семь правил.

IsVisibleUsageRule

Предположим, что мы хотим проверить, отображается ли кнопка. Первая мысль, которая приходит в голову, — использовать говорящую проверкуisVisible, но не всё так просто. Здесь лучше подойдут isDisplayed или isCompletelyDisplayed. Эта неочевидная важная деталь описана в документации: «Unless you’re specifically targeting the visibility value with your test, use isDisplayed».

Также правильный выбор проверки оказывает влияние на стабильность за счёт Kaspresso flakySafety. Ведь значение свойства isVisible мы узнаём слишком быстро: в случае использования isVisible элемент будет в неподходящем состоянии, в сравнении с рекомендуемым isDisplayed. Поэтому и выбираем второй вариант.

// Не рекомендуем
step("Тапаем на кнопку") {
   SomeScreen {
       someButton {
           isVisible()
           click()
       }
   }
}

// Рекомендуем
step("Тапаем на кнопку") {
   SomeScreen {
       someButton {
           isDisplayed()
           click()
       }
   }

LargeScreenObjectRule

Большие Page Object поддерживать достаточно сложно. Например, экран, который содержит много логики и элементов, либо несколько экранов, которые содержат очень похожий повторяющийся элемент. Либо когда в приложении есть feature toggle и у части элементов может быть несколько реализаций с разными идентификаторами.

В таких случаях мы ограничиваем размер в пользу Page Element в составе Page Object. При этом можно не начинать с преждевременных оптимизаций, а переработать уже существующие большие Page Object.

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

Вероятно, вы не сможете с наскока переписать все большие Page Object, и вам может потребоваться baseline.xml, чтобы временно игнорировать нарушения (о нём я расскажу немного позже).

RestrictedKeywordRule

Тесты — вещь однозначная, поэтому в них не должно быть ветвлений. Именно поэтому мы запретили конструкции if и when на уровне теста. Если у вас возникает желание использовать их, то это может говорить о низкой testability в вашем приложении. То есть если у вас, скажем, по четвергам в приложении отображается некое всплывающее окно, которое может помешать прохождению общей группы автотестов, то нужно выключить его насовсем.

// Не рекомендуем
step("Тапаем на кнопку если показалась") {
   SomeScreen {
       try {
           someButton {
               isDisplayed()
               click()
           }
       } catch(e: NoMatchingViewException) { }
   }
}

// Рекомендуем
// повышать testability и не использовать ветвления в тестах

Также наличие конструкцийif–else может указывать на то, что тест можно разделить на несколько тестовых случаев.

А ещё неправильная обработка ошибок на уровне теста, Scenario и Page Object приводит к ожиданию до тайм‑аута (у нас это 20 секунд), поэтому теперь мы запрещаем try‑catch на этих уровнях. Это связано с тем, что Kaspresso flakySafety пытается вас подстраховать и повторить неудачное действие, если это возможно, но в данном случае вы могли злоупотребить этим через try‑catch.

TestClassNamingRule

Многим AQA известно, что название тестового класса в Junit4 должно содержать в конце Test или Tests, иначе могут возникнуть сложности с его запуском на разных этапах. Так исторически сложилось в индустрии, что в качестве конвенции используются паттерн **/*Test*.java и его аналоги.

// Не рекомендуем
class BottomSheet : BaseTestCase() { … }

// Рекомендуем
class BottomSheetTest : BaseTestCase() { … }

TestClassPrivateMemberRule

Если при создании теста вы копируете существующие фрагменты кода, то они могут начать ссылаться на константы тестов доноров не из private companion object. Если вы этого не заметите, то одни тесты начнут зависеть от других тестов, а это не очень хорошо.

Константы должны находиться внутри private companion object, чтобы их нельзя было использовать из других тестов. Также это избавляет от необходимости писать private для каждой константы. Если вам нужны общие константы, то следует выносить их в общие Kotlin Object или Kotlin Enum.

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

// Не рекомендуем
class SomeTests : BaseTestCase() {
   companion object {
       const val INDEX_OF_FIRST_SEPARATOR = 2
   }
}

// Рекомендуем
class SomeTests : BaseTestCase() {
   private companion object {
       const val INDEX_OF_FIRST_SEPARATOR = 2
   }
}

TestMethodNamingRule

Длина квалифицированного пути тестового метода должна быть не больше зафиксированного значения. С таким квалифицированным именем будет удобно работать в marathon-файле в allowlist и blocklist. В документации показан очень короткий пример квалифицированного пути, но в реальности он легко может заходить за IDEA Right Margin.

Название тестовой функции не должно содержать слово test, так как оно не несёт полезной нагрузки. Ещё оно должно состоять из нескольких слов, а также стоит описать в названии теста, что именно вы проверяете. Мой опыт подсказывает, что создание лаконичных названий не всегда даётся легко.

// Не рекомендуем
@Test
fun test() { … }

// Рекомендуем
@Test
fun checkFooterForAvailableState() { … }

TmsLinkAnnotationRule

Это правило специфично для нашего CI, но у вас может быть похожая ситуация. Мы связываем тест-кейс и класс автотестов через аннотацию @TmsLink. Если этого не сделать, то результаты автотестов не будут прикреплены скриптом к регрессионному тест-рану и принесут меньше пользы, чем могли бы.

// Недопустимо без @TmsLink для нашего процесса
class SomeTests : BaseTestCase() { … }

// Требуется аннотация нашим процессом
@TmsLink("testcases/1234")
class SomeTests : BaseTestCase() { … }

Также мы проверяем строковое содержимое аннотации через RegExp, а ещё то, что не используется очень похожая @TmsLinks, которая не предусмотрена нашим процессом. Вы можете написать похожее правило и проверить наличие лесенки аннотаций @Epic, @Feature и @Story, которая позволит вам создать иерархию автотестов в Allure Report.

От чего мы пока отказались

У нас было ещё несколько идей правил, которые не попали в первую итерацию. Но так или иначе они имеют право на существование. Возможно, эти мысли окажутся полезными для вашего проекта.

Запрет в тесте неявных ожиданий. Если вы не дожидаетесь нужного события явно, а просто ждёте Х секунд через Thread.sleep и Screen.idle, то очевидно, что такие автотесты займут больше времени. Актуальная проблема для автоматизации тестирования в целом, но в нашем проекте она не встречается.

Запрет больших JSON-моков. Если JSON-мок большой, то это может создавать определённые сложности. Чем больше в моке не используемых автотестами сущностей (например, на витрине магазина), тем дольше приложение будет отрисовывать их и, следовательно, дольше будут идти и сами тесты. Также структура файла в IDE будет анализироваться дольше.

Detekt — это анализатор кода на Kotlin. Проверка JSON или XML в нём не предусмотрена. Пока что отложили эту идею и, возможно, сделаем позже с помощью Android Lint.

Запрет наследования неподходящих классов. При описании Kaspresso‑экранов возможно использовать как базовый класс import io.github.kakaocup.kakao.screen.Screen, так и com.kaspersky.kaspresso.screens.KScreen. Отличие заключается в том, что KScreen требует переопределять layoutId и viewClass, которые могут выступать в роли документации — её наличие проверит компилятор. На данном этапе у нас нет проблем с использованием неподходящего класса, поэтому пока что это правило неактуально.

Kaspresso childWith. Выше я уточнил, как мы решали проблему повторяющихся ошибок в коде наших тестов с помощью правил. Отдельно можно упомянуть, что большинство моментов бездействия в тестах было связано с тем, что билдер Kaspresso childWith использовался неправильно или вёл себя непредсказуемо. Например, у нас были:

childWith без последующих проверок — false-positive-тесты с нашей стороны;
childWith пустой внутри — наша ошибка, ведь это билдер (хотя, возможно, такое стоит проверять в Kaspresso до старта тестов — так же, как, например, проверяются Item, не добавленные в KRecyclerView);
childWith в связке с doesNotExist — неожиданное поведение, которое иногда выражается в прохождении проверки с большой задержкой.

Отмечу, что проблема childWith многогранная и нам не удалось её покрыть правилом с ходу.

В то же время мы смогли описать в правиле статического анализа проблему рантайма, упомянутую ранее в главе про RestrictedKeywordRule и связанную с try–catch, что всё-таки оставляет надежду на покрытие правилом childWith в будущем.

В ближайшем будущем разработчики Detekt планируют выпустить версию 2.0, которая попробует более элегантно решить часть уже решённых нами проблем. Например, в рамках одного правила можно будет добавлять паттерн именования как для классов приложения, так и для тестовых. Также планируется уход от PSI. Кстати, именно поэтому на данном этапе мы не стали писать слишком много правил — только лишь те, которые были очень востребованы.

Как написать своё правило

Кратко расскажу про основные этапы пути, если вы решили написать своё правило для Detekt.

Шаг 1. Защитите идею перед пользователями правила. Если вы создаёте решение, которое будут использовать как минимум несколько десятков человек, то стоит обосновать его необходимость.

Шаг 2. Добавьте правило в detekt-config.yml и свой RuleSetProvider.

Шаг 3. Напишите первую наивную реализацию правила с использованием visit-методов. При необходимости напишите вспомогательный код. Например, если вам потребовался код для домена тестов, то он может быть таким:

private const val TEST_ANNOTATION = "Test"

fun KtElement.inTestMethod(): Boolean = getNonStrictParentOfType()?.isTestMethod() ?: false

fun KtNamedFunction.isTestMethod(): Boolean = annotationEntries.any { it.shortName?.toString() == TEST_ANNOTATION }

Шаг 4. Напишите юнит-тесты к правилу. Здесь получается интересная картина: юнит-тесты тестируют правило, правило проверяет код автотестов, автотесты тестируют приложение.

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

После массового исправления нарушений юнит-тесты продолжат покрывать основные случаи. В том числе они будут служить своего рода документацией. Этому также будет способствовать хранение тестовых данных юнит-тестов в виде raw strings с подсветкой синтаксиса через IDE language injection.

Юнит-тест для нашего правила, который использует фичу IDE language injections и достаточно прост для понимания

Шаг 5. Исправьте все нарушения в проекте в соответствии с «правилом бойскаутов». Этот принцип программирования гласит: «Всегда оставляйте код, с которым вы работаете, в более чистом состоянии, чем он был до вас».

Шаг 6. Доработайте наивную реализацию правила с учётом нарушений из проекта и юнит-тестов для этого правила.

Если рассмотреть пример несложного правила, опустив детали с его подключением, то оно может быть таким:

class IsVisibleUsageRule(config: Config) : Rule(config) {

   override val issue = Issue(
       id = javaClass.simpleName,
       severity = Severity.Defect,
       description = "In general, 'isVisible' should not be used",
       debt = Debt.FIVE_MINS,
   )

   override fun visitCallExpression(expression: KtCallExpression) {
       super.visitCallExpression(expression)

       val isVisibleAssertion = expression.getCalleeExpressionIfAny()?.text?.equals("isVisible") ?: false
       if (isVisibleAssertion) {
           report(
               CodeSmell(
                   issue,
                   Entity.from(expression),
                   "In general, 'isVisible' should not be used (use isDisplayed)")
           )
       }
   }
}

Issue содержит параметры для отчёта, а у visit-функции есть доступ к исходному коду — она описывает проверку.

А так выглядит конфигурация правила в detekt-config.yml.

IsVisibleUsageRule:
 active: true
 includes: "**/androidTest/**"

Способы игнорирования нарушений

Если вы внедряете статический анализатор в существующий проект, то на старте вы можете получить десятки, сотни, а то и тысячи нарушений, которые предстоит проработать вашей команде. Некоторые проблемы могут не умещаться в рамки одного Pull Request, поэтому вам понадобятся инструменты для работы с таким техдолгом. Что же нам может предложить Detekt?:

baseline.xml, упомянутый мной ранее. Он нужен для тех нарушений, с которыми надо разобраться в ближайшее время. Генерируется gradle-задачей на основе текущего состояния кодовой базы.
Suppress-аннотации. Они нужны для тех нарушений, с которыми вы собираетесь жить длительное время. Нежелательны, потому что лучше либо исправить нарушение, либо предусмотреть частный случай на уровне правила. При этом если код правила недоступен вам для изменения или у попавшего под правило кода очень неочевидное поведение, то аннотация пригодится. И в том числе будет выступать маркером потенциально проблемных мест.
include/exclude-паттерны файлов в detekt-config.yml. Важный этап, ведь использование PSI для потенциальных сотен тысяч Kotlin-файлов вашей кодовой базы не бесплатное по времени: чем меньше файлов будет обходить анализ, тем быстрее будут закончены все проверки.

Отчётность

Detekt поддерживает отчёты в разных форматах. Вы можете прочитать отчёт программно и открыть блокирующий Issue со списком нарушений в Pull Request. Нам оказалась не нужна вся мощь формата Sarif, и на все проблемы мы открываем один issue из списка нарушений в формате[$ruleId]$uri:${startLine}:${startColumn}. Используя этот формат, можно перейти в IDE на конкретную строчку кода с нарушением.

Конечно, в нашем решении приходится копипастить reference, но такая проблема решена в коробочных продуктах. Например, в Qodana — одном из многих полезных инструментов, созданных в JetBrains.

В каждом из сводных отчётов перечисляются reference для перехода к конкретному месту с нарушением

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

Но при этом важно, чтобы ограничения приносили больше пользы, чем неудобств. Я считаю, что в таком случае уместно лишь незначительное снижение «качества жизни» разработчика, иначе новые инструменты будут вызывать только головную боль и неприязнь.

В качестве бонуса прикладываю GitHub-репозиторий с примерами наших правил. Если кого-то заинтересует библиотека из Maven Central, то оставьте комментарий — мы её опубликуем.