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

Fix: Error from no-content responses #304

Merged
merged 2 commits into from
Oct 21, 2020

Conversation

vyj7
Copy link
Contributor

@vyj7 vyj7 commented Sep 28, 2020

Problem: Logging an empty response was throwing IllegalArgumentException from EntityUtils
Solution: For an empty response log nothing by adding a Null check

Contribution Checklist

…ion from EntityUtils

solution: for empty response log "".
@codecov-commenter
Copy link

codecov-commenter commented Sep 28, 2020

Codecov Report

Merging #304 into master will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master     #304   +/-   ##
=========================================
  Coverage     90.00%   90.00%           
- Complexity     1438     1439    +1     
=========================================
  Files           249      249           
  Lines          3943     3943           
  Branches        191      192    +1     
=========================================
  Hits           3549     3549           
  Misses          343      343           
  Partials         51       51           
Impacted Files Coverage Δ Complexity Δ
...n/java/com/vonage/client/logging/LoggingUtils.java 93.33% <100.00%> (ø) 2.00 <0.00> (+1.00)

@yallen011 yallen011 linked an issue Sep 29, 2020 that may be closed by this pull request
@slorello89
Copy link
Contributor

Hi @vyj7 - this looks like it ought to fix the issue, however without a Unit Test there's no way to set up a regression to validate that it won't happen again in the future - can you please add a unit test to this PR to make sure it doesn't toss the exception anymore? Thanks

Copy link
Contributor

@slorello89 slorello89 left a comment

Choose a reason for hiding this comment

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

Missing Unit Tests

@vyj7
Copy link
Contributor Author

vyj7 commented Oct 5, 2020

@slorello89 I have added UT for LoggingUtils Class

@vyj7 vyj7 requested a review from slorello89 October 5, 2020 16:57
Copy link
Contributor

@slorello89 slorello89 left a comment

Choose a reason for hiding this comment

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

Outstanding, thank you for this PR - very helpful! Happy Hacktoberfest!

@slorello89
Copy link
Contributor

@yallen011 - I'll leave this for you to merge but it looks all good in my book!

@martyndavies martyndavies added the Hacktoberfest-accepted the PR is accepted as valid for Hacktoberfest label Oct 13, 2020
Copy link
Contributor

@yallen011 yallen011 left a comment

Choose a reason for hiding this comment

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

looks good to me. Thank you for contributing to the Vonage Java Server SDK and Hacktoberfest 👻 🎃

@yallen011 yallen011 merged commit 77dc56c into Vonage:master Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hacktoberfest Hacktoberfest-accepted the PR is accepted as valid for Hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error from no-content responses
5 participants