Skip to content

Use standard error format - #154

Merged
wmdietl merged 4 commits into
main-eisopfrom
fix-test
Feb 8, 2024
Merged

wmdietl merged 4 commits into
main-eisopfrom
fix-test

Conversation

@wmdietl

@wmdietl wmdietl commented Feb 6, 2024

Copy link
Copy Markdown
Collaborator

In eisop/checker-framework#693 I removed some hacky workaround, not realizing that it is still used here.
As we're not planning to write many such tests, I think it's okay to just use the standard error format here.

Ideally, we would have a mechanism to allow type systems to more easily adapt the expected error lines themselves.
However, at the moment there is a bunch of static methods that make extension hard.

@wmdietl
wmdietl requested a review from netdpb February 6, 2024 22:20
@wmdietl

wmdietl commented Feb 6, 2024

Copy link
Copy Markdown
Collaborator Author

@cpovirk The samples-google-prototype also uses the special // jspecify format. Would you mind if I rewrote that as // :: error: jspecify? Hopefully, a simple sed script will be able to do that.

@wmdietl

wmdietl commented Feb 6, 2024

Copy link
Copy Markdown
Collaborator Author

@netdpb It took me a while to figure out why the conformance tests were not updating for me locally. Is it possible that you need to make a new release of org.jspecify.conformance:conformance-tests? I only managed to get the new tests when using a local clone and --include-build, which shouldn't be a requirement, right?

@netdpb

netdpb commented Feb 6, 2024

Copy link
Copy Markdown
Collaborator

If you change the samples to use the new format, does the build succeed? If so, I have no objection.

On the other hand, I think you'll need to change ExpectedFact.NULLNESS_MISMATCH as well.

@netdpb

netdpb commented Feb 6, 2024

Copy link
Copy Markdown
Collaborator

@netdpb It took me a while to figure out why the conformance tests were not updating for me locally. Is it possible that you need to make a new release of org.jspecify.conformance:conformance-tests? I only managed to get the new tests when using a local clone and --include-build, which shouldn't be a requirement, right?

You may have to run with --refresh-dependencies in order to get the latest copy of the tests. Normally Gradle caches snapshot versions for 24 hours.

@wmdietl

wmdietl commented Feb 6, 2024

Copy link
Copy Markdown
Collaborator Author

@netdpb It took me a while to figure out why the conformance tests were not updating for me locally. Is it possible that you need to make a new release of org.jspecify.conformance:conformance-tests? I only managed to get the new tests when using a local clone and --include-build, which shouldn't be a requirement, right?

You may have to run with --refresh-dependencies in order to get the latest copy of the tests. Normally Gradle caches snapshot versions for 24 hours.

I just ran ./gradlew --refresh-dependencies.
Afterwards, I still see different behavior between using --include-build or not, even though my local jspecify clone is up-to-date.

@wmdietl

wmdietl commented Feb 6, 2024

Copy link
Copy Markdown
Collaborator Author

On the other hand, I think you'll need to change ExpectedFact.NULLNESS_MISMATCH as well.

Isn't ExpectedFact just for the conformance test framework?
Are the files in samples-google-prototype processed as CF test cases or as conformance test files?

@wmdietl

wmdietl commented Feb 7, 2024

Copy link
Copy Markdown
Collaborator Author

I've created a new jspecify branch: https://github.com/jspecify/jspecify/tree/samples-google-prototype-eisop
And this commit starts using it.
If you look at the CI output before, you'll notice that none of the error markers for the jspecifySamplesTest target match.
With this new branch, the errors match again - they don't all pass, but we're back to many expected errors being found.

@netdpb

netdpb commented Feb 7, 2024

Copy link
Copy Markdown
Collaborator

On the other hand, I think you'll need to change ExpectedFact.NULLNESS_MISMATCH as well.

Isn't ExpectedFact just for the conformance test framework? Are the files in samples-google-prototype processed as CF test cases or as conformance test files?

Both. jspecifySamplesTest (which runs NullSpecTest) uses the CF test framework; conformanceTests (which runs ConformanceTest.conformanceTestOnSamples()) runs the conformance test framework on the samples. That's why ExpectedFact.NULLNESS_MISMATCH is there.

@netdpb

netdpb commented Feb 7, 2024

Copy link
Copy Markdown
Collaborator

@netdpb It took me a while to figure out why the conformance tests were not updating for me locally. Is it possible that you need to make a new release of org.jspecify.conformance:conformance-tests? I only managed to get the new tests when using a local clone and --include-build, which shouldn't be a requirement, right?

You may have to run with --refresh-dependencies in order to get the latest copy of the tests. Normally Gradle caches snapshot versions for 24 hours.

I just ran ./gradlew --refresh-dependencies. Afterwards, I still see different behavior between using --include-build or not, even though my local jspecify clone is up-to-date.

Which task did you run with --refresh-dependencies? You might have to clean when switching between including the jspecify build and not.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It would be nice to have a separate (previously merged) PR just to update the conformance test report, so this PR just has difference from that.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I split that off into #157 and now this PR is just the changes to adapt the error format.

@wmdietl

wmdietl commented Feb 8, 2024

Copy link
Copy Markdown
Collaborator Author

Conformance tests and minimal tests pass now.

@wmdietl
wmdietl merged commit e673ee8 into main-eisop Feb 8, 2024
@wmdietl
wmdietl deleted the fix-test branch February 8, 2024 00:24
wmdietl added a commit that referenced this pull request Apr 10, 2024
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.

2 participants