Skip to content

Commit

Permalink
Issue checkstyle#6916: migrate tests to junit5 for package internal
Browse files Browse the repository at this point in the history
  • Loading branch information
@pbludov
pbludov committed Dec 11, 2019
1 parent f83f3cd commit 5a2d5de
Show file tree
Hide file tree
Showing 9 changed files with 347 additions and 363 deletions.
10 changes: 9 additions & 1 deletion config/import-control-test.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,15 @@
</subpackage>
</subpackage>
<subpackage name="internal">
<allow pkg="org.junit"/>
<subpackage name="powermock">
<!--
Powermock is not compatible with Junit5
till https://github.com/powermock/powermock/issues/830
and https://github.com/junit-team/junit5/issues/201
Once done, junit-vintage-engine dependency should be removed from pom.xml.
-->
<allow pkg="org.junit" local-only="true"/>
</subpackage>
</subpackage>
<file name=".*Test(Support)?" regex="true">
<allow pkg="org.junit"/>
Expand Down
89 changes: 42 additions & 47 deletions src/test/java/com/puppycrawl/tools/checkstyle/internal/AllChecksTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,12 @@

package com.puppycrawl.tools.checkstyle.internal;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;

import java.lang.reflect.Field;
import java.lang.reflect.Modifier;
import java.util.ArrayList;
Expand All @@ -35,8 +41,7 @@
import java.util.stream.Collectors;
import java.util.stream.Stream;

import org.junit.Assert;
import org.junit.Test;
import org.junit.jupiter.api.Test;

import com.puppycrawl.tools.checkstyle.AbstractModuleTestSupport;
import com.puppycrawl.tools.checkstyle.Checker;
Expand Down Expand Up @@ -277,7 +282,7 @@ public void testDefaultTokensAreSubsetOfAcceptableTokens() throws Exception {
final String errorMessage = String.format(Locale.ROOT,
"%s's default tokens must be a subset"
+ " of acceptable tokens.", check.getName());
Assert.fail(errorMessage);
fail(errorMessage);
}
}
}
Expand All @@ -296,7 +301,7 @@ public void testRequiredTokensAreSubsetOfAcceptableTokens() throws Exception {
final String errorMessage = String.format(Locale.ROOT,
"%s's required tokens must be a subset"
+ " of acceptable tokens.", check.getName());
Assert.fail(errorMessage);
fail(errorMessage);
}
}
}
Expand All @@ -315,7 +320,7 @@ public void testRequiredTokensAreSubsetOfDefaultTokens() throws Exception {
final String errorMessage = String.format(Locale.ROOT,
"%s's required tokens must be a subset"
+ " of default tokens.", check.getName());
Assert.fail(errorMessage);
fail(errorMessage);
}
}
}
Expand All @@ -331,12 +336,10 @@ public void testAllModulesHaveMultiThreadAnnotation() throws Exception {
continue;
}

Assert.assertTrue(
"module '" + module.getSimpleName()
+ "' must contain a multi-thread annotation",
module.isAnnotationPresent(GlobalStatefulCheck.class)
|| module.isAnnotationPresent(FileStatefulCheck.class)
|| module.isAnnotationPresent(StatelessCheck.class));
assertTrue(module.isAnnotationPresent(GlobalStatefulCheck.class)
|| module.isAnnotationPresent(FileStatefulCheck.class)
|| module.isAnnotationPresent(StatelessCheck.class),
"module '" + module.getSimpleName() + "' must contain a multi-thread annotation");
}
}

Expand All @@ -349,7 +352,7 @@ public void testAllModulesAreReferencedInConfigFile() throws Exception {
.forEach(check -> {
final String errorMessage = String.format(Locale.ROOT,
"%s is not referenced in checkstyle_checks.xml", check);
Assert.fail(errorMessage);
fail(errorMessage);
});
}

Expand Down Expand Up @@ -424,10 +427,9 @@ private static void validateAllCheckTokensAreReferencedInConfigFile(String confi
}

for (Entry<String, Set<String>> entry : checkTokens.entrySet()) {
Assert.assertEquals("'" + entry.getKey()
+ "' should have all acceptable tokens from check in " + configName
+ " config or specify an override to ignore the specific tokens",
entry.getValue(), configCheckTokens.get(entry.getKey()));
assertEquals(entry.getValue(), configCheckTokens.get(entry.getKey()),
"'" + entry.getKey() + "' should have all acceptable tokens from check in "
+ configName + " config or specify an override to ignore the specific tokens");
}
}

Expand All @@ -447,7 +449,7 @@ public void testAllCheckstyleModulesHaveXdocDocumentation() throws Exception {
final String missingModuleMessage = String.format(Locale.ROOT,
"Module %s does not have xdoc documentation.",
moduleName);
Assert.fail(missingModuleMessage);
fail(missingModuleMessage);
});
}

Expand All @@ -457,8 +459,8 @@ public void testAllCheckstyleModulesInCheckstyleConfig() throws Exception {
final Set<String> moduleNames = CheckUtil.getSimpleNames(CheckUtil.getCheckstyleModules());

for (String moduleName : moduleNames) {
Assert.assertTrue("checkstyle_checks.xml is missing module: " + moduleName,
configChecks.contains(moduleName));
assertTrue(configChecks.contains(moduleName),
"checkstyle_checks.xml is missing module: " + moduleName);
}
}

Expand All @@ -467,9 +469,8 @@ public void testAllCheckstyleChecksHaveMessage() throws Exception {
for (Class<?> module : CheckUtil.getCheckstyleChecks()) {
final String name = module.getSimpleName();

Assert.assertFalse(name
+ " should have at least one 'MSG_*' field for error messages", CheckUtil
.getCheckMessages(module).isEmpty());
assertFalse(CheckUtil.getCheckMessages(module).isEmpty(),
name + " should have at least one 'MSG_*' field for error messages");
}
}

Expand All @@ -480,9 +481,10 @@ public void testAllCheckstyleMessages() throws Exception {
// test validity of messages from modules
for (Class<?> module : CheckUtil.getCheckstyleModules()) {
for (Field message : CheckUtil.getCheckMessages(module)) {
Assert.assertEquals(module.getSimpleName() + "." + message.getName()
+ " should be 'public static final'", Modifier.PUBLIC | Modifier.STATIC
| Modifier.FINAL, message.getModifiers());
assertEquals(Modifier.PUBLIC | Modifier.STATIC | Modifier.FINAL,
message.getModifiers(),
module.getSimpleName() + "." + message.getName()
+ " should be 'public static final'");

// below is required for package/private classes
if (!message.isAccessible()) {
Expand All @@ -505,8 +507,9 @@ public void testAllCheckstyleMessages() throws Exception {
continue;
}

Assert.assertTrue("property '" + key + "' isn't used by any check in package '"
+ entry.getKey() + "'", entry.getValue().contains(key.toString()));
assertTrue(entry.getValue().contains(key.toString()),
"property '" + key + "' isn't used by any check in package '"
+ entry.getKey() + "'");
}
}
}
Expand All @@ -515,12 +518,8 @@ private static void verifyCheckstyleMessage(Map<String, List<String>> usedMessag
Class<?> module, Field message) throws Exception {
final String messageString = message.get(null).toString();
final String packageName = module.getPackage().getName();
List<String> packageMessages = usedMessages.get(packageName);

if (packageMessages == null) {
packageMessages = new ArrayList<>();
usedMessages.put(packageName, packageMessages);
}
final List<String> packageMessages =
usedMessages.computeIfAbsent(packageName, key -> new ArrayList<>());

packageMessages.add(messageString);

Expand All @@ -532,24 +531,20 @@ private static void verifyCheckstyleMessage(Map<String, List<String>> usedMessag
}
// -@cs[IllegalCatch] There is no other way to deliver filename that was used
catch (Exception ex) {
Assert.fail(module.getSimpleName() + " with the message '" + messageString
fail(module.getSimpleName() + " with the message '" + messageString
+ "' in locale '" + locale.getLanguage() + "' failed with: "
+ ex.getClass().getSimpleName() + " - " + ex.getMessage());
}

Assert.assertNotNull(
module.getSimpleName() + " should have text for the message '"
+ messageString + "' in locale " + locale.getLanguage() + "'",
result);
Assert.assertFalse(
module.getSimpleName() + " should have non-empty text for the message '"
+ messageString + "' in locale '" + locale.getLanguage() + "'",
result.trim().isEmpty());
Assert.assertFalse(
module.getSimpleName() + " should have non-TODO text for the message '"
+ messageString + "' in locale " + locale.getLanguage() + "'",
!"todo.match".equals(messageString)
&& result.trim().startsWith("TODO"));
assertNotNull(result, module.getSimpleName() + " should have text for the message '"
+ messageString + "' in locale " + locale.getLanguage() + "'");
assertFalse(result.trim().isEmpty(), module.getSimpleName()
+ " should have non-empty text for the message '"
+ messageString + "' in locale '" + locale.getLanguage() + "'");
assertFalse(!"todo.match".equals(messageString) && result.trim().startsWith("TODO"),
module.getSimpleName()
+ " should have non-TODO text for the message '"
+ messageString + "' in locale " + locale.getLanguage() + "'");
}
}

Expand Down
42 changes: 15 additions & 27 deletions src/test/java/com/puppycrawl/tools/checkstyle/internal/AllTestsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@

package com.puppycrawl.tools.checkstyle.internal;

import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
Expand All @@ -32,8 +35,7 @@
import java.util.function.Consumer;
import java.util.stream.Stream;

import org.junit.Assert;
import org.junit.Test;
import org.junit.jupiter.api.Test;

/**
* AllTestsTest.
Expand All @@ -49,7 +51,7 @@ public void testAllInputsHaveTest() throws Exception {
grabAllTests(allTests, filePath.toFile());
});

Assert.assertTrue("found tests", !allTests.keySet().isEmpty());
assertFalse(allTests.keySet().isEmpty(), "found tests");

walk(Paths.get("src/test/resources/com/puppycrawl"), filePath -> {
verifyInputFile(allTests, filePath.toFile());
Expand All @@ -67,7 +69,7 @@ public void testAllTestsHaveProductionCode() throws Exception {
grabAllFiles(allTests, filePath.toFile());
});

Assert.assertTrue("found tests", !allTests.keySet().isEmpty());
assertFalse(allTests.keySet().isEmpty(), "found tests");

walk(Paths.get("src/test/java"), filePath -> {
verifyHasProductionFile(allTests, filePath.toFile());
Expand Down Expand Up @@ -99,14 +101,7 @@ private static void grabAllTests(Map<String, List<String>> allTests, File file)

final int slash = path.lastIndexOf(File.separatorChar);
final String packge = path.substring(0, slash);

List<String> classes = allTests.get(packge);

if (classes == null) {
classes = new ArrayList<>();

allTests.put(packge, classes);
}
final List<String> classes = allTests.computeIfAbsent(packge, key -> new ArrayList<>());

classes.add(path.substring(slash + 1));
}
Expand All @@ -125,14 +120,7 @@ private static void grabAllFiles(Map<String, List<String>> allTests, File file)

final int slash = path.lastIndexOf(File.separatorChar);
final String packge = path.substring(0, slash);

List<String> classes = allTests.get(packge);

if (classes == null) {
classes = new ArrayList<>();

allTests.put(packge, classes);
}
final List<String> classes = allTests.computeIfAbsent(packge, key -> new ArrayList<>());

classes.add(path.substring(slash + 1));
}
Expand All @@ -157,8 +145,8 @@ private static void verifyInputFile(Map<String, List<String>> allTests, File fil
final boolean skipFileNaming = shouldSkipInputFileNameCheck(path, fileName);

if (!skipFileNaming) {
Assert.assertTrue("Resource must start with 'Input' or 'Expected': " + path,
fileName.startsWith("Input") || fileName.startsWith("Expected"));
assertTrue(fileName.startsWith("Input") || fileName.startsWith("Expected"),
"Resource must start with 'Input' or 'Expected': " + path);

if (fileName.startsWith("Input")) {
fileName = fileName.substring(5);
Expand Down Expand Up @@ -202,9 +190,9 @@ && checkInputMatchCorrectFileStructure(classes, folderPath, skipFileNaming,
}
}

Assert.assertTrue("Resource must be named after a Test like 'InputMyCustomCase.java' "
assertTrue(found, "Resource must be named after a Test like 'InputMyCustomCase.java' "
+ "and be in the sub-package of the test like 'mycustom' "
+ "for test 'MyCustomCheckTest': " + path, found);
+ "for test 'MyCustomCheckTest': " + path);
}

private static void verifyHasProductionFile(Map<String, List<String>> allTests, File file) {
Expand All @@ -229,9 +217,9 @@ private static void verifyHasProductionFile(Map<String, List<String>> allTests,
final String packge = path.substring(0, slash);
final List<String> classes = allTests.get(packge);

Assert.assertTrue("Test must be named after a production class "
+ "and must be in the same package of the production class: " + path,
classes != null && classes.contains(fileName));
assertTrue(classes != null && classes.contains(fileName),
"Test must be named after a production class "
+ "and must be in the same package of the production class: " + path);
}
}
}
Expand Down
Loading

0 comments on commit 5a2d5de

Please sign in to comment.