jspecify / jspecify-reference-checker

The reference implementation for the JSpecify nullness specification (and later, its other specifications as well)
https://jspecify.org
Apache License 2.0
27 stars 7 forks source link

Test fixes #173

Closed wmdietl closed 7 months ago

wmdietl commented 7 months ago

CI fails because of #159. This PR fixes the test setup to actually highlight that failure.

wmdietl commented 7 months ago

@netdpb The conformance tests suddenly accept DetailMessages with compilation errors. I verified that the compilation errors are reported in analyze, with kind error. I haven't tracked down where they are filtered out. I don't think https://github.com/eisop/checker-framework/pull/739 is the problem, but that's a possibility.

./gradlew test fails in this PR, so it might be easier to run in #174.

There, you'll e.g. see that FAIL: AnnotatedReceiver.java: no unexpected facts changed into PASS: AnnotatedReceiver.java: no unexpected facts even though one sees the compilation failure when checking with demo.

cpovirk commented 7 months ago

It looks like the compile error may be getting reported under full path but that the conformance-test runner may be looking for errors only in the relative paths:

@@ -73,6 +75,10 @@ public final class ConformanceTestReport {
     report.format(
         "# %,d pass; %,d fail; %,d total; %.1f%% score%n",
         passes, fails, total, 100.0 * passes / total);
+    Set<Path> unknownFilesWithErrors = difference(reportedFactsByFile.keySet(), files);
+    if (!unknownFilesWithErrors.isEmpty()) {
+      throw new RuntimeException("unknown files with errors: " + unknownFilesWithErrors);
+    }
     for (Path file : files) {
       ImmutableListMultimap<Long, ExpectedFact> expectedFactsInFile =
           index(expectedFactsByFile.get(file), Fact::getLineNumber);
java.lang.RuntimeException: unknown files with errors: [, /tmp/tmp.b0K8g39UiV/jspecify-reference-checker/build/conformanceTests/samples/ContainmentExtends.java, /tmp/tmp.b0K8g39UiV/jspecify-reference-checker/build/conformanceTests/samples/TypeVariableUnionNullToObjectUnionNull.java, ...
wmdietl commented 7 months ago

Ah! I return the first DetailMessage before relativizing! I'll see whether that fixes things.

wmdietl commented 7 months ago

@cpovirk Thanks for tracking this down! Relativizing earlier fixes the issue. Should I file against the conformance test suite to make this more robust? Or is this working as intended?

netdpb commented 7 months ago

@cpovirk Thanks for tracking this down! Relativizing earlier fixes the issue. Should I file against the conformance test suite to make this more robust? Or is this working as intended?

I think the point of that rootDirectory parameter is to relativize. How would you make it more robust?

wmdietl commented 7 months ago

@cpovirk Thanks for tracking this down! Relativizing earlier fixes the issue. Should I file against the conformance test suite to make this more robust? Or is this working as intended?

I think the point of that rootDirectory parameter is to relativize. How would you make it more robust?

Output diagnostics even if they are not relative to the rootDirectory? When is it okay for unknownFilesWithErrors from Chris's comment to be non-empty?

netdpb commented 7 months ago

@cpovirk Thanks for tracking this down! Relativizing earlier fixes the issue. Should I file against the conformance test suite to make this more robust? Or is this working as intended?

I think the point of that rootDirectory parameter is to relativize. How would you make it more robust?

Output diagnostics even if they are not relative to the rootDirectory? When is it okay for unknownFilesWithErrors from Chris's comment to be non-empty?

It shouldn't ever be. Maybe unexpected errors in unexpected locations (or no location) should cause the runner to throw an exception? I don't want to put those in the test report.

Anyway, feel free to file a separate issue for what to do in that case.

wmdietl commented 7 months ago

@cpovirk Thanks for tracking this down! Relativizing earlier fixes the issue. Should I file against the conformance test suite to make this more robust? Or is this working as intended?

I think the point of that rootDirectory parameter is to relativize. How would you make it more robust?

Output diagnostics even if they are not relative to the rootDirectory? When is it okay for unknownFilesWithErrors from Chris's comment to be non-empty?

It shouldn't ever be. Maybe unexpected errors in unexpected locations (or no location) should cause the runner to throw an exception? I don't want to put those in the test report.

Anyway, feel free to file a separate issue for what to do in that case.

Filed #176.