How To Improve Code Quality in an Autotest Project
Code quality can often be neglected in autotests. In a project maintained by multiple QA engineers, it’s vital to establish a formal set of rules so everyone can easily navigate the project. Moreover, new tests are often written similarly to current ones or even copied with minor changes. If you have thousands of tests, code smells spread fast. To prevent this issue at Wrike, we’ve been using a combination of PMD and Checkstyle tools for the past two years. And it works great.
In this article, we want to share our experience with setting up, customizing, and using these tools. We’ll also explore code quality assurance tools used in the autotests Java project based on the JUnit framework. As for version control, we use Git and Maven for build automation.
As there are more than 30,000 tests in our autotest project and more than 70 contributors, code cleanliness is important to our QA Automation team.
The most common code smells we encounter are:
- Unused or unwanted imports, variables, and private methods.
- Lines that are too long and file sizes that are too large.
- Entity names that don’t follow naming conventions.
- Stylistic problems, e.g., empty lines, several expressions on one line, missing spaces, etc.
In addition, we have specific problems related to autotest projects:
- The test markup for an Epic/Feature/Story is wrong.
- Some test groups need to be tagged.
- Test cases IDs must be unique.
To keep code clean, code review practice is widespread. Such an approach is acceptable except for one weak spot — the reviewer is human and sometimes makes mistakes. And we want every single line of code to be free of the smells listed above.
Now, most modern IDEs have out-of-the-box support of code inspections. Intellij IDEA, for instance, has a good one for a pure development process but is too heavy for CI, so we excluded this option. Besides, we need a tool for all our developers, not just for those using IDEA.
So we found the Checkstyle tool for our needs. It doesn’t work with logical cases like test annotations that must meet certain requirements, unused entities, and so on. This is the work of a PMD static code analyzer, which deals with the most popular cases and supports custom rules. The only disadvantage is that it processes each file separately. For example, unused public methods will be ignored unlike in an IDE. But it’s sufficient for our project.
Setting up PMD and Checkstyle
You can use PMD in different ways. We work with PMD Java API, which is somewhat difficult to set up but the most flexible in checks configuration and processing results. PMD fine-tuning can be found in the documentation here.
You’ll need to pass a list of files to be checked and a configuration with all the required rules. In return, you’ll have a Report class. A report may be printed to the console in a readable format.
A configuration is an XML file with a list of rules — both basic and custom.
<?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>
We’ll add Checkstyle as the Maven dependency just like PMD:
<dependency>
<groupId>com.puppycrawl.tools</groupId>
<artifactId>checkstyle</artifactId>
<version>8.41</version>
</dependency>Now we’ll pass a report file path and a list of files to be checked to the Main class and run a check. Once it’s successfully completed, we can open the report file and analyze it. Otherwise, Checkstyle will exit with error code 1 and the report won’t show up. To avoid that, we’ll override SecurityManager so the program would just throw an exception instead of terminating. And we’ll print all errors from the report file to the console.
This is how it’s done:
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();
}
}
The Checkstyle Rule list is also an XML file:
<?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>
There’s an XML file with a list of files, which will be excluded from scanning:
<?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>
Once both tools are configured, let’s determine files for our checks. Since it isn’t practical to check the whole project each time, we’ll make changes in several files. To check only the changed files we’ll use the next git command: git diff --name-only origin/master. That will return a list of files with changes in comparison to the master branch. And don’t forget to fetch changes from the master with the git fetch command first.
For those who don’t want to set up PMD and Checkstyle from scratch, we’ve prepared a ready-made solution. In this link, you’ll find a project with the checks and rules described above.
For example, let’s take the next class:
public class CommonTest { @Test
public void test() {
if (true) {
;
}
}
}
Then, Checkstyle and PMD checks will produce this output:
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.javaCheckstyle 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...
…
Setting-up custom rules
Let’s create a few custom PMD rules and add them to checks created in the previous step. But first, we recommend you run through the PMD documentation about AST and custom rules.
Tests uniqueness: The history of test runs are stored in the Allure Server. To link test history with a concrete test case, we’ll use our own @TestCaseId annotation. Allure Server collects test history based on ID. A test name can be used as a unique identifier, but we prefer an ID because it won’t be changed, unlike the name.
So, we’ll need to check that all tests are unique and have an ID. CheckDuplicateTestIdRule and TestMustHaveTestIdAnnotationRule custom rules will do the trick.
The TestMustHaveTestIdAnnotationRule will check IDs as follows: If a method has JUnit5 @Test or @ParameterizedTest annotations, it must also be annotated with @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);
}
}
The CheckDuplicateTestIdRule checks the project for ID duplicates. Each method is checked separately so we can tell which one has a problem from the report. The findAllIds method collects every ID for all rule calls in one check run.
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);
}
}
One more rule for IDs is ParametrizedTestIdsCountRule. We need to make sure that the number of IDs in a test annotation equals the number of test parameters so we don’t lose any tests from the Allure reports.
PMD requires the provider method to be in the same class with tests. Otherwise, the rule won’t work (we couldn’t overcome that restriction).
And as Enum classes can be defined in other files towards tests, we gave up on the @EnumSource annotation for the same reason and use @MethodSource instead. A method that provides test parameters must return a pre-computed list so we can define the required number of IDs.
If it’s impossible to comply with all the rule restrictions, we have an option to suppress it by using the @SuppressWarnings annotation.
See the ParametrizedTestIdsCountRule source code in the finished project.
Now as we’ve finished with the rules, let’s add them to the project. To do that we need a configuration file like this:
<?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>
Test classified: Sometimes it’s nice to group tests. JUnit5 has a @Tag annotation for that. For example, in the Wrike autotest project, we tag all screenshot tests with a special tag so they can be run separately. A screenshot test is one using a ScreenshotSteps object.
There’s a way to tell if a test uses an object of some class. We create a call hierarchy and recursively search it for object usage. If at least one method in the call hierarchy uses a ScreenshotSteps object, the test must be tagged.
Here’s a simplified version of the rule:
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);
}}
In addition, we use Allure’s Epic/Feature/Story classification approach. In some project modules, tests need to be annotated with one @Epic and no more than one @Feature or @Story. And we have to make sure tests meet that requirement. We implemented a universal abstract rule checking class and methods annotations for this purpose:
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);
}}
With that abstract rule, the ones for @Epic, @Feature and @Story look quite simple:
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);
}}
Other custom rules: One more issue we face is the use of @RepeatedTest instead of @Test. It’s reasonable for us to do local checks on test stability. It’s easy to overlook the wrong annotation in a code review and, as a result, the test will be run more than once. So we’ll just create a rule forbidding @RepeatedTest annotation imports. If a developer tries to push a branch with the @RepeatedTest, the CI pipeline will fail.
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);
}}
In the Wrike autotest project, we have 62 Checkstyle checks and 271 PMD rules (236 are common and 35 are custom). It takes 10 minutes to check the entire project with 38,000 tests and 13,000 Java files. But realistically, the difference between the master branch and another one is usually fewer than 100 files, so the check takes just two to three minutes.
With the introduction of Checkstyle and PMD, we’ve managed to keep the project clean and avoid logical issues.
For those who want to try our PMD and Checkstyle solution, we provide the finished project here.
Please share your own experience with keeping your code clean.
