-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Refactoring the API calls #763
Conversation
…arameters are all correctly encoded and sent, and responses are parsed.
Well, that's disappointing - Travis CI timed out starting the emulator. I notice in the build history this is happening a lot. Want to maybe look at using a tool like Robolectric to be able to run the tests locally without an emulator? |
… (as a unit test) rather than on-device.
Thanks for the PR, @psh ! It's huge, please give us some time to look through it. ;) |
Hi @psh , I've tested this and it works well except for one (significant) problem - it crashes each time it attempts to finalize an upload. When the loading bar has completed, the app crashes with the error log:
The image upload actually does go through, e.g. https://commons.wikimedia.org/wiki/File:Fog_in_Brisbane.jpg , but it displays erroneously in the contributions view as a failed upload: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue with upload - see thread comment
…Result when on the happy-path.
Codecov Report
@@ Coverage Diff @@
## master #763 +/- ##
=======================================
- Coverage 6.8% 6.6% -0.2%
=======================================
Files 94 99 +5
Lines 5073 5134 +61
Branches 460 465 +5
=======================================
- Hits 345 339 -6
- Misses 4700 4768 +68
+ Partials 28 27 -1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NPE fixed and the branch has been brought up to date with current master.
It looks like my last patch #738 brought some conflicts here. Please free to ping me if you need help. |
Sorry, @psh , I missed your comment from 2 days ago. Should I go ahead and re-test the upload issue? |
The branch is now up-to-date with master. The conflicts were trivial (just imports) I deal with much worse on my team in work! A PR of this size is bound to run into them. @misaochan - I gave uploading a quick smoke test, the NPE is fixed for the images I tried - please retest. |
Works great for me now, thanks @psh ! :) |
In the process of refactoring related to the networking stack, I introduced a class
ApacheHttpClientMediaWikiApi
and felt the need to at least smoke test a few API calls to make sure that requests and responses are correctly handled. I used theMockWebServer
(from OkHttp) to serve up canned responses and assert that requests are well formed at a unit-test level. This was already a transitive dependency from the mapbox library, made explicit in thebuild.gradle
file.My refactor pulls all of the API calls into a single place so that the body of the app depends on a
MediaWikiApi
interface returning application specific models, with a concreteApacheHttpClientMediaWikiApi
class that hides the implementation. This makes the code more testable (isolating the networking tests to a single class) and makes it easier to use mocks in the rest of the app if you want to unit test just the interactions later.Although the testing isn't yet complete, I wanted to get the code into your hands as (IMO) it forms a solid basis to address your high-priority item #705 (upload tests). This PR doesn't include those tests, but it does aim to set you up for success in this endeavor. :)