Enhanced Serialization Test Infrastructure #579

nimakarimipour · 2022-02-23T03:45:56Z

Following Type Change Serialization PR NullAway can serialize information regarding the source code through the creation of different objects (e.g. ErrorInfo and SuggestedFixInfo).

The current testing infrastructure works as below:

A SerializationTestHelper object is instantiated and the input source code is passed with the API below:

SerializationTestHelper tester = new SerializationTestHelper();
tester.addSourceLines(...)

And the expected FixDisplay is passed to the tester using the API below:

tester.setExpectedFixes(
       new FixDisplay(), new FixDisplay(), ...);

And finally after setting the correct flags, the doTest() method is invoked.

At this moment, SerializationTestHelper instance will only look for fixes.tsv file, and for each line in the file, it creates the corresponding FixDisplay instances, therefore, the expected output can only be in the type of FixDisplay. Also, no test is created to verify correctness of error serialization feature. Since SerializationTestHelper is not flexible to test different output types, it cannot be reused in testing upcoming features of serialization package.

This PR generalizes the SerializationTestHelper class to be reusable with any type of serialization output.

SerializationTestHelper<T>

SerializationTestHelper<FixDisplay> // to test fix displays
SerializationTestHelper<ErrorDisplay> // to test error serialization

With the new implementation, tester instance will look for the target file which is configurable, verifies the correctness of the header and then creates instances of T per line in the output file and executes the tests.

Also tests to cover error serialization is added.

Regards

msridhar

Overall this change looks good. I have some minor comments

nullaway/src/test/java/com/uber/nullaway/tools/Factory.java

msridhar · 2022-02-23T21:45:40Z

nullaway/src/test/java/com/uber/nullaway/NullAwaySerializationTest.java

+              values.length == 10,
+              "Needs exactly 10 values to create FixDisplay object but found: " + values.length);
+          // Fixes are written in Temp Directory and is not known at compile time, therefore,
+          // relative


extra newline in comment

msridhar · 2022-02-23T21:48:12Z

nullaway/src/test/java/com/uber/nullaway/tools/SerializationTestHelper.java

-public class SerializationTestHelper {
-  private FixDisplay[] expectedOutputFixes;
-  private final Path outputPath;
+public class SerializationTestHelper<T> {


Suggested change

public class SerializationTestHelper<T> {

public class SerializationTestHelper<T extends Display> {

See other comment about Display interface

Co-authored-by: Manu Sridharan <[email protected]>

nimakarimipour · 2022-02-23T23:27:31Z

Hi @msridhar, I believe this is ready for another review.

msridhar

LGTM, just a couple more minor things

msridhar · 2022-02-23T23:38:09Z

nullaway/src/test/java/com/uber/nullaway/NullAwaySerializationTest.java

+  private final DisplayFactory<FixDisplay> fixDisplayFactory;
+  private final DisplayFactory<ErrorDisplay> errorDisplayFactory;
+
+  enum Modes {


This enum seems like overkill. I think you could just have four static final fields, two for the filenames and two for the headers, and use those.

msridhar · 2022-02-24T00:53:13Z

nullaway/src/test/java/com/uber/nullaway/tools/SerializationTestHelper.java

  }

-  private void clearOutput() {
+  public SerializationTestHelper<T> setOutputFileName(String fileName, String header) {


Suggested change

public SerializationTestHelper<T> setOutputFileName(String fileName, String header) {

public SerializationTestHelper<T> setOutputFileNameAndHeader(String fileName, String header) {

nimakarimipour · 2022-02-24T01:17:47Z

@msridhar, resolved the comments. Ready for review.

msridhar · 2022-02-24T01:18:59Z

Adding @lazaroclapp in case he wants to review before landing

lazaroclapp

For the most part, this looks good, some comments below!

lazaroclapp · 2022-02-24T18:43:52Z

nullaway/src/test/java/com/uber/nullaway/tools/ErrorDisplay.java

+
+/**
+ * Helper class to represent a {@link com.uber.nullaway.fixserialization.out.ErrorInfo} contents in
+ * {@code String}.


"in String" or "in a test case's (expected or actual) output." ? (Similar fix for FixDispaly, although it wasn't introduced in this PR)

lazaroclapp · 2022-02-24T18:45:15Z

nullaway/src/test/java/com/uber/nullaway/tools/ErrorDisplay.java

+    }
+    ErrorDisplay that = (ErrorDisplay) o;
+    return type.equals(that.type)
+        && (message.contains(that.message) || that.message.contains(message))


Is there a reason for this double contains test as opposed to .equals(...) between the strings? What am I missing?

Yes, the reason is that actual complete error messages from NullAway can be really long and may make the tests less readable and may not be the primary intention of the test. For instance, an actual error reported by NullAway is:
initializer method does not guarantee @NonNull field foo (line 4) is initialized along all control-flow paths (remember to check for exceptions or early returns).
Which with this approach it is simply written in the test as initializer method does not guarantee @NonNull field foo.
This is very similar to // BUG: Diagnostic contains: approach. Please let me know if you rather to see the complete error message.

Ah, makes sense to me. I think I was thinking this was an &&, but it's basically if either message is a substring of the other. Makes sense! Maybe worth a quick comment on that line.

lazaroclapp · 2022-02-24T18:49:20Z

nullaway/src/test/java/com/uber/nullaway/tools/SerializationTestHelper.java

+public class SerializationTestHelper<T extends Display> {
+
+  private final Path outputDir;
+  private List<T> expectedOutputs;


Any reason for this being a mutable list rather than ImmutableList?

Note: I get why actualOutputs is mutable, but we don't ever change expectedOutputs (which makes sense), so it probably should be immutable.

lazaroclapp · 2022-02-24T18:52:54Z

nullaway/src/test/java/com/uber/nullaway/tools/SerializationTestHelper.java

          .append("\n");
    }
    fail(errorMessage.toString());
  }

-  private FixDisplay[] readOutputFixes() {
-    ArrayList<FixDisplay> fixDisplays = new ArrayList<>();
+  private List<T> readActualOutputs(Path output, String header) {


The name of the argument output refers to the file path, the name outputs refers to individual items of type T found in the output of the test case. This is confusing (it is easy, when reading the code below, to fall into the trap of thinking than output is an element of outputs). I'd actually recommend this argument keep the older name of outputPath to distinguish it from the actual outputs.

Yes, sure. Thank you very much for noting that.

112e88e

lazaroclapp · 2022-02-24T18:53:59Z

nullaway/src/test/java/com/uber/nullaway/tools/SerializationTestHelper.java

    }
+    compilationTestHelper.doTest();
+    List<T> actualOutputs = readActualOutputs(outputPath, header);


Why is header passed here, if it's already a field of this?

Oh sorry, it was left from old codes.
c10a7d2

lazaroclapp · 2022-02-24T18:55:36Z

nullaway/src/test/java/com/uber/nullaway/tools/SerializationTestHelper.java

-        // paths are getting compared.
-        fixDisplay.uri = fixDisplay.uri.substring(fixDisplay.uri.indexOf("com/uber/"));
-        fixDisplays.add(fixDisplay);
+        T outputInstance = factory.fromValuesInString(line.split("\\t"));


If the argument is renamed outputPath, then it would make a lot of sense for this to be T output.

lazaroclapp · 2022-02-24T19:03:08Z

nullaway/src/test/java/com/uber/nullaway/NullAwaySerializationTest.java

        .doTest();
  }

  @Test
-  public void test_custom_annot() {
+  public void SuggestCustomAnnotTest() {


This is a method, name should start with lowercase.

lazaroclapp · 2022-02-24T19:04:25Z

nullaway/src/test/java/com/uber/nullaway/NullAwaySerializationTest.java

        .doTest();
  }

  @Test
-  public void test_method_param_protection_test() {
+  public void ExamineMethodParamProtectionTest() {


(See comment above: lowercase first letter)

lazaroclapp · 2022-02-24T19:15:12Z

nullaway/src/test/java/com/uber/nullaway/NullAwaySerializationTest.java

  private String configPath;
+  private Path root;


@msridhar The output files these test read are controlled by the path passed when constructing the SerializationTestHelper<> object plus the file passed to .expectedOutputs(...). For most of these tests, that is ${this.root}/${this.SUGGEST_FIX_FILE_NAME}.

That means these tests can't ever run in parallel. Just out of curiosity, is there any guarantee for gradle/JUnit such as: different test classes run in parallel, but test cases within the same @RunWith annotated class run serially?

That would make sense to me, and would mean this is correct. But I can't find no such guarantee in the gradle parallel testing docs, just a warning that:

When using parallel test execution, make sure your tests are properly isolated from one another. Tests that interact with the filesystem are particularly prone to conflict, causing intermittent test failures.

https://docs.gradle.org/current/userguide/java_testing.html

@lazaroclapp AFAIK Gradle will not run test methods within the same test class in parallel with each other. It only runs test methods from different test classes in parallel. This could cause a problem down the road if we split this test into multiple tests I guess. Could we fix by using a unique temp (sub-)directory root in every test?

This would work even if we split the test into multiple classes, because each such class will likely have its own private Path root instance. My only worry here is if there is some aggressively parallel JUnit+Gradle mode that runs a unit tests within the same test class in parallel, if you don't think that's likely (and it would break a lot of setUp(...) methods I've seen), then this is probably fine as it is.

I am assuming the latest decision is to leave this as it is. In that case this is ready for another round of review.

Looking more, I think the way @Rule works, the temporary folder should already get created fresh for each test and deleted afterward. Due to how these things work, I don't think any test runner would ever attempt to run tests within a class in parallel. So I think this code is fine as-is. That said, I did find another change we should make that I will comment on separately.

lazaroclapp · 2022-02-24T21:49:20Z

nullaway/src/test/java/com/uber/nullaway/tools/SerializationTestHelper.java

@@ -59,12 +59,12 @@ public SerializationTestHelper(Path outputDir) {

  @SafeVarargs
  public final SerializationTestHelper<T> setExpectedOutputs(T... outputs) {
-    this.expectedOutputs = Arrays.asList(outputs);
+    this.expectedOutputs = ImmutableList.copyOf(Arrays.asList(outputs));


You can directly pass the outputs array to copyOf(...). There is a version of that method that takes an array and this avoids the double list construction.

Oh, great. Looks much better.

aaf7d47

msridhar · 2022-02-24T22:49:28Z

nullaway/src/test/java/com/uber/nullaway/NullAwaySerializationTest.java

@@ -70,8 +102,9 @@ public void cleanup() {
  }


This cleanup() method should be deleted. JUnit will already take care of deleting the temporaryFolder after each test.

Oh great, Deleted it.

7587be4

lazaroclapp

LGTM!

nimakarimipour added 3 commits February 22, 2022 19:07

Init

75710f2

Added Factory class

2b786e9

Completed

d947ec3

nimakarimipour requested a review from msridhar February 23, 2022 03:58

nimakarimipour mentioned this pull request Feb 23, 2022

Field initialization serialization #576

Merged

msridhar requested changes Feb 23, 2022

View reviewed changes

nimakarimipour and others added 6 commits February 23, 2022 14:33

Update nullaway/src/test/java/com/uber/nullaway/tools/Factory.java

3daa5b0

Co-authored-by: Manu Sridharan <[email protected]>

Renamed

f9053d6

Added marker interface

772011d

Removed extra new line

340802a

Removed extra new line

3e12e6b

Renamed

10a3305

nimakarimipour requested a review from msridhar February 23, 2022 23:27

Renamed tests

d719ff9

msridhar requested changes Feb 24, 2022

View reviewed changes

nimakarimipour added 2 commits February 23, 2022 17:02

Renamed method

ac0ca35

Removed Enum

8d021ba

nimakarimipour requested a review from msridhar February 24, 2022 01:17

msridhar approved these changes Feb 24, 2022

View reviewed changes

msridhar requested a review from lazaroclapp February 24, 2022 01:18

lazaroclapp reviewed Feb 24, 2022

View reviewed changes

nimakarimipour added 3 commits February 24, 2022 13:09

Updated javadoc

2b94867

Converted to ImmutableList

08f16ce

Renamed

112e88e

lazaroclapp reviewed Feb 24, 2022

View reviewed changes

nimakarimipour added 2 commits February 24, 2022 13:52

Removed redudant method arg

c10a7d2

Renamed

75a1a70

nimakarimipour added 3 commits February 24, 2022 13:58

Renamed

ac2ec38

Passed array directly to make ImmutableList

aaf7d47

Added comment

9631abf

nimakarimipour requested a review from lazaroclapp February 24, 2022 22:11

msridhar reviewed Feb 24, 2022

View reviewed changes

Deleted cleanup

7587be4

lazaroclapp approved these changes Feb 25, 2022

View reviewed changes

msridhar merged commit 743e2b6 into uber:master Feb 25, 2022

nimakarimipour deleted the serialization_test branch March 21, 2022 23:56

	public class SerializationTestHelper<T> {
	public class SerializationTestHelper<T extends Display> {

	public SerializationTestHelper<T> setOutputFileName(String fileName, String header) {
	public SerializationTestHelper<T> setOutputFileNameAndHeader(String fileName, String header) {

Enhanced Serialization Test Infrastructure #579

Enhanced Serialization Test Infrastructure #579

Conversation

nimakarimipour commented Feb 23, 2022 • edited Loading

msridhar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nimakarimipour commented Feb 23, 2022

msridhar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nimakarimipour commented Feb 24, 2022

msridhar commented Feb 24, 2022

lazaroclapp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lazaroclapp left a comment

Choose a reason for hiding this comment

nimakarimipour commented Feb 23, 2022 •

edited

Loading