From fe9476af186efbc9dce642256c7c46369d4e393c Mon Sep 17 00:00:00 2001 From: Manu Sridharan Date: Fri, 6 Sep 2024 09:25:23 -0700 Subject: [PATCH] Create basic unit tests for library model generation (#1031) This is in preparation for #1006. When working on that PR I realized we really need some unit tests to more easily debug various scenarios (right now all we have are integration tests). Plus this will make our code coverage reports more useful. This adds some infrastructure for unit tests and a couple of basic tests; we will add more as we go. Also fix a couple minor issues in `LibraryModelGenerator`. --- .../library-model-generator/build.gradle | 2 + .../libmodel/LibraryModelGenerator.java | 74 ++++++++++----- .../libmodel/LibraryModelGeneratorTest.java | 91 +++++++++++++++++++ 3 files changed, 143 insertions(+), 24 deletions(-) create mode 100644 library-model/library-model-generator/src/test/java/com/uber/nullaway/libmodel/LibraryModelGeneratorTest.java diff --git a/library-model/library-model-generator/build.gradle b/library-model/library-model-generator/build.gradle index 1d497fccd7..cf2cb1f2be 100644 --- a/library-model/library-model-generator/build.gradle +++ b/library-model/library-model-generator/build.gradle @@ -23,4 +23,6 @@ dependencies { implementation deps.build.javaparser compileOnly deps.apt.autoValueAnnot annotationProcessor deps.apt.autoValue + + testImplementation deps.test.junit4 } diff --git a/library-model/library-model-generator/src/main/java/com/uber/nullaway/libmodel/LibraryModelGenerator.java b/library-model/library-model-generator/src/main/java/com/uber/nullaway/libmodel/LibraryModelGenerator.java index bc178ae644..03546f7c1b 100644 --- a/library-model/library-model-generator/src/main/java/com/uber/nullaway/libmodel/LibraryModelGenerator.java +++ b/library-model/library-model-generator/src/main/java/com/uber/nullaway/libmodel/LibraryModelGenerator.java @@ -62,18 +62,45 @@ */ public class LibraryModelGenerator { + /** + * Data class for storing the annotation information collected from the source files. This is the + * information that is stored in the astubx file. + */ + public static class LibraryModelData { + public final Map methodRecords; + public final Map> nullableUpperBounds; + + public LibraryModelData( + Map methodRecords, + Map> nullableUpperBounds) { + this.methodRecords = methodRecords; + this.nullableUpperBounds = nullableUpperBounds; + } + + @Override + public String toString() { + return "ModelData{" + + "methodRecords=" + + methodRecords + + ", nullableUpperBounds=" + + nullableUpperBounds + + '}'; + } + } + /** * Parses all the source files within the directory using javaparser. * * @param inputSourceDirectory Directory containing annotated java source files. - * @param outputDirectory Directory to write the astubx file into. + * @param outputFile absolute path to the output file. */ - public void generateAstubxForLibraryModels(String inputSourceDirectory, String outputDirectory) { + public static LibraryModelData generateAstubxForLibraryModels( + String inputSourceDirectory, String outputFile) { Map methodRecords = new LinkedHashMap<>(); Map> nullableUpperBounds = new LinkedHashMap<>(); Path root = dirnameToPath(inputSourceDirectory); - AnnotationCollectorCallback ac = - new AnnotationCollectorCallback(methodRecords, nullableUpperBounds); + LibraryModelData modelData = new LibraryModelData(methodRecords, nullableUpperBounds); + AnnotationCollectorCallback ac = new AnnotationCollectorCallback(modelData); CollectionStrategy strategy = new ParserCollectionStrategy(); // Required to include directories that contain a module-info.java, which don't parse by // default. @@ -90,19 +117,19 @@ public void generateAstubxForLibraryModels(String inputSourceDirectory, String o throw new RuntimeException(e); } }); - writeToAstubx(outputDirectory, methodRecords, nullableUpperBounds); + writeToAstubx(outputFile, modelData); + return modelData; } /** * Writes the Nullability annotation information into the output directory as an astubx file. * - * @param outputPath Output Directory. - * @param methodRecords Map containing the collected Nullability annotation information. + * @param outputPath path to output astubx file. + * @param modelData ModelData instance containing the collected annotation information. */ - private void writeToAstubx( - String outputPath, - Map methodRecords, - Map> nullableUpperBounds) { + private static void writeToAstubx(String outputPath, LibraryModelData modelData) { + Map methodRecords = modelData.methodRecords; + Map> nullableUpperBounds = modelData.nullableUpperBounds; if (methodRecords.isEmpty() && nullableUpperBounds.isEmpty()) { return; } @@ -127,7 +154,7 @@ private void writeToAstubx( } } - public Path dirnameToPath(String dir) { + public static Path dirnameToPath(String dir) { File f = new File(dir); String absoluteDir = f.getAbsolutePath(); if (absoluteDir.endsWith("/.")) { @@ -140,11 +167,8 @@ private static class AnnotationCollectorCallback implements SourceRoot.Callback private final AnnotationCollectionVisitor annotationCollectionVisitor; - public AnnotationCollectorCallback( - Map methodRecords, - Map> nullableUpperBounds) { - this.annotationCollectionVisitor = - new AnnotationCollectionVisitor(methodRecords, nullableUpperBounds); + public AnnotationCollectorCallback(LibraryModelData modelData) { + this.annotationCollectionVisitor = new AnnotationCollectionVisitor(modelData); } @Override @@ -171,11 +195,9 @@ private static class AnnotationCollectionVisitor extends VoidVisitorAdapter methodRecords, - Map> nullableUpperBounds) { - this.methodRecords = methodRecords; - this.nullableUpperBounds = nullableUpperBounds; + public AnnotationCollectionVisitor(LibraryModelData modelData) { + this.methodRecords = modelData.methodRecords; + this.nullableUpperBounds = modelData.nullableUpperBounds; } @Override @@ -199,7 +221,11 @@ public void visit(ClassOrInterfaceDeclaration cid, Void arg) { logic does not currently handle cases where @NullMarked annotations appear on some nested classes but not others. It also does not consider annotations within package-info.java or module-info.java files.*/ - parentName += "." + cid.getNameAsString(); + String oldParentName = parentName; + if (!parentName.isEmpty()) { + parentName += "."; + } + parentName += cid.getNameAsString(); cid.getAnnotations() .forEach( a -> { @@ -216,7 +242,7 @@ public void visit(ClassOrInterfaceDeclaration cid, Void arg) { } super.visit(cid, null); // We reset the variable that constructs the parent name after visiting all the children. - parentName = parentName.substring(0, parentName.lastIndexOf("." + cid.getNameAsString())); + parentName = oldParentName; } @Override diff --git a/library-model/library-model-generator/src/test/java/com/uber/nullaway/libmodel/LibraryModelGeneratorTest.java b/library-model/library-model-generator/src/test/java/com/uber/nullaway/libmodel/LibraryModelGeneratorTest.java new file mode 100644 index 0000000000..25bbbe2091 --- /dev/null +++ b/library-model/library-model-generator/src/test/java/com/uber/nullaway/libmodel/LibraryModelGeneratorTest.java @@ -0,0 +1,91 @@ +package com.uber.nullaway.libmodel; + +import static org.hamcrest.CoreMatchers.equalTo; +import static org.hamcrest.MatcherAssert.assertThat; + +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Paths; +import java.util.Set; +import org.junit.Assert; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +public class LibraryModelGeneratorTest { + + /** For input source code files */ + @Rule public TemporaryFolder inputSourcesFolder = new TemporaryFolder(); + + /** For output astubx files */ + @Rule public TemporaryFolder outputFolder = new TemporaryFolder(); + + private void runTest( + String sourceFileName, + String[] lines, + ImmutableMap expectedMethodRecords, + ImmutableMap> expectedNullableUpperBounds) + throws IOException { + // write it to a source file in inputSourcesFolder with the right file name + Files.write( + inputSourcesFolder.newFile(sourceFileName).toPath(), + String.join("\n", lines).getBytes(StandardCharsets.UTF_8)); + // run the generator + String astubxOutputPath = + Paths.get(outputFolder.getRoot().getAbsolutePath(), "output.astubx").toString(); + LibraryModelGenerator.LibraryModelData modelData = + LibraryModelGenerator.generateAstubxForLibraryModels( + inputSourcesFolder.getRoot().getAbsolutePath(), astubxOutputPath); + System.err.println("modelData: " + modelData); + Assert.assertTrue("astubx file was not created", Files.exists(Paths.get(astubxOutputPath))); + assertThat(modelData.methodRecords, equalTo(expectedMethodRecords)); + assertThat(modelData.nullableUpperBounds, equalTo(expectedNullableUpperBounds)); + } + + @Test + public void nullableReturn() throws IOException { + String[] lines = + new String[] { + "import org.jspecify.annotations.NullMarked;", + "import org.jspecify.annotations.Nullable;", + "@NullMarked", + "public class AnnotationExample {", + " @Nullable", + " public String makeUpperCase(String inputString) {", + " if (inputString == null || inputString.isEmpty()) {", + " return null;", + " } else {", + " return inputString.toUpperCase();", + " }", + " }", + "}" + }; + ImmutableMap expectedMethodRecords = + ImmutableMap.of( + "AnnotationExample:String makeUpperCase(String)", + MethodAnnotationsRecord.create(ImmutableSet.of("Nullable"), ImmutableMap.of())); + runTest("AnnotationExample.java", lines, expectedMethodRecords, ImmutableMap.of()); + } + + @Test + public void nullableUpperBound() throws IOException { + String[] lines = + new String[] { + "import org.jspecify.annotations.NullMarked;", + "import org.jspecify.annotations.Nullable;", + "@NullMarked", + "public class NullableUpperBound {", + " T nullableObject;", + " public T getNullable() {", + " return nullableObject;", + " }", + "}" + }; + ImmutableMap> expectedNullableUpperBounds = + ImmutableMap.of("NullableUpperBound", ImmutableSet.of(0)); + runTest("NullableUpperBound.java", lines, ImmutableMap.of(), expectedNullableUpperBounds); + } +}