8000 The /status table command was getting slower when we had multiple trades opened by jblestang · Pull Request #293 · freqtrade/freqtrade · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

The /status table command was getting slower when we had multiple trades opened #293

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

Merged
merged 33 commits into from
Jan 7, 2018
Merged

The /status table command was getting slower when we had multiple trades opened #293

merged 33 commits into from
Jan 7, 2018

Conversation

jblestang
Copy link
Contributor

For each trades opened, the telegram bot was fetching the last bid price making a request towards bittrex.com. This PR allow the use of cached values retrieved in the _process function and fix issue #278.

By opportunity all calls to get the bid prices where replaced in the Bot.

@glonlas
Copy link
Member
glonlas commented Jan 3, 2018

Very nice PR, I like it. Only one comment, there is no unitest to test the new « refresh » logic in get_ticker().
Could you add this unit test to test the function when you refresh the value and when you read it from the cache? Then we are good to go.

@jblestang
Copy link
Contributor Author

Yes sure!

"""
Gets ticker for given pair.
:param pair: Pair as str, format: BTC_ETC
:param refresh: Shall we query a new value or a cached value is enought
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type enought -> enough.

Copy link
Member
@glonlas glonlas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The unit test should be improved.

@@ -163,7 +168,19 @@ def test_get_ticker(mocker, ticker):
ticker = get_ticker(pair='BTC_ETH')
assert ticker['bid'] == 0.00001098
assert ticker['ask'] == 0.00001099

# if not caching the result we should get the same ticker
ticker = get_ticker(pair='BTC_ETH', refresh=False)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition is not tested properly. You should change the ticker before to see that get_ticker() still return the same.

The test could look like that.

def test_get_ticker(mocker, ticker):

    api_mock = MagicMock()
    api_mock.get_ticker = MagicMock(return_value=ticker())
    mocker.patch('freqtrade.exchange._API', api_mock)

    ticker = get_ticker(pair='BTC_ETH')
    assert ticker['bid'] == 0.00001098
    assert ticker['ask'] == 0.00001099

    # if not caching the result we should get the same ticker
    api_mock.get_ticker = MagicMock(return_value={"bid": 0, "ask": 1})
    mocker.patch('freqtrade.exchange._API', api_mock)
    ticker = get_ticker(pair='BTC_ETH', refresh=False)
    assert ticker['bid'] == 0.00001098
    assert ticker['ask'] == 0.00001099

    # change the ticker
    api_mock.get_ticker = MagicMock(return_value={"bid": 2, "ask": 3})
    mocker.patch('freqtrade.exchange._API', api_mock)

    ticker = get_ticker(pair='BTC_ETH', refresh=True)
    assert ticker['bid'] == 2
    assert ticker['ask'] == 3

However, when I execute this test, it fails.

22:01 $ pytest freqtrade/tests/exchange/
============================= test session starts ==============================
platform darwin -- Python 3.6.3, pytest-3.3.1, py-1.5.2, pluggy-0.6.0
rootdir: /Users/geraldlonlas/Desktop/dev/freqtrade, inifile:
plugins: mock-1.6.3, cov-2.5.1
collected 21 items

freqtrade/tests/exchange/test_exchange.py ..............F...             [ 85%]
freqtrade/tests/exchange/test_exchange_bittrex.py ...                    [100%]

=================================== FAILURES ===================================
_______________________________ test_get_ticker ________________________________

mocker = <pytest_mock.MockFixture object at 0x1073437b8>
ticker = {'ask': 1, 'bid': 0}

    def test_get_ticker(mocker, ticker):

        api_mock = MagicMock()
        api_mock.get_ticker = MagicMock(return_value=ticker())
        mocker.patch('freqtrade.exchange._API', api_mock)

        ticker = get_ticker(pair='BTC_ETH')
        assert ticker['bid'] == 0.00001098
        assert ticker['ask'] == 0.00001099

        # if not caching the result we should get the same ticker
        api_mock.get_ticker = MagicMock(return_value={"bid": 0, "ask": 1})
        ticker = get_ticker(pair='BTC_ETH', refresh=False)
>       assert ticker['bid'] == 0.00001098
E       assert 0 == 1.098e-05

freqtrade/tests/exchange/test_exchange.py:175: AssertionError
===================== 1 failed, 20 passed in 0.19 seconds ======================

This argument enables usage of different backtesting directories.
Useful if one wants compare backtesting performance over time.
@glonlas
Copy link
Member
glonlas commented Jan 6, 2018

Any update on the Unit tests?

@jblestang
Copy link
Contributor Author

Unitest fixed

@jblestang
Copy link
Contributor Author

Sorry I did merge develop instead of rebasing ... (we do merges @ work ... )

Copy link
Member
@glonlas glonlas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the update.

# if not caching the result we should get the same ticker
ticker = get_ticker(pair='BTC_ETH', refresh=False)
print(str(ticker))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have let a print() ;)

@glonlas glonlas merged commit fca6a09 into freqtrade:develop Jan 7, 2018
@jblestang jblestang deleted 867F the fix_issue_278 branch January 20, 2018 20:58
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

Successfully merging this pull request may close these issues.

4 participants
0