qtrade-exchange / qtrade-py-client

A simple requests based wrapper for the qTrade.io API
https://qtrade-exchange.github.io/qtrade-docs/?python--PyClient#public
MIT License
7 stars 4 forks source link

test_api.py improvements #6

Open icook opened 4 years ago

icook commented 4 years ago

Tests could use some improvement. In no specific order, some issues I see:

  1. Try to implement Don't repeat yourself (DRY) when possible. For instance, line 1156 and 1172 are identical, better to define it above and reference it.
  2. api._markets_map is set to the exact same value twice, and they're copy and pasted. Just define them as a nicely named global and set them inside the test as needed. If you need specific values to test particular functionality, set api._markets_map to the reusable fixture using deep_copy, and then modify it explicitly as needed as I did on 1195.
  3. Generally, each test should be its own test function with an descriptive function name. Try to only test one piece of functionality in each test. This is not hard and fast, but in general.
  4. Make use of assert raises if writing a negative test.
  5. If you're initializing the api in the same way over and over for several (3+) tests, define a fixture that does the setup for you. The fixture will be generated fresh for each test.
  6. test_refresh_common could be made much clearer by setting up the mock return value, then referencing it in your assertions. For instance, assert api.markets["GRIN_BTC"] == req_return['markets'][0]. This makes your intent much clearer, and the test more flexible and less cluttered. Similar patterns could benefit several of the functions.
  7. The req_1, req_2 pattern in test_orders could be improved by observing that MagicMock doesn't require a function as the side effect, and will accept a return value. If each mock api._req is setup for a single test, then there is no need to assert anything about call parameters, so I would do something like:
import copy

order_return = {
            "order": {
                "id": 8987684,
                "market_amount": "0.01",
                "market_amount_remaining": "0.01",
                "created_at": "2019-11-14T23:46:52.897345Z",
                "price": "1",
                "order_type": "sell_limit",
                "market_id": 1,
                "open": True,
                "trades": [],
            }
        }

def test_buy_prevent_taker():
    ret = copy.deepcopy(order_return)
    # set whatever specific stuff you need here
    ret["order"]["market_amount"] = 2
    api._req = mock.MagicMock(side_effect=ret)

In addition, your assertions should be specific instead of general. It's okay to have one test that asserts the entire return value, but other tests should make assertions only about the specific logic that they intend to test, not all logic.

@Henelik

icook commented 4 years ago

A few more things.

python3.7 -m pip install --user pytest-cov
python3.7 -m pytest --cov-report html --cov-report term --cov=qtrade_client tests/ --cov-branch
google-chrome htmlcov/index.html

Will let you see the test coverage report. Looking over that I spot a few things that would be good to cover:

  1. HMAC auth with a query string should be tested since it was a bug in the past. I'd like to use a real, verified working set of data, so you can use code like below:
from qtrade_client.api import QtradeAPI

# String is of the format "[key_id]:[key]"
hmac_keypair = "1:1111111111111111111111111111111111111111111111111111111111111111"
client = QtradeAPI("https://api1.qtex.dev", key=hmac_keypair)
client.orders(open=False)

And inject a print statement in the HMAC code to extract your desired assertion. Specifically we're trying to exercise line 41.

  1. After review I realize that the several order tests aren't quite testing what they probably should be. We're asserting that the return value is what we've just mocked for the request method, which will of course always be true... I like the assert_called_with usage that has been added in various places, and this would make good sense to be added to the order tests since that will actually test the logic of the order wrapper.