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

Consider SocketException as network error #659

Merged
merged 2 commits into from
May 18, 2023

Conversation

poovamraj
Copy link
Contributor

Changes

Currently NetworkErrorException is not working properly. The following comment is added for it to be fixed in the next major

// When the request failed due to network issues
// Currently [NetworkErrorException] is not properly thrown from [createErrorAdapter] in
// [AuthenticationAPIClient] and [UserAPIClient]. This will be fixed in the next major to avoid
// breaking change in the current major. We are not using IOException to check for the error
// since it is too broad.

As it can be seen in the previous PR - #235. NetworkErrorException should be thrown if OkHttp fails because of network issues. But currently this is not being done

A valid issue was raised to conside SocketException as a network error - #657. We are fixing this by adding it to the assertion

References

#657

Testing

Please describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. Since this library has unit testing, tests should be added for new functionality and existing tests should complete without errors.

  • This change adds unit test coverage
  • This change adds integration test coverage
  • This change has been tested on the latest version of the platform/language or why not

@poovamraj poovamraj requested a review from a team as a code owner May 16, 2023 12:44
@poovamraj poovamraj merged commit 4d7ebbb into main May 18, 2023
9 checks passed
@poovamraj poovamraj deleted the socket-exception-as-network-exception branch May 18, 2023 17:14
@poovamraj poovamraj mentioned this pull request May 19, 2023
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.

None yet

2 participants