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

Use published conformance tests #126

Merged
merged 42 commits into from
Jan 9, 2024

Conversation

netdpb
Copy link
Collaborator

@netdpb netdpb commented Jan 2, 2024

Use the published JSpecify artifacts instead of requiring a local clone of https://github.com/jspecify/jspecify. Allow the user to include a local clone with --included-build path/to/jspecify, and use those artifacts instead in that case.

Requires jspecify/jspecify#446.

Part of #107.

@netdpb netdpb added the conformance-tests Framework and implementation for conformance tests label Jan 2, 2024
@netdpb netdpb requested review from cpovirk and wmdietl January 2, 2024 22:16
@netdpb
Copy link
Collaborator Author

netdpb commented Jan 2, 2024

I need to update development docs too. Basically, if you don't need to make coordinated changes to the reference checker and jspecify or the conformance tests at the same time, you don't have to clone jspecify at all. Otherwise, clone jspecify and build with --include-build path/to/jspecify and it'll use the local build for jspecify and the conformance tests.

@netdpb
Copy link
Collaborator Author

netdpb commented Jan 2, 2024

Right now the CI downloads JSpecify 0.3.0 and the latest snapshot of the tests from Maven. I should change the CI to keep the current behavior of using JSpecify HEAD.

Copy link
Collaborator

@wmdietl wmdietl left a comment

Choose a reason for hiding this comment

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

I assume CI fails because jspecify/jspecify#446 wasn't executed yet?

settings.gradle Outdated
@@ -33,8 +25,9 @@ dependencyResolutionManagement {
library("errorProne-core", "com.google.errorprone:error_prone_core:2.18.0")
library("errorProne-javac", "com.google.errorprone:javac:9+181-r4173-1")
library("guava", "com.google.guava:guava:31.1-jre")
library("jspecify", "org.jspecify:jspecify:0.0.0-SNAPSHOT")
library("jspecify", "org.jspecify:jspecify:0.3.0")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a bit confused by the different versions used... demo uses jspecify-0.0.0-SNAPSHOT.jar and the conformance artifacts below use 0.0.0-SNAPSHOT. Why is this now 0.3.0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm going to document this somewhere in the repo as well, but here's what I'm thinking:

The reference checker has the following dependencies:

  • on jspecify for its implementation: some of its own APIs are annotated
  • on the conformance test suite ZIP file
  • on the conformance test framework library

Those would also be dependencies of any JSpecify-compliant tool, although the first one is optional: they don't have to annotate their own code.

For each of those dependencies, I want developers to be able to depend on either a published version (snapshot or not) or a local build.

I expect the version numbers of those three things, once they're published, to vary independently. We'd update the test suite version every time we change or add a test case; the test framework library only when that changes; and jspecify itself very rarely.

I want to publish snapshot versions of the test suite and the test framework so that by default the reference checker depends on those for its tests. That puts the reference checker on the same footing as any other tool, which I would expect to use (probably non-snapshot) published versions of both.

So the idea will be, eventually, that by default the reference checker uses published snapshots of both, but there's a simple way to use a local clone (at whatever commit) for either one. Then our CI would use a local clone of the main branch for both in order to retain the current behavior.

That's what I'm trying to do now (but I'm not done) for the conformance test suite and jspecify: If you want the local build, you --include-build path/to/jspecify and those get substituted no matter what version is specified here. Otherwise, we'll use jspecify:0.3.0 (because why shouldn't we?) and conformance-tests-0.0.0-SNAPSHOT (which is published already, although only manually for now).

@netdpb
Copy link
Collaborator Author

netdpb commented Jan 4, 2024

OK, I think this is ready to be re-reviewed.

@netdpb netdpb requested a review from wmdietl January 4, 2024 23:18
Copy link
Collaborator

@wmdietl wmdietl left a comment

Choose a reason for hiding this comment

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

Thanks for all the documentation additions! The changes look good to me.

I tried the PR locally, in an empty directory doing:

git clone [email protected]:jspecify/jspecify-reference-checker.git
cd jspecify-reference-checker/
gh pr checkout 126
./gradlew build

I would have expected this to pass, like it does on CI.
However, this results in:

tests.ConformanceTest > conformanceTests FAILED
    diff (-expected +actual):
        @@ -1,4 +1,4 @@
        -# 12 pass; 7 fail; 19 total; 63.2% score
        +# 10 pass; 9 fail; 19 total; 52.6% score
         PASS: Basic.java:28 test:expression-type:Object?:nullable
         PASS: Basic.java:28 test:sink-type:Object!:return
         PASS: Basic.java:28 test:cannot-convert:Object? to Object!
        @@ -16,6 +16,6 @@
         FAIL: Irrelevant.java:47 test:irrelevant-annotation:NullMarked
         FAIL: Irrelevant.java:49 test:irrelevant-annotation:NullUnmarked
         FAIL: Irrelevant.java: no unexpected facts
        -PASS: UsesDep.java:24 test:cannot-convert:null? to Dep*
        -PASS: UsesDep.java: no unexpected facts
        +FAIL: UsesDep.java:24 test:cannot-convert:null? to Dep*
        +FAIL: UsesDep.java: no unexpected facts

What does the PASS vs FAIL output mean? Does it not get the expected output?
Can you reproduce this?

@wmdietl
Copy link
Collaborator

wmdietl commented Jan 5, 2024

I would have expected this to pass, like it does on CI.

The difference to the CI build is that I'm not using the --include-build ../jspecify part. But I think this is supposed to work without that argument, right?

@wmdietl
Copy link
Collaborator

wmdietl commented Jan 5, 2024

@netdpb I PRed your PR netdpb#9. This doesn't fix the problem above, just makes the demo script easier to use.

@netdpb
Copy link
Collaborator Author

netdpb commented Jan 8, 2024

@netdpb I PRed your PR netdpb#9. This doesn't fix the problem above, just makes the demo script easier to use.

Oooh, fancy! 🎩

@netdpb
Copy link
Collaborator Author

netdpb commented Jan 8, 2024

@wmdietl, which version of Java are you using? When I run with Java 11, I see the same failure you do. When I run with Java 17 (like the CI, although not the identical distro), it passes.

I have no idea why.

@netdpb
Copy link
Collaborator Author

netdpb commented Jan 8, 2024

Aha! I found it.

      bad class file: .../jspecify/jspecify-reference-checker/build/conformanceTests/deps/conformance-tests-0.0.0-SNAPSHOT-deps.jar(/org/jspecify/conformance/deps/Dep.class)
        class file has wrong version 61.0, should be 55.0
        Please remove or make sure it appears in the correct subdirectory of the classpath.

With which Java version should we publish the conformance tests?

@netdpb
Copy link
Collaborator Author

netdpb commented Jan 8, 2024

I'm going with Java 8, just like the annotations. See jspecify/jspecify#451.

@netdpb
Copy link
Collaborator Author

netdpb commented Jan 8, 2024

@wmdietl I think the problem is now fixed. I pasted those commands in while in an empty directory, and it passed. (You might have to refresh your Gradle dependencies so that it picks up the newest snapshot version.)

Let me know if it works for you.

@netdpb netdpb requested a review from wmdietl January 8, 2024 22:57
Copy link
Collaborator

@wmdietl wmdietl left a comment

Choose a reason for hiding this comment

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

I've been using Java 11, which explains that failure. Thanks for making it work for Java 8+.
I've just run the conformance tests again and things are working correctly now.

@netdpb netdpb merged commit ec180c5 into jspecify:main Jan 9, 2024
2 checks passed
@netdpb netdpb deleted the publish-conformance-tests branch January 9, 2024 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conformance-tests Framework and implementation for conformance tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants