-
Notifications
You must be signed in to change notification settings - Fork 29
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
Upgrade mockito now that the minimum Java is 11 on CI #247
base: master
Are you sure you want to change the base?
Conversation
TWiStErRob
commented
Jul 14, 2023
•
edited
Loading
edited
- Closes fix(deps): update dependency org.mockito:mockito-junit-jupiter to v5 #198
@@ -89,9 +89,7 @@ dependencies { | |||
testImplementation("com.github.tomakehurst:wiremock:2.27.2") | |||
testImplementation("ru.lanwen.wiremock:wiremock-junit5:1.3.1") | |||
testImplementation("org.assertj:assertj-core:3.24.2") | |||
// This cannot be updated to 5.x as it requires Java 11, | |||
// but we are running CI on Java 8 in .github/workflows/java-versions.yml. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(btw, shouldn't Java 8 CI still exist considering the target bytecode is 8?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've dropped the Java 8 CI builds to speed up the process after #171 is applied. Java <11 is unsupported for building 2.0.0+ (and with that MR merged it will effectively be enforced), so I'm not sure, if we need it. We probably could have some "smoke tests" trying to execute Gradle build with Java 8 for the artifacts build and "released" with JDK 17, but it is also not supported, so we might wait for people to complaint about any runtime regression in their projects. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would make sense to build using 17, and and target bytecode 8. Best of both worlds and the effort should be really low. Tests can still run on 11 or 17. I just queued up a similar PR in Mockito: TWiStErRob/mockito#1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, for now since 8 is in a "dropped" state, I think this PR can go ahead. It should only affect unit tests. I guess compatTests don't use mocks?
As discussed with @TWiStErRob, it would be beneficial to have a possibility to build the project with JDK 17 and run functional tests with JDK 8 on CI server. However, it could be done in a separate PR. We could also think, if - to prevent some |