Check for successful tar response in TarFetchedCallback#1620
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1620 +/- ##
============================================
+ Coverage 22.76% 23.58% +0.81%
- Complexity 704 728 +24
============================================
Files 189 193 +4
Lines 8244 8276 +32
Branches 609 609
============================================
+ Hits 1877 1952 +75
+ Misses 6184 6139 -45
- Partials 183 185 +2 |
Guardiola31337
left a comment
There was a problem hiding this comment.
Great work @danesfeder
I've left a couple of minor comments / questions.
Thanks for running with this one.
|
|
||
| @Test | ||
| public void onSuccessfulResponse_downloadTaskIsExecuted() { | ||
| RouteTileDownloader downloader = mock(RouteTileDownloader.class); |
There was a problem hiding this comment.
Could you extract (if possible) some of the shared arrange code across these tests into a private factory method?
|
|
||
| @Test | ||
| public void onFinishedDownloading_tarIsUnpacked() { | ||
| RouteTileDownloader downloader = mock(RouteTileDownloader.class); |
There was a problem hiding this comment.
Could you extract (if possible) some of the shared arrange code across these tests into a private factory method?
| if (response.isSuccessful()) { | ||
| downloadTask.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR, response.body()); | ||
| } else { | ||
| OfflineError error = new OfflineError(response.message()); |
There was a problem hiding this comment.
It'd be 💯 appending the response code to the error message.
There was a problem hiding this comment.
For extra context these are the messages used in iOS mapbox/mapbox-navigation-ios#1892
|
|
||
| @Override | ||
| public void onErrorDownloading() { | ||
| OfflineError error = new OfflineError("Error occurred downloading tiles: null file"); |
There was a problem hiding this comment.
How about extracting this magic number into a constant?
9abe3b5 to
acb39eb
Compare
|
@Guardiola31337 thanks for the feedback, this is updated |
acb39eb to
5fc072a
Compare
5fc072a to
91fd5fd
Compare
Guardiola31337
left a comment
There was a problem hiding this comment.
Thanks for addressing the feedback @danesfeder
🚢:ship::ship:
We can check for a successful
Responsein theTarFetchedCallbackto provide more information for situations like #1618, where developers are using tokens are aren't compatible with offline routing.The main point of this PR was:
But, I also took some time to extract some of the callbacks and listeners that were living in the
RouteTileDownloader. By extracting them, we can add tests to verify the business logic being executed in each callback or listener. I did not change any business logic, this was mainly shuffling classes around / adding constructors for the dependencies they needed.