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

Log message as well #4964

Merged
merged 4 commits into from Aug 13, 2019

Conversation

@RoiEXLab
Copy link
Member

commented Aug 13, 2019

Overview

To debug https://forums.triplea-game.org/topic/1510/ further

Functional Changes

More logging.

Manual Testing Performed

None.

RoiEXLab added some commits Aug 13, 2019

throw new IllegalStateException("Forum responded with code " + code);
throw new IllegalStateException(String.format(
"Forum responded with code %s and message '%s'",
code, EntityUtils.toString(response.getEntity())));

This comment has been minimized.

Copy link
@DanVanAtta

DanVanAtta Aug 13, 2019

Member

Will entity always be non-null? Empty and error responses are certain to be not null?

This comment has been minimized.

Copy link
@DanVanAtta

DanVanAtta Aug 13, 2019

Member

Does response.toString() provide too much info?

This comment has been minimized.

Copy link
@RoiEXLab

RoiEXLab Aug 13, 2019

Author Member

I don't think response.toString() does provide any information. I might be wrong, but EntityUtils is what's used everywhere so I used the thing I know works.

The documentation is lacking a lot of details like if certain values can be null, but AFAIK getEntity() always returned a non-null value. If anything goes wrong with the request that might prevent creating an entity this will result in an exception and never reach this point of execution.

This comment has been minimized.

Copy link
@DanVanAtta

DanVanAtta Aug 13, 2019

Member

Looking through source code implementations, it looked like at least one implementation of Response did have a toString implementation and the other delegated back to that same implementation.

I don't think we can assume a non-null response.

@DanVanAtta

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

The 500 we are debugging, there is no info on the server about it?

@DanVanAtta

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

The following files had format violations:
game-core/src/main/java/games/strategy/engine/pbem/NodeBbForumPoster.java

@RoiEXLab

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

The 500 we are debugging, there is no info on the server about it?

The server logged that there's a 500 using a java client with java 8_144 and apache http components (user agent string), but obviously it doesn't log all the responses, otherwise the log wouldn't be lasting very long.
There isn't any stacktrace if that was your question.

@codecov-io

This comment has been minimized.

Copy link

commented Aug 13, 2019

Codecov Report

Merging #4964 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #4964      +/-   ##
============================================
- Coverage     24.09%   24.09%   -0.01%     
+ Complexity     6746     6745       -1     
============================================
  Files           986      986              
  Lines         77629    77633       +4     
  Branches      11635    11636       +1     
============================================
- Hits          18707    18706       -1     
- Misses        56780    56785       +5     
  Partials       2142     2142
Impacted Files Coverage Δ Complexity Δ
.../games/strategy/engine/pbem/NodeBbForumPoster.java 0% <0%> (ø) 0 <0> (ø) ⬇️
.../src/main/java/games/strategy/net/nio/Decoder.java 65.51% <0%> (-1.73%) 11% <0%> (-1%)

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 4a71534...38838ae. Read the comment docs.

@DanVanAtta
Copy link
Member

left a comment

Need to protect against a possible NPE or show it is really impossible. The code blows up if entity is null.

@RoiEXLab

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

Will do later this evening

@DanVanAtta

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

@RoiEXLab thanks, feel free to merge after you take either route of toString'ing response or adding a NPE check.

@RoiEXLab RoiEXLab merged commit 134fc65 into triplea-game:master Aug 13, 2019

2 checks passed

LGTM analysis: Java No new or fixed alerts
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@RoiEXLab RoiEXLab deleted the RoiEXLab:more-logging branch Aug 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.