Skip to content

deps: prepare to bump JUnit to 4.13 - #7068

Closed
elharo wants to merge 24 commits into
grpc:masterfrom
elharo:guava
Closed

deps: prepare to bump JUnit to 4.13#7068
elharo wants to merge 24 commits into
grpc:masterfrom
elharo:guava

Conversation

@elharo

@elharo elharo commented May 27, 2020

Copy link
Copy Markdown
Contributor

@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
Copy Markdown
Contributor

@elharo Would you please fix failed test?

@elharo elharo left a comment

Copy link
Copy Markdown
Contributor Author

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

@elharo elharo left a comment

Copy link
Copy Markdown
Contributor Author

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?

@dapengzhang0 dapengzhang0 left a comment

Copy link
Copy Markdown
Contributor

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.

@elharo elharo left a comment

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor

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

dapengzhang0 commented May 29, 2020

Copy link
Copy Markdown
Contributor

Also, why do we depend on guava-testlib?

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

@dapengzhang0

Copy link
Copy Markdown
Contributor

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

@elharo

elharo commented Jun 1, 2020

Copy link
Copy Markdown
Contributor Author

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.

@elharo elharo left a comment

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor

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

ejona86 commented Sep 28, 2020

Copy link
Copy Markdown
Member

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

@ejona86

ejona86 commented Sep 28, 2020

Copy link
Copy Markdown
Member

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.

4 participants