Skip to content

Conversation

@wmdietl
Copy link
Collaborator

@wmdietl wmdietl commented Apr 14, 2024

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

@wmdietl
Copy link
Collaborator Author

wmdietl commented Apr 15, 2024

@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 eisop/checker-framework#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
Copy link
Collaborator

cpovirk commented Apr 16, 2024

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
Copy link
Collaborator Author

wmdietl commented Apr 16, 2024

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

@wmdietl
Copy link
Collaborator Author

wmdietl commented Apr 16, 2024

@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?

@wmdietl wmdietl requested a review from netdpb April 16, 2024 18:24
@netdpb
Copy link
Collaborator

netdpb commented Apr 16, 2024

@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
Copy link
Collaborator Author

wmdietl commented Apr 16, 2024

@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
Copy link
Collaborator

netdpb commented Apr 17, 2024

@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
Copy link
Collaborator Author

wmdietl commented Apr 17, 2024

@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.

Path file = input.getFile();
if (rootDirectory != null && !file.toString().equals("")) {
if (rootDirectory != null && file.startsWith(rootDirectory)) {
// Empty file strings cannot be relativized.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can remove this comment.

@wmdietl wmdietl merged commit 0c36cdf into main-eisop Apr 17, 2024
@wmdietl wmdietl deleted the test-fixes branch April 17, 2024 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants