Skip to content

Test that NullMarked.class.getAnnotations() really does fail under JDK8.#270

Merged
cpovirk merged 1 commit intojspecify:mainfrom
cpovirk:testfail8
Aug 22, 2022
Merged

Test that NullMarked.class.getAnnotations() really does fail under JDK8.#270
cpovirk merged 1 commit intojspecify:mainfrom
cpovirk:testfail8

Conversation

@cpovirk
Copy link
Copy Markdown
Collaborator

@cpovirk cpovirk commented Aug 22, 2022

No description provided.

@cpovirk
Copy link
Copy Markdown
Collaborator Author

cpovirk commented Aug 22, 2022

OK, I was wondering if this couldn't actually work right in CI, since we don't request JDK 8. But it looks like we request the latest Ubuntu release, which currently is 22.04, which has JDK 8 by default.

It would be more hygienic for us to request it explicitly. But for now, everything works.

@cpovirk cpovirk merged commit 498e587 into jspecify:main Aug 22, 2022
@cpovirk cpovirk deleted the testfail8 branch August 22, 2022 21:58
@msridhar
Copy link
Copy Markdown
Collaborator

It's not clear to me this new test is even running in CI, since we never use JDK 8. Do you want to do a limited set of tests on JDK 8 in CI?

@cpovirk
Copy link
Copy Markdown
Collaborator Author

cpovirk commented Aug 22, 2022

Thanks, I'll investigate further, probably by sending a PR that should fall the JDK 8 test. (I do see the Java 8 integration test show up in the Gradle output, but I don't know if the toolchains magic responds to a missing JDK by quietly substituting another JDK or by failing the build. I'd aimed to test that with this PR, but I didn't think the "quiet substitution" scenario through: In that case, I think we'd quietly run the Java 9+ tests twice and the Java 8 tests zero times. So this PR doesn't distinguish between that and actually testing insert JDK 8.)

@cpovirk
Copy link
Copy Markdown
Collaborator Author

cpovirk commented Aug 23, 2022

Hooray for the failure that I was aiming for:

@NullMarked > with Java 8 > basicReflectionDoesNotThrowException() FAILED
    org.opentest4j.AssertionFailedError at NullMarkedTest.java:66

@msridhar
Copy link
Copy Markdown
Collaborator

Sorry for the confusion on my end!

@cpovirk
Copy link
Copy Markdown
Collaborator Author

cpovirk commented Aug 23, 2022

No problem at all. It was the kind of test I'd been thinking I'd done originally, but I hadn't!

cpovirk added a commit that referenced this pull request May 24, 2024
- Upgrade Spotless to upgrade google-java-format.
- Make nested test classes `static` so that we don't end up with
  [synthetic parameters that upset our JDK-8 integration
  test](https://bugs.openjdk.org/browse/JDK-8058322).
  (https://errorprone.info/bugpattern/ClassCanBeStatic!)

Thanks to cushon@ for the pointers.

While there, rename a test that I should have renamed as part of
#270.

And update our CI versions to include JDK 21. (I haven't added JDK 22,
as we've seen some potential signs of remaining flakiness there.)
cpovirk added a commit that referenced this pull request Jun 24, 2024
- Upgrade Spotless to upgrade google-java-format.
- Make nested test classes `static` so that we don't end up with
  [synthetic parameters that upset our JDK-8 integration
  test](https://bugs.openjdk.org/browse/JDK-8058322).
  (https://errorprone.info/bugpattern/ClassCanBeStatic!)

Thanks to cushon@ for the pointers.

While there, rename a test that I should have renamed as part of
#270.

And update our CI versions to include JDK 21. (I haven't added JDK 22,
as we've seen some potential signs of remaining flakiness there.)
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