Качеством кода в тестах часто пренебрегают. Когда в совместной разработке участвуют десятки QA-инженеров, возникает острая необходимость ввести формализованные правила, чтобы все могли быстро ориентироваться в тестовом проекте. К тому же часто тесты пишутся по аналогии или копируются с небольшими изменениями. Когда счет тестов идет на тысячи, то код, написанный в плохом стиле, быстро распространяется. Для решения этих проблем в тестовом проекте Wrike мы уже больше двух лет используем связку инструментов PMD и Checkstyle. И она отлично работает. В этой статье хотим поделиться опытом по настройке этих инструментов, их использованию и кастомизации.
В статье мы рассматриваем инструменты для контроля качества кода в тестовом проекте, который написан на Java с помощью фреймворка JUnit. В качестве системы хранения версий мы используем Git, а для сборки — Maven.
Для команды QA Automation в Wrike проблема чистоты кода очень актуальна: в нашем тестовом проекте более 30 тысяч тестов, а контрибьютят в него более 70 человек.
Наиболее частые проблемы, с которыми мы сталкиваемся:
Неиспользуемые или нежелательные импорты, переменные и private-методы.
Большая длина строки и большой размер файла.
Названия сущностей не в соответствии с договоренностями.
Проблемы стилистического характера: пустые строки, несколько выражений на одной строке, отсутствие пробелов и т.п.
А еще есть проблемы, которые связаны со спецификой тестовых проектов:
Неправильная разметка Epic/Feature/Story тестов.
Необходимость проставления тегов некоторым группам тестов.
Поддержка уникальности ID тестовых сценариев.
Для поддержания чистоты кода в компаниях часто используется практика ревью кода перед мержем ветки в мастер. Такой подход работает хорошо, но ревьюер — это человек, а человек может ошибиться. Хочется, чтобы на ревью приходил код без пропущенных пробелов, неправильно названных переменных и других подобных недостатков.
Практически все современные IDE поддерживают механизм инспекций в среде разработки без подключения внешних инструментов. Инструмент инспекций в популярной Intellij IDEA удобен для разработки, но достаточно тяжеловесный для организации процессов CI, поэтому этот вариант для нас не подошел. Также мы хотели сделать универсальный инструмент для всех разработчиков, а не только тех, которые используют Intellij IDEA.
Проверять практически все стилистические проблемы в коде нам в Wrike помогает инструмент Checkstyle. Но он не позволяет проверить логические кейсы — проставление аннотаций тестам, которые соответствуют определенным требованиям, проверка на неиспользуемые сущности в коде и т.д.
Для проверки логических ошибок мы используем статический анализатор кода PMD. Он позволяет проверять наиболее популярные кейсы, а также писать свои кастомные правила для любых проверок внутри файла. Но у него тоже есть недостаток — PMD может производить проверку файла только независимо от других файлов. Этого достаточно для большинства проверок, но мы не можем, например, проверить неиспользуемые public-методы, как в IDE.
Как подключить PMD и Checkstyle
Существует несколько способов работы с PMD. Мы используем PMD Java API. Хоть этот способ не самый простой для подключения, он позволяет наиболее гибко настраивать проверки и обрабатывать результаты. Тонкости настройки PMD можно прочитать в мануале.
PMD принимает список файлов для проверки и конфигурацию, в которой указаны нужные правила, и возвращает отчет в виде класса Report. Отчет можно вывести в консоль в читабельном виде.
Список правил для проверки хранится в XML-файле, туда же можно добавлять кастомные правила.
<?xml version="1.0"?>
<ruleset name="Custom Ruleset of standard checks"
xmlns="http://pmd.sourceforge.net/ruleset/2.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://pmd.sourceforge.net/ruleset/2.0.0 http://pmd.sourceforge.net/ruleset_2_0_0.xsd">
<description>
Ruleset of standard checks.
</description>
<exclude-pattern>.*/com/wrike/codestyle/.*</exclude-pattern>
<rule ref="category/java/codestyle.xml">
<exclude name="LocalVariableCouldBeFinal"/>
...
</rule>
...
<rule ref="category/java/codestyle.xml/FieldNamingConventions">
<properties>
<property name="enumConstantPattern" value="[A-Z][a-zA-Z_0-9]*"/>
</properties>
</rule>
</ruleset>Checkstyle, как и PMD, подключается через Maven:
<dependency>
<groupId>com.puppycrawl.tools</groupId>
<artifactId>checkstyle</artifactId>
<version>8.41</version>
</dependency>Для запуска проверки необходимо передать в Main список файлов для проверки и путь к файлу для отчета. После выполнения файл отчета можно проанализировать. Если Checkstyle находит ошибки, он завершает исполняемую программу с кодом ошибки 1. В этом случае мы не сможем проанализировать файл отчета. Чтобы избежать этого, перед выполнением Checkstyle нужно переопределить SecurityManager таким образом, чтобы программа не заканчивала работу, а «бросала» исключение. Если исключение было «брошено», будем выводить ошибки из файла отчета в консоль.
Выглядит это так:
private void checkCodeStyle(Set<File> fileSet) {
System.setSecurityManager(new SecurityManager() {
@Override
public void checkPermission(Permission perm) {
/* Allow everything.*/
}
@Override
public void checkExit(int status) {
/* Don't allow exit with failed status code.*/
if (status > 0) {
throw new CodeStyleViolationException();
}
}
});
try {
checkFilesWithCheckStyle(fileSet);
} catch (CodeStyleViolationException e) {
printViolation();
}
}
Список правил в Checkstyle также представляет собой XML-файл:
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
"-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
"https://checkstyle.org/dtds/configuration_1_3.dtd">
<module name="Checker">
<property name="charset" value="UTF-8"/>
<property name="severity" value="error"/>
<module name="SuppressWarningsFilter"/>
...
<module name="MethodName">
<property name="format" value="^[a-z][a-zA-Z0-9_]*$"/>
<message key="name.invalidPattern"
value="Method name ''{0}'' must match pattern ''{1}''."/>
</module>
...
<module name="SuppressWarningsHolder"/>
</module>Список файлов, которые исключаются из проверки, хранится в отдельном XML-файле:
<?xml version="1.0"?>
<!DOCTYPE suppressions PUBLIC
"-//Checkstyle//DTD SuppressionFilter Configuration 1.2//EN"
"https://checkstyle.org/dtds/suppressions_1_2.dtd">
<suppressions>
<suppress files="[/\\]\.idea[/\\].*" checks=".*"/>
<suppress files="src[/\\]main[/\\]resources[/\\]checkstyle[/\\]" checks=".*"/>
</suppressions>После того, как обе утилиты сконфигурированы, необходимо определить, какие файлы проверять. Не хочется прогонять проверки Checkstyle и PMD по всему проекту, если изменились всего несколько файлов. Для решения этой проблемы мы используем команду git diff --name-only origin/master. С ее помощью мы получаем список файлов для проверки, которые были изменены в этой ветке (в сравнении с мастером). Предварительно выполняется команда git fetch, которая «подтягивает» все изменения.
Чтобы не настраивать PMD и Checkstyle самостоятельно, вы можете воспользоваться готовым проектом. В нем уже настроены все базовые проверки и кастомные правила, описанные в этой статье.
Для данного класса:
public class CommonTest {
@Test
public void test() {
if (true) {
;
}
}
}
Вывод проверок Checkstyle и PMD выглядит так:
Вывод проверок
2021-07-20 15:24:41,815 [main] ERROR com.wrike.codestyle.PMDTest -
Problems with PMD, found [2] violation(s)
2021-07-20 15:24:41,815 [main] ERROR com.wrike.codestyle.PMDTest - [
/src/test/java/com/wrike/tests/CommonTest.java
.(CommonTest.java:9)
at line `9` and column `13`
Do not use if statements that are always true or always false
UnconditionalIfStatement[
public class Foo {
public void close() {
if (true) { // fixed conditional, not recommended
// ...
}
}
}
]
check in https://pmd.github.io/pmd-6.33.0/pmd_rules_java_errorprone.html#unconditionalifstatement
-----------------------------------------------------------------------------
/src/test/java/com/wrike/tests/CommonTest.java
.(CommonTest.java:10)
at line `10` and column `13`
An empty statement (semicolon) not part of a loop
EmptyStatementNotInLoop[
public void doit() {
// this is probably not what you meant to do
;
// the extra semicolon here this is not necessary
System.out.println("look at the extra semicolon");;
}
]`
check in https://pmd.github.io/pmd-6.33.0/pmd_rules_java_errorprone.html#emptystatementnotinloop
-----------------------------------------------------------------------------
]
com.wrike.codestyle.PMDTest$PMDViolationException
...
2021-07-20 15:24:41,829 [main] INFO com.wrike.codestyle.CheckCodeStyleUtils -
List of 1 files for checking is:
/src/test/java/com/wrike/tests/CommonTest.java
Checkstyle ends with 1 errors.
2021-07-20 15:24:42,416 [main] ERROR com.wrike.codestyle.CheckStyleTest - [
/src/test/java/com/wrike/tests/CommonTest.java:7:5
.(CommonTest.java:7)
'METHOD_DEF' has more than 1 empty lines after. [EmptyLineSeparator]
----------------------------------------]
2021-07-20 15:24:42,417 [main] INFO com.wrike.codestyle.CheckStyleTest - 2021-07-20 15:24:42,417 [main] INFO com.wrike.codestyle.CheckStyleTest -
com.wrike.codestyle.CheckStyleTest$CodeStyleViolationException: Your code have [1] violation(s)! Please fix issue. Check logs for more information...
Как настроить кастомные правила
С помощью стандартных правил PMD и Checkstyle можно проверить не все кейсы. Напишем несколько кастомных правил для PMD и подключим их к уже существующим проверкам. Для написания кастомных правил мы советуем ознакомиться с документацией PMD по AST-классам и кастомным правилам.
Уникальность тестов. Для хранения истории прогонов тестов мы используем Allure Server. Чтобы однозначно связать историю теста с конкретным тест-кейсом, мы используем собственную аннотацию @TestCaseId. Allure Server собирает историю теста, основываясь на его ID. В качестве уникального идентификатора можно было бы использовать имя теста, но оно может меняться, а ID — нет.
Для проверки уникальности теста и наличия у него ID мы используем два кастомных правила: CheckDuplicateTestIdRule и TestMustHaveTestIdAnnotationRule.
Правило TestMustHaveTestIdAnnotationRule проверяет наличие ID у каждого теста. Если у метода есть аннотации @Test или @ParameterizedTest из JUnit5, то он должен также иметь аннотацию @TestCaseId:
public class TestMustHaveTestIdAnnotationRule extends AbstractJavaRule {
@Override
public Object visit(final ASTMethodDeclaration node, final Object data) {
ASTAnnotation testAnnotation = node.getAnnotation(Test.class.getCanonicalName());
ASTAnnotation parameterizedTestAnnotation = node.getAnnotation(ParameterizedTest.class.getCanonicalName());
if (testAnnotation != null || parameterizedTestAnnotation != null) {
ASTAnnotation testCaseIdAnnotation = node.getAnnotation(TestCaseId.class.getCanonicalName());
if (testCaseIdAnnotation == null) {
addViolation(data, node);
}
}
return super.visit(node, data);
}
}Правило CheckDuplicateTestIdRule проверяет, что ID во всем проекте не дублируются. Каждый метод проверяется отдельно, чтобы в отчете о проверке можно было вывести, в каком методе ошибка. Метод findAllIds ищет все ID один раз для всего правила.
public class CheckDuplicateTestIdRule extends AbstractJavaRule {
@Override
public Object visit(final ASTMethodDeclaration node, final Object data) {
ASTAnnotation testCaseIdAnnotation = node.getAnnotation(TestCaseId.class.getCanonicalName());
if (testCaseIdAnnotation != null) {
Map<Integer, Integer> allIds = DuplicateTestCaseIdUtils.findAllIds();
ASTSingleMemberAnnotation memberAnnotation = (ASTSingleMemberAnnotation) testCaseIdAnnotation.getChild(0);
memberAnnotation.findDescendantsOfType(ASTLiteral.class).stream()
.map(ASTLiteral::getValueAsInt)
.filter(it -> allIds.get(it) > 1)
.forEach(it -> addViolation(data, node, it.toString()));
}
return super.visit(node, data);
}
}Еще одно правило для тестов с ID — проверка количества ID для параметризованных тестов ParametrizedTestIdsCountRule. Количество ID должно быть строго равно количеству передаваемых наборов параметров, чтобы тесты не «терялись» и попадали в отчеты Allure. Для работы правила необходимо, чтобы метод provider находился в том же классе, что и тесты — это ограничение PMD, которое не получилось обойти.
Также мы решили отказаться от аннотации @EnumSource, потому что Enum может также находиться в другом файле. Вместо @EnumSource в тестовом проекте мы используем @MethodSource. Метод, который предоставляет параметры для теста, должен возвращать заранее вычисленный список, чтобы можно было посчитать количество необходимых ID. В случаях, когда невозможно выполнить все ограничения правила по объективным причинам, его можно подавить с помощью аннотации @SuppressWarnings. Код правила ParametrizedTestIdsCountRule можно посмотреть в готовом проекте.
Все правила написаны, теперь подключим их к нашему проекту. Для этого создадим новый файл конфигурации pmd-testcaseid-ruleset.xml и заполним его:
pmd-testcaseid-ruleset.xml
<?xml version="1.0"?>
<ruleset name="TestCaseId rule set"
xmlns="http://pmd.sourceforge.net/ruleset/2.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://pmd.sourceforge.net/ruleset/2.0.0 http://pmd.sourceforge.net/ruleset_2_0_0.xsd">
<description>
Rules for check tests ids
</description>
<rule name="TestMustHaveTestIdAnnotationRule"
language="java"
class="com.wrike.checkstyle.wrikerules.TestMustHaveTestIdAnnotationRule"
message="You have to use @TestCaseId with @Test annotation.">
<description>
You have to use @TestCaseId with @Test annotation.
</description>
<priority>1</priority>
</rule>
<rule name="CheckDuplicateTestIdRule"
language="java"
class="com.wrike.checkstyle.wrikerules.CheckDuplicateTestIdRule"
message="There is a duplicate @TestCaseId {0}">
<description>
There is a duplicate @TestCaseId.
</description>
<priority>1</priority>
</rule>
<rule name="ParametrizedTestIdsCountRule"
language="java"
class="com.wrike.checkstyle.wrikerules.ParametrizedTestIdsCountRule"
message="Unexpected test case ids count for parametrized test. Expected {0} ids, but found {1} ids.">
<description>
Check that parameterized tests have the right count of test case ids.
</description>
<priority>1</priority>
</rule>
</ruleset>
Разметка тестов. Иногда хочется объединить группу тестов по какому-то признаку. Для этого в JUnit5 есть аннотация Tag. В тестовом проекте Wrike мы размечаем все скриншотные тесты отдельным тегом, чтобы запускать их отдельно от других тестов. Тест считается скриншотным, если он использует объект класса ScreenshotSteps.
Чтобы определить, что тест использует объект определенного класса, необходимо построить дерево вызовов методов и рекурсивно по нему пройтись. Если хотя бы один метод в иерархии использует объект класса ScreenshotSteps, то тест нужно разметить тегом.
Сокращенный код правила выглядит так:
Код правила ScreenshotTestMustHaveScreenshotTag
public class ScreenshotTestMustHaveScreenshotTag extends AbstractJavaRule {
private static final String SCREENSHOT_TAG = "SCREENSHOT_TEST";
private Map<ASTMethodDeclaration, Set<ASTMethodDeclaration>> screenshotMethodUsages;
private Map<ASTMethodDeclaration, Set<ASTMethodDeclaration>> methodUsages;
private final Set<ASTMethodDeclaration> visitedMethods = new HashSet<>();
@Override
public Object visit(final ASTClassOrInterfaceDeclaration node, final Object data) {
if (nodeHasScreenshotTag(node)) {
return super.visit(node, data);
}
Optional<Entry<VariableNameDeclaration, List<NameOccurrence>>> screenshotStepsDeclaration = getScreenshotStepsDeclaration(node);
if (screenshotStepsDeclaration.isEmpty()) {
return super.visit(node, data);
}
Set<ASTMethodDeclaration> screenshotMethods = getScreenshotMethods(screenshotStepsDeclaration);
methodUsages = getMethodUsages(node);
screenshotMethodUsages = methodUsages.entrySet().stream()
.filter(entry -> screenshotMethods.contains(entry.getKey()))
.collect(Collectors.toMap(Entry::getKey, Entry::getValue));
Set<ASTMethodDeclaration> allScreenshotMethods = dfsTree();
allScreenshotMethods.forEach(method -> {
Set<? extends Class<?>> annotationSet = getAnnotations(method);
if ((annotationSet.contains(ParameterizedTest.class) || annotationSet.contains(Test.class))
&& !nodeHasScreenshotTag(method)) {
addViolation(data, method);
}
});
return super.visit(node, data);
}
}Также для разметки тестов мы используем Epic/Feature/Story — популярный способ разметки тестов, который поддерживает Allure. В некоторых модулях проекта мы придерживаемся следующей концепции: тест должен иметь один @Epic и не более одной @Feature и @Story.
Для решения этой проблемы мы реализовали универсальное абстрактное правило, которое проверяет аннотации класса и методов:
Код правила TestShouldHaveOneEntity
public abstract class TestShouldHaveOneEntity extends AbstractJavaRule {
private static final String TEST_METHOD_JUNIT4_OR_JUNIT5 = "Test";
private final String entityAnnotation;
private final String multipleEntityAnnotation;
private final boolean mustHaveAnnotation;
public TestShouldHaveOneEntity(String annotationEntity, String multipleEntityAnnotation, boolean mustHaveAnnotation) {
super();
this.entityAnnotation = annotationEntity;
this.multipleEntityAnnotation = multipleEntityAnnotation;
this.mustHaveAnnotation = mustHaveAnnotation;
}
@Override
public Object visit(final ASTMethodDeclaration node, final Object data) {
Set<String> annotationNameSet = getMethodAnnotations(node);
if (annotationNameSet.contains(TEST_METHOD_JUNIT4_OR_JUNIT5)) {
Set<String> classAnnotationNameSet = getClassAnnotations(node);
if (annotationNameSet.stream().anyMatch(annotation -> annotation.equals(multipleEntityAnnotation))
|| classAnnotationNameSet.stream().anyMatch(annotation -> annotation.equals(multipleEntityAnnotation))) {
addViolation(data, node);
return super.visit(node, data);
}
boolean methodHasEntityAnnotation = annotationNameSet.stream().anyMatch(annotation -> annotation.equals(entityAnnotation));
boolean classHasEntityAnnotation = classAnnotationNameSet.stream().anyMatch(annotation -> annotation.equals(entityAnnotation));
if (mustHaveAnnotation && methodHasEntityAnnotation == classHasEntityAnnotation) {
addViolation(data, node);
return super.visit(node, data);
}
if (!mustHaveAnnotation && methodHasEntityAnnotation && classHasEntityAnnotation) {
addViolation(data, node);
}
}
return super.visit(node, data);
}
}Теперь правила для @Epic, @Feature и @Story выглядят достаточно просто:
public class TestShouldHaveOnlyOneEpic extends TestShouldHaveOneEntity {
public TestShouldHaveOnlyOneEpic() {
super(Epic.class.getSimpleName(), Epics.class.getSimpleName(), true);
}
}
public class TestShouldHaveNoMoreThanOneStory extends TestShouldHaveOneEntity {
public TestShouldHaveNoMoreThanOneStory() {
super(Story.class.getSimpleName(), Stories.class.getSimpleName(), false);
}
}
public class TestShouldHaveNoMoreThanOneFeature extends TestShouldHaveOneEntity {
public TestShouldHaveNoMoreThanOneFeature() {
super(Feature.class.getSimpleName(), Features.class.getSimpleName(), false);
}
}Другие кастомные правила. Еще одна проблема, с которой приходится сталкиваться — использование аннотации @RepeatedTest вместо @Test. Обычно такая аннотация используется для того, чтобы локально проверять тесты на стабильность. На ревью очень легко упустить этот момент, и тест будет запускаться несколько раз вместо положенного одного. Чтобы решить эту проблему, импорт @RepeatedTest запрещается специальным правилом. Если разработчик запушил ветку с этой аннотацией, ошибка будет выловлена на этапе пайплайна в CI.
public class ShouldNotImportRepeatedTest extends AbstractJavaRule {
private static final String REPEATED_TEST = RepeatedTest.class.getCanonicalName();
@Override
public Object visit(final ASTImportDeclaration node, final Object data) {
String nodeImportedName = node.getImportedName();
if (nodeImportedName.contains(REPEATED_TEST)) {
addViolation(data, node);
return super.visit(node, data);
}
return super.visit(node, data);
}
}В тестовом проекте Wrike мы используем 62 проверки Checkstyle и 271 правило PMD, из которых 236 стандартных и 35 кастомных. Проверка всего проекта, состоящего из 38к тестов и 13к Java файлов, занимает меньше 10 минут. Однако практически всегда diff между веткой и мастером составляет менее 100 файлов, и проверка занимает 2-3 минуты.
Благодаря внедрению Checkstyle и PMD нам удается поддерживать чистоту в проекте и избегать логических ошибок.
Если вы хотите попробовать использовать связку PMD и Checkstyle, ищите проект с настроенными правилами по этой ссылке.
А какие инструменты для поддержания порядка в кодовой базе используете вы?