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

731: Fix NPE in WebSocketClient if an error occurs during connection #783

Merged
merged 3 commits into from
Nov 13, 2018

Conversation

snazha-blkio
Copy link
Contributor

What does this PR do?

Addresses the case where WebSocketClient is used without a listener.

Where should the reviewer start?

WebSocketClient.

Why is it needed?

To safeguard against NPEs, and to address an issue raised via another PR: #731

@snazha-blkio snazha-blkio added this to the 4.0.0-alpha-2 milestone Nov 12, 2018
@snazha-blkio snazha-blkio self-assigned this Nov 12, 2018
@snazha-blkio snazha-blkio changed the title 731: Fix NPE in WebSocketClient when it is used directly without a li… 731: Fix NPE in WebSocketClient if an error occurs during connection Nov 12, 2018
Copy link
Contributor

@iikirilov iikirilov 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. Any test coming for this?

@snazha-blkio
Copy link
Contributor Author

Although WebSocketClientTest.java already tests the functionality I added a dedicated test for the case where the listener is not set.

@snazha-blkio snazha-blkio changed the base branch from master to release/4.0 November 13, 2018 11:52
…fix/731-npe-websocket-client

                                                                        # Conflicts:
                                                                        #	core/src/main/java/org/web3j/protocol/websocket/WebSocketClient.java
@snazha-blkio snazha-blkio merged commit d651afd into release/4.0 Nov 13, 2018
Copy link
Contributor

@iikirilov iikirilov left a comment

Choose a reason for hiding this comment

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

Awesome. You have some checkstyle errors - you can see them locally with ./gradlew check

@codecov
Copy link

codecov bot commented Nov 13, 2018

Codecov Report

Merging #783 into release/4.0 will increase coverage by 0.01%.
The diff coverage is 83.33%.

Impacted file tree graph

@@                Coverage Diff                @@
##             release/4.0     #783      +/-   ##
=================================================
+ Coverage          77.24%   77.25%   +0.01%     
- Complexity          1849     1850       +1     
=================================================
  Files                241      241              
  Lines               6815     6818       +3     
  Branches            1012     1012              
=================================================
+ Hits                5264     5267       +3     
  Misses              1301     1301              
  Partials             250      250
Impacted Files Coverage Δ Complexity Δ
.../org/web3j/protocol/websocket/WebSocketClient.java 76% <83.33%> (+3.27%) 7 <4> (+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 14665e5...851f6ac. Read the comment docs.

@snazha-blkio snazha-blkio deleted the bugfix/731-npe-websocket-client branch November 18, 2018 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants