Качеством кода в тестах часто пренебрегают. Когда в совместной разработке участвуют десятки 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, ищите проект с настроенными правилами по этой ссылке.
А какие инструменты для поддержания порядка в кодовой базе используете вы?
