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

deps: prepare to bump JUnit to 4.13 #7068

Closed
wants to merge 24 commits into from
Closed

deps: prepare to bump JUnit to 4.13 #7068

wants to merge 24 commits into from

Conversation

elharo
Copy link
Contributor

@elharo elharo commented May 27, 2020

@dapengzhang0 dapengzhang0 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 27, 2020
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 27, 2020
@dapengzhang0
Copy link
Member

@elharo Would you please fix failed test?

Copy link
Contributor Author

@elharo elharo left a 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.

@dapengzhang0 dapengzhang0 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 28, 2020
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 28, 2020
Copy link
Contributor Author

@elharo elharo left a 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?

Copy link
Member

@dapengzhang0 dapengzhang0 left a 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.

Copy link
Contributor Author

@elharo elharo left a 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?

@elharo elharo changed the title deps: bump Guava to 29.0-android deps: bump JUnit to 4.13 May 29, 2020
@elharo elharo changed the title deps: bump JUnit to 4.13 deps: prepare to bump JUnit to 4.13 May 29, 2020
@dapengzhang0
Copy link
Member

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.

I'm okay for a new PR. But the junit fix in this PR still has some problems.

@dapengzhang0
Copy link
Member

dapengzhang0 commented May 29, 2020

Also, why do we depend on guava-testlib?

Some unit tests are using some utilities in guava-testlib.

@dapengzhang0
Copy link
Member

@elharo We may not want to upgrade junit to 4.13 as of now, since internally we still use 4.12.

@elharo
Copy link
Contributor Author

elharo commented Jun 1, 2020

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.

Copy link
Contributor Author

@elharo elharo left a 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.

@dapengzhang0
Copy link
Member

Had a discussion with @ejona86 @voidzcy @sanjaypujare , we would like to keep the existing ExpectedException unchanged and only suppress the checker by adding @SuppressWarnings("ExpectedExceptionChecker"). In the future, when grpc migrates to Java 8, we can replace them with assertThrows() as recommended by the deprecation javadoc.

@ejona86
Copy link
Member

ejona86 commented Sep 28, 2020

I've created #7468 as an alternative to this PR.

@ejona86
Copy link
Member

ejona86 commented Sep 28, 2020

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.

@ejona86 ejona86 closed this Sep 28, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants