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

test: cleaning up deprecated usages #858

Merged
merged 2 commits into from Apr 10, 2020

Conversation

rahulKQL
Copy link
Contributor

@rahulKQL rahulKQL commented Jan 23, 2020

Fixes #857

removed deprecated ExpectedException.none() usage across unit test cases.

@googlebot googlebot added the cla: yes label Jan 23, 2020
@codecov
Copy link

@codecov codecov bot commented Jan 23, 2020

Codecov Report

Merging #858 into master will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #858   +/-   ##
=========================================
  Coverage     78.66%   78.67%           
  Complexity     1167     1167           
=========================================
  Files           204      204           
  Lines          5152     5154    +2     
  Branches        413      413           
=========================================
+ Hits           4053     4055    +2     
  Misses          925      925           
  Partials        174      174           
Impacted Files Coverage Δ Complexity Δ
...java/com/google/api/gax/rpc/UnaryCallSettings.java 100.00% <0.00%> (ø) 5.00% <0.00%> (ø%)
...oogle/api/gax/rpc/ServerStreamingCallSettings.java 67.85% <0.00%> (+0.58%) 6.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a676891...a65b19f. Read the comment docs.

@rahulKQL rahulKQL requested review from igorbernstein2 and kolea2 Jan 23, 2020
@rahulKQL rahulKQL requested a review from elharo Feb 3, 2020
Copy link
Contributor

@elharo elharo left a comment

You can catch more specific types than java.lang.Exception for more precise tests here.

elharo
elharo approved these changes Feb 3, 2020
Copy link
Contributor

@elharo elharo left a comment

noit sure why this failed:

FAILURE: Build failed with an exception.

  • What went wrong:
    A problem occurred configuring root project 'gax-java'.

Could not resolve all artifacts for configuration ':classpath'.
Could not resolve net.ltgt.gradle:gradle-apt-plugin:0.10.
Required by:
project :
> Could not resolve net.ltgt.gradle:gradle-apt-plugin:0.10.
> Could not get resource 'https://plugins.gradle.org/m2/net/ltgt/gradle/gradle-apt-plugin/0.10/gradle-apt-plugin-0.10.pom'.
> Could not GET 'https://plugins.gradle.org/m2/net/ltgt/gradle/gradle-apt-plugin/0.10/gradle-apt-plugin-0.10.pom'.
> sun.security.validator.ValidatorException: PKIX path validation failed: java.security.cert.CertPathValidatorException: signature check failed
Could not resolve gradle.plugin.com.dorongold.plugins:task-tree:1.3.1.
Required by:
project :
> Could not resolve gradle.plugin.com.dorongold.plugins:task-tree:1.3.1.
> Could not get resource 'https://plugins.gradle.org/m2/gradle/plugin/com/dorongold/plugins/task-tree/1.3.1/task-tree-1.3.1.pom'.
> Could not GET 'https://plugins.gradle.org/m2/gradle/plugin/com/dorongold/plugins/task-tree/1.3.1/task-tree-1.3.1.pom'.
> sun.security.validator.ValidatorException: PKIX path validation failed: java.security.cert.CertPathValidatorException: signature check failed
Could not resolve gradle.plugin.com.github.sherter.google-java-format:google-java-format-gradle-plugin:0.6.
Required by:
project :
> Could not resolve gradle.plugin.com.github.sherter.google-java-format:google-java-format-gradle-plugin:0.6.
> Could not get resource 'https://plugins.gradle.org/m2/gradle/plugin/com/github/sherter/google-java-format/google-java-format-gradle-plugin/0.6/google-java-format-gradle-plugin-0.6.pom'.
> Could not GET 'https://plugins.gradle.org/m2/gradle/plugin/com/github/sherter/google-java-format/google-java-format-gradle-plugin/0.6/google-java-format-gradle-plugin-0.6.pom'.
> sun.security.validator.ValidatorException: PKIX path validation failed: java.security.cert.CertPathValidatorException: signature check failed

@rahulKQL rahulKQL added the kokoro:force-run label Feb 4, 2020
@kokoro-team kokoro-team removed the kokoro:force-run label Feb 4, 2020
try {
callable.call(1);
Assert.fail("Callable should have thrown an exception");
} catch (Exception expected) {
Copy link
Contributor

@elharo elharo Feb 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RuntimeException or something more specific. Other exceptions can still cause the test to fail.

@rahulKQL rahulKQL added kokoro:run kokoro:force-run and removed kokoro:run labels Feb 27, 2020
@kokoro-team kokoro-team removed the kokoro:force-run label Feb 27, 2020
@rahulKQL rahulKQL added the kokoro:force-run label Feb 28, 2020
@kokoro-team kokoro-team removed the kokoro:force-run label Feb 28, 2020
removed deprecated `ExpectedException.none()` usage across unit test cases.

chore: address feedback comments
@igorbernstein2 igorbernstein2 requested review from vam-google and removed request for igorbernstein2 Apr 2, 2020
@rahulKQL rahulKQL added the kokoro:force-run label Apr 8, 2020
@kokoro-team kokoro-team removed the kokoro:force-run label Apr 8, 2020
@rahulKQL
Copy link
Contributor Author

@rahulKQL rahulKQL commented Apr 8, 2020

@elharo I am really sorry for this stale PR. Until last week it's Java 7 presubmit job was failing, finally its green now.

Would you please have another look, I addressed your previous feedback comments.

@rahulKQL rahulKQL requested a review from elharo Apr 8, 2020
@@ -29,27 +29,26 @@
*/
package com.google.api.gax.grpc;

import static com.google.common.truth.Truth.assertThat;
Copy link
Contributor

@elharo elharo Apr 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't static import this since there are at least three different assertThat's this could be

Copy link
Collaborator

@igorbernstein2 igorbernstein2 Apr 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this the recommended way to use Truth?
https://truth.dev/#2-add-static-imports-for-truths-entry-points

Copy link
Contributor

@elharo elharo Apr 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actively disagree with that recommendation. I like to know where my methods come from. In this case I was about to send you a completely wrong review because I assumed it was a different assertThat() method than it actually was. As I said, there are at least three in common use. Removing ambiguity is important for readability.

Copy link
Contributor Author

@rahulKQL rahulKQL Apr 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elharo Thanks for the review!!

I have removed static Truth.assertThat import introduced in this PR, Please have another look.

@@ -29,6 +29,8 @@
*/
package com.google.api.gax.httpjson;

import static com.google.common.truth.Truth.assertThat;
Copy link
Contributor

@elharo elharo Apr 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't static import this since there are at least three different assertThat's this could be

@@ -29,28 +29,25 @@
*/
package com.google.api.gax.rpc;

import com.google.common.truth.Truth;
import static com.google.common.truth.Truth.assertThat;
Copy link
Contributor

@elharo elharo Apr 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't static import this since there are at least three different assertThat's this could be

@@ -29,6 +29,8 @@
*/
package com.google.api.gax.rpc;

import static com.google.common.truth.Truth.assertThat;
Copy link
Contributor

@elharo elharo Apr 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't static import this since there are at least three different assertThat's this could be

@rahulKQL rahulKQL requested a review from elharo Apr 9, 2020
elharo
elharo approved these changes Apr 9, 2020
Copy link
Contributor

@vam-google vam-google left a comment

LGTM

@rahulKQL rahulKQL merged commit b988b95 into googleapis:master Apr 10, 2020
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants