Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test fixes #173

Merged
merged 10 commits into from
Apr 17, 2024
Merged

Test fixes #173

merged 10 commits into from
Apr 17, 2024

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.

src/test/java/tests/DetailMessage.java Outdated Show resolved Hide resolved
@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.

@@ -62,7 +62,7 @@ final class DetailMessage extends TestDiagnostic {
*/
static DetailMessage parse(TestDiagnostic input, @Nullable Path rootDirectory) {
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
1 of 3 checks passed
@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.

3 participants