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

test_api.py improvements #6

Open
icook opened this issue Dec 10, 2019 · 1 comment
Open

test_api.py improvements #6

icook opened this issue Dec 10, 2019 · 1 comment

Comments

@icook
Copy link
Collaborator

icook commented Dec 10, 2019

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
Copy link
Collaborator Author

icook commented Dec 13, 2019

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.

@Henelik Henelik mentioned this issue Dec 16, 2019
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

No branches or pull requests

1 participant