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
deps: prepare to bump JUnit to 4.13 #7068
Conversation
merge from origin
@elharo Would you please fix failed test? |
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.
PTAL. Errors I found in the logs seemed to all be deprecated classes. Holler if you see any other errors I missed.
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.
@dapengzhang0 This is proving surprisingly large. Can you please look at the kokoro results and confirm that the deprecations are the problem and that these need to be fixed in this PR?
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.
guava-testlib v29 transitively depends on junit 4.13. In the codebase there are so many deprecations introduced by junit 4.13. Fixing that is invasive. Please revert the junit test fixes, and exclude junit dependency from guava-testlib instead:
testImplementation (libraries.guava_testlib) {
exclude group: 'junit', module: 'junit'
}
Fixing deprecated usage of junit (optional) should be in another PR.
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 don't want to revert all this work. We can have a new PR to update Guava and then use this to update JUnit.
Also, why do we depend on guava-testlib?
I'm okay for a new PR. But the junit fix in this PR still has some problems. |
Some unit tests are using some utilities in guava-testlib. |
@elharo We may not want to upgrade junit to 4.13 as of now, since internally we still use 4.12. |
google-cloud-java and the rest of the open source GCP Java projects have pretty much already all moved to JUnit 4.13. We're not using anything newly introduced in 4.13 so internal google repos don't block this. |
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.
@dapengzhang0 This no longer updates dependencies. However it does update a much (though not all) code that will become deprecated once we move to JUnit 4.13.
Had a discussion with @ejona86 @voidzcy @sanjaypujare , we would like to keep the existing |
I've created #7468 as an alternative to this PR. |
Closing in favor of #7468. Much less invasive. It'll be good when we can swap to assertThrows when it is convenient. Thank you for this PR, even if it didn't get merged. I know it was quite annoying to swap to the try-fail approach. #7079 helped as well; I didn't have any conflicts when I bumped the junit version for testing. |
@dapengzhang0