test: Add stronger processor tests (#167)

* Add expected output file for BigDecimalDOubleConstructor.java

* Add expected output file for ArrayHashCodeAndToString.java

* Add processor test with reference output

* Refactor tests to be more maintainable

* Add a sanity check that the expected output has no issues

* Further reduce code duplication between tests

* Fix formatting

src/test/java/sorald/processor/ProcessorTest.java

@@ -1,36 +1,32 @@
 package sorald.processor;
 
+import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 
 import java.io.File;
-import java.nio.file.Paths;
-import java.util.Arrays;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.Comparator;
 import java.util.List;
+import java.util.stream.Collectors;
 import java.util.stream.Stream;
 import org.junit.jupiter.api.extension.ExtensionContext;
+import org.junit.jupiter.api.io.TempDir;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.ArgumentsProvider;
 import org.junit.jupiter.params.provider.ArgumentsSource;
-import org.sonar.java.checks.InterruptedExceptionCheck;
-import org.sonar.java.checks.SynchronizationOnStringOrBoxedCheck;
-import org.sonar.java.checks.serialization.SerializableFieldInSerializableClassCheck;
 import org.sonar.plugins.java.api.JavaFileScanner;
 import sorald.Constants;
-import sorald.Main;
-import sorald.PrettyPrintingStrategy;
 import sorald.TestHelper;
 import sorald.sonar.RuleVerifier;
+import spoon.Launcher;
+import spoon.reflect.CtModel;
+import spoon.reflect.declaration.CtImport;
+import spoon.reflect.declaration.CtType;
 
 public class ProcessorTest {
 
-    // The processors related to these checks currently cause problems with the sniper printer
-    private static final List<Class<?>> BROKEN_WITH_SNIPER =
-            Arrays.asList(
-                    SynchronizationOnStringOrBoxedCheck.class,
-                    InterruptedExceptionCheck.class,
-                    SerializableFieldInSerializableClassCheck.class);
-
     /**
      * Parameterized test that processes a single Java file at a time with a single processor.
      *
@@ -48,29 +44,10 @@ public void testProcessSingleFile(
         assertFalse(
                 new File(Constants.SORALD_WORKSPACE).exists(),
                 "workspace should must be clean before test");
-        String pathToRepairedFile =
-                Paths.get(Constants.SORALD_WORKSPACE)
-                        .resolve(Constants.SPOONED)
-                        .resolve(testCase.outfileRelpath)
-                        .toString();
-        String originalFileAbspath = testCase.nonCompliantFile.toPath().toAbsolutePath().toString();
-        boolean brokenWithSniper = BROKEN_WITH_SNIPER.contains(testCase.checkClass);
-
-        RuleVerifier.verifyHasIssue(originalFileAbspath, testCase.createCheckInstance());
-        Main.main(
-                new String[] {
-                    Constants.ARG_SYMBOL + Constants.ARG_ORIGINAL_FILES_PATH,
-                    originalFileAbspath,
-                    Constants.ARG_SYMBOL + Constants.ARG_RULE_KEYS,
-                    testCase.ruleKey,
-                    Constants.ARG_SYMBOL + Constants.ARG_WORKSPACE,
-                    Constants.SORALD_WORKSPACE,
-                    Constants.ARG_SYMBOL + Constants.ARG_PRETTY_PRINTING_STRATEGY,
-                    brokenWithSniper
-                            ? PrettyPrintingStrategy.NORMAL.name()
-                            : PrettyPrintingStrategy.SNIPER.name()
-                });
 
+        ProcessorTestHelper.runSorald(testCase);
+
+        String pathToRepairedFile = testCase.repairedFilePath().toString();
         TestHelper.removeComplianceComments(pathToRepairedFile);
         RuleVerifier.verifyNoIssue(pathToRepairedFile, testCase.createCheckInstance());
     }
@@ -83,14 +60,95 @@ private static class NonCompliantJavaFileProvider implements ArgumentsProvider {
 
         @Override
         public Stream<? extends Arguments> provideArguments(ExtensionContext extensionContext) {
-            return Arrays.stream(ProcessorTestHelper.TEST_FILES_ROOT.toFile().listFiles())
-                    .filter(File::isDirectory)
-                    .flatMap(
-                            dir ->
-                                    Arrays.stream(dir.listFiles())
-                                            .filter(file -> file.getName().endsWith(".java"))
-                                            .map(ProcessorTestHelper::toProcessorTestCase))
+            return ProcessorTestHelper.getTestCaseStream().map(Arguments::of);
+        }
+    }
+
+    /**
+     * Parameterized test that processes a single Java file at a time with a single processor, and
+     * compares the output to a reference. It executes on a subset of the test files acted upon by
+     * {@link ProcessorTest#testProcessSingleFile(ProcessorTestHelper.ProcessorTestCase, File)}.
+     *
+     * <p>If a input test file A.java has a sibling A.java.expected, then this test is executed with
+     * A.java.expected as the expected output from processing A.java.
+     */
+    @ParameterizedTest
+    @ArgumentsSource(NonCompliantJavaFileWithExpectedProvider.class)
+    public void testProcessSingleFile(
+            ProcessorTestHelper.ProcessorTestCase<? extends JavaFileScanner> testCase,
+            @TempDir File tempdir)
+            throws Exception {
+        // arrange
+        assertFalse(
+                new File(Constants.SORALD_WORKSPACE).exists(),
+                "workspace should must be clean before test");
+
+        // Spoon does not like parsing files that don't end in .java, so we must copy the .expected
+        // files to end with .java
+        Path expectedOutput = tempdir.toPath().resolve(testCase.nonCompliantFile.getName());
+        Files.copy(
+                testCase.expectedOutfile().orElseThrow(IllegalStateException::new).toPath(),
+                expectedOutput);
+        RuleVerifier.verifyNoIssue(
+                expectedOutput.toAbsolutePath().toString(), testCase.createCheckInstance());
+
+        // act
+        ProcessorTestHelper.runSorald(testCase);
+
+        // assert
+        Path pathToRepairedFile = testCase.repairedFilePath();
+        CtModel repairedModel = parseNoComments(pathToRepairedFile);
+        CtModel expectedModel = parseNoComments(expectedOutput);
+
+        List<CtType<?>> repairedTypes = getSortedTypes(repairedModel);
+        List<CtType<?>> expectedTypes = getSortedTypes(expectedModel);
+        List<CtImport> repairedImports = getSortedImports(repairedModel);
+        List<CtImport> expectedImports = getSortedImports(expectedModel);
+
+        assertEquals(expectedTypes, repairedTypes);
+        assertEquals(repairedImports, expectedImports);
+    }
+
+    /**
+     * Provider class that provides test cases based on the buggy/non-compliant Java source files in
+     * the test files directory, that also have an expected outcome for the bugfix. The expected
+     * files have the same name as their corresponding buggy files, but with the suffix ".expected".
+     */
+    private static class NonCompliantJavaFileWithExpectedProvider implements ArgumentsProvider {
+
+        @Override
+        public Stream<? extends Arguments> provideArguments(ExtensionContext extensionContext) {
+            return ProcessorTestHelper.getTestCaseStream()
+                    .filter(testCase -> testCase.expectedOutfile().isPresent())
                     .map(Arguments::of);
         }
     }
+
+    private static CtModel parseNoComments(Path javaFile) {
+        Launcher launcher = new Launcher();
+        launcher.getEnvironment().setCommentEnabled(false);
+        launcher.getEnvironment().setAutoImports(true);
+        launcher.addInputResource(javaFile.toString());
+        return launcher.buildModel();
+    }
+
+    private static List<CtImport> getSortedImports(CtModel model) {
+        return model.getAllTypes().stream()
+                .flatMap(
+                        type ->
+                                type
+                                        .getFactory()
+                                        .CompilationUnit()
+                                        .getOrCreate(type)
+                                        .getImports()
+                                        .stream())
+                .sorted(Comparator.comparing(CtImport::prettyprint))
+                .collect(Collectors.toList());
+    }
+
+    private static List<CtType<?>> getSortedTypes(CtModel model) {
+        return model.getAllTypes().stream()
+                .sorted(Comparator.comparing(CtType::getQualifiedName))
+                .collect(Collectors.toList());
+    }
 }

src/test/java/sorald/processor/ProcessorTestHelper.java

@@ -6,17 +6,33 @@
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
+import java.util.Arrays;
 import java.util.List;
+import java.util.Optional;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
+import java.util.stream.Stream;
+import org.sonar.java.checks.InterruptedExceptionCheck;
+import org.sonar.java.checks.SynchronizationOnStringOrBoxedCheck;
+import org.sonar.java.checks.serialization.SerializableFieldInSerializableClassCheck;
 import org.sonar.plugins.java.api.JavaFileScanner;
 import sorald.Constants;
+import sorald.Main;
+import sorald.PrettyPrintingStrategy;
 import sorald.sonar.Checks;
+import sorald.sonar.RuleVerifier;
 
 /** Helper functions for {@link ProcessorTest}. */
 public class ProcessorTestHelper {
     static final Path TEST_FILES_ROOT =
             Paths.get(Constants.PATH_TO_RESOURCES_FOLDER).resolve("processor_test_files");
+    static final String EXPECTED_FILE_SUFFIX = ".expected";
+    // The processors related to these checks currently cause problems with the sniper printer
+    static final List<Class<?>> BROKEN_WITH_SNIPER =
+            Arrays.asList(
+                    SynchronizationOnStringOrBoxedCheck.class,
+                    InterruptedExceptionCheck.class,
+                    SerializableFieldInSerializableClassCheck.class);
 
     /**
      * Create a {@link ProcessorTestCase} from a non-compliant (according to SonarQube rules) Java
@@ -72,6 +88,40 @@ private static String parseSourceFilePackage(Path sourceFile) {
         return "";
     }
 
+    /**
+     * Return a stream of all valid test cases, based on the tests files in {@link
+     * ProcessorTestHelper#TEST_FILES_ROOT}.
+     */
+    static Stream<ProcessorTestCase<?>> getTestCaseStream() {
+        return Arrays.stream(ProcessorTestHelper.TEST_FILES_ROOT.toFile().listFiles())
+                .filter(File::isDirectory)
+                .map(File::listFiles)
+                .flatMap(Arrays::stream)
+                .filter(file -> file.getName().endsWith(".java"))
+                .map(ProcessorTestHelper::toProcessorTestCase);
+    }
+
+    /** Run sorald on the given test case. */
+    static void runSorald(ProcessorTestCase<?> testCase) throws Exception {
+        String originalFileAbspath = testCase.nonCompliantFile.toPath().toAbsolutePath().toString();
+        RuleVerifier.verifyHasIssue(originalFileAbspath, testCase.createCheckInstance());
+
+        boolean brokenWithSniper = BROKEN_WITH_SNIPER.contains(testCase.checkClass);
+        Main.main(
+                new String[] {
+                    Constants.ARG_SYMBOL + Constants.ARG_ORIGINAL_FILES_PATH,
+                    originalFileAbspath,
+                    Constants.ARG_SYMBOL + Constants.ARG_RULE_KEYS,
+                    testCase.ruleKey,
+                    Constants.ARG_SYMBOL + Constants.ARG_WORKSPACE,
+                    Constants.SORALD_WORKSPACE,
+                    Constants.ARG_SYMBOL + Constants.ARG_PRETTY_PRINTING_STRATEGY,
+                    brokenWithSniper
+                            ? PrettyPrintingStrategy.NORMAL.name()
+                            : PrettyPrintingStrategy.SNIPER.name()
+                });
+    }
+
     /**
      * A wrapper class to hold the information required to execute a test case for a single file and
      * rule with the associated processor.
@@ -111,5 +161,20 @@ public T createCheckInstance()
                         InstantiationException {
             return checkClass.getConstructor().newInstance();
         }
+
+        public Optional<File> expectedOutfile() {
+            File expectedOutfile =
+                    nonCompliantFile
+                            .toPath()
+                            .resolveSibling(nonCompliantFile.getName() + EXPECTED_FILE_SUFFIX)
+                            .toFile();
+            return Optional.ofNullable(expectedOutfile.isFile() ? expectedOutfile : null);
+        }
+
+        public Path repairedFilePath() {
+            return Paths.get(Constants.SORALD_WORKSPACE)
+                    .resolve(Constants.SPOONED)
+                    .resolve(outfileRelpath);
+        }
     }
 }

src/test/resources/processor_test_files/2111_BigDecimalDoubleConstructor/BigDecimalDoubleConstructor.java.expected

@@ -0,0 +1,39 @@
+// Test for rule s2111
+
+import java.math.BigDecimal;
+import java.math.MathContext;
+
+public class BigDecimalDoubleConstructor {
+
+    // Tests from https://rules.sonarsource.com/java/type/Bug/RSPEC-2111
+    public void main(String[] args) {
+        double d = 1.1;
+        BigDecimal bd1 = BigDecimal.valueOf(d);
+        BigDecimal bd2 = BigDecimal.valueOf(1.1);
+    }
+
+    // Tests from https://github.com/SonarSource/sonar-java/blob/master/java-checks-test-sources/src/main/java/checks/BigDecimalDoubleConstructorCheck.java
+    public void main2(String[] args) {
+        MathContext mc;
+        BigDecimal bd1 = new BigDecimal("1");
+        BigDecimal bd2 = BigDecimal.valueOf(2.0);
+        BigDecimal bd4 = new BigDecimal("2.0", mc);
+        BigDecimal bd5 = BigDecimal.valueOf(2.0f);
+        BigDecimal bd6 = new BigDecimal("2.0", mc);
+        BigDecimal bd3 = BigDecimal.valueOf(2.0);
+    }
+
+    // Aditional tests
+    public void foo(String[] args) {
+        double d = 1.1;
+        float f = 2.2;
+        float f1 = 2f;
+        BigDecimal bd3 = BigDecimal.valueOf(f);
+        BigDecimal bd4 = BigDecimal.valueOf(f1);
+        BigDecimal bd5 = BigDecimal.valueOf(d);
+        BigDecimal bd6 = new BigDecimal("1.1");
+        BigDecimal bd7 = BigDecimal.valueOf(f);
+        BigDecimal bd8 = BigDecimal.valueOf(f1);
+    }
+
+}

src/test/resources/processor_test_files/2116_ArrayHashCodeAndToString/ArrayHashCodeAndToString.java.expected

@@ -0,0 +1,30 @@
+
+// Test for rule s2116
+
+public class ArrayHashCodeAndToString {
+
+	// Tests from https://rules.sonarsource.com/java/type/Bug/RSPEC-2116
+	public static void main( String[] args ) {
+		String argStr = java.util.Arrays.toString(args); // Noncompliant
+		int argHash = java.util.Arrays.hashCode(args); // Noncompliant
+	}
+
+	// Tests from https://github.com/SonarSource/sonar-java/blob/master/java-checks-test-sources/src/main/java/checks/ArrayHashCodeAndToStringCheck.java
+	void method(String[] args, String string) {
+		Class class1 = args.getClass();
+		String str = string.toString();
+		int hash = string.hashCode();
+	}
+
+	// Aditional tests
+	public void foo(String[] args) {
+		String[] array1 = {"F", "O", "O"};
+		System.out.println(java.util.Arrays.toString(array1)); // Noncompliant
+		varargsTest(1, 2, 3);
+	}
+
+	private void varargsTest(int... array2) {
+		String a = java.util.Arrays.toString(array2); // Noncompliant
+	}
+
+}