[WIP] Added mock test skeleton for buy, sell and withdraw functions #95
Conversation
|
I have a love/hate relationship with mocks. This repo exemplifies 2 cases where mocks can be really deceptive: dynamic languages and external integrations. I'm ok with mocking responses for the 1.1 API since it is stable. The 2.0 API is a moving target though and has changed multiple times in the last few months. Mocking those responses can lead us into a false sense of security about whether our code is working or not. I suppose since the general shape of the responses (from even the 2.0 API) is stable, shallow testing against a canned response is fine - i.e. whether a list of results is present, success is true or false, etc. Let's just not take it any further then that 😄 Since your commit message is WIP I suppose you don't want this merged yet? |
|
I labelled it WIP as I wanted to add more detailed mock tests (parameter check etc), but if you want to leave it shallow then I would consider it done. I also wanted to hear your thoughts before going more deeply into the test suite. |
bittrex/test/bittrex_tests.py
Outdated
|
|
||
| def mocked_get_order_query(protection=None, path_dict=None, options=None): | ||
| return json.loads( | ||
| '{\ |
There was a problem hiding this comment.
Maybe import a multiline string from somewhere else or put it in a file.
There was a problem hiding this comment.
Ok, will change it to make the test file more readable
There was a problem hiding this comment.
Also for in the event the format changes we can just paste in what Bittrex gives us in the future.
|
I've moved the Json to separate file.
|
|
Yeah, I saw those failures. I was hoping they'd clear up by now so we
could get a clean build. This series of events lends some credence to your
desire to add mocks :)
But at the same time I think the ground truth of whether our code does what
it claims is most important. Those API calls just don't work so we should
acknowledge it and move forward.
Would you mind adding skips to those tests? Let's get a clean build and
then we can merge. If the 2.0 API is being changed this PR is not the
place to handle it.
If you don't have time I can add the skips...
…On Wed, Dec 13, 2017, 4:13 PM Tine ***@***.***> wrote:
I've moved the Json to separate file.
Travis tests are failing because of API v2.0 calls failing for:
- get_markets
- get_market_history
- get_openorders
- list_markets_by_currency
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#95 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA2Cbt6wgD_y8DZ0GZ4EHkfN8r5uMpwjks5tAD4IgaJpZM4Qyg44>
.
|
|
@timechild PR #104 has been merged to master -- it removed the broken 2.0 endpoints from the API altogether. It may be easiest to rebase this against master? |
This adds mock tests for methods that are harder to test: buy_limit, sell_limit, cancel, withdraw, get_order for API v1.1 and trade_sell and trade_buy for API v2.0.
Tests are using Python mock (Standard library 3.3+) module to replace _api_query method and return a JSON response according to Bittrex docs.
At the moment there is only a basic assertion testing, but this can be expanded in the future.
Few other minor assertion test were added as well.
This PR increases test coverage from 74% to 78%.