search

dimitri-xyz / haskell-coinbase-pro

A Haskell client for the Coinbase Pro API
MIT License
11 stars 6 forks source link

Fix heartbeats #6

Closed ericpashman closed 4 years ago

ericpashman commented 4 years ago

This patch (1) fixes a runtime error on parsing heartbeats (due to typo), exposes the setHeartbeat function, and (3) turns on heartbeats in the tests, while also reducing the number of decode-encode tests on messages received from the server from 1000 to 10.

Note that (3) has a couple of side-effects beyond actually testing the parsing of heartbeats. With heartbeats turned on, the client will always receive at least one message per second from the server (if all is well), so tests on the sandbox server no longer fail due inactivity, as they previously typically did. But this also means that the tests now pass even when the only messages received and passed to the encode-decode test are heartbeats. This is undesirable, but IMO the tests require some major work for a number of reasons; I'm about to open a separate issue describing some of the problems there.

dimitri-xyz commented 4 years ago

I am uncomfortable reducing the number of decode-encode tests from 1000 to 10. This means that most of the time the tests will not be testing anything and the test will pass. That introduces (or worsens) a silent failure. A test that is unable to test anything must signal a failure.

In fact, I am not sure I am comfortable with the current solution of bumping that number up to 1000 in the hopes of receiving different kinds of messages, but at least we can guarantee we received the most common type. Do we need a separate test for the heartbeat?

ericpashman commented 4 years ago

Yes, I agree, decreasing the number of decode-encode tests is a poor solution. But it seems to me a necessary (temporary) evil if we're actually going to run the existing tests before we write better ones. Before applying this patch, with 1000 iterations and no heartbeats, the tests will just about always fail on the sandbox server due to inactivity, after making the user wait for a long timeout. That's pointless. But running the tests on the live server is an even worse option, because it's either similarly useless (in the case of an unfunded account, because many of the tests will fail trivially) or actually dangerous (in the case of a funded account, because the tests place real orders).

The change from 1000 to 10 in my code is a practical compromise. With heartbeats enabled, the original 1000 tests will typically pass on the sandbox server after about 16 minutes, by which time the client will have received and encode-decode tested nearly 1000 heartbeats and maybe a handful of other messages. Since this is basically pointless, I chose to decrease the parameter to 10 to speed up the tests. (Note that a value of 10 also sometimes eventually produces meaningful test results when heartbeats are not enabled, since the client might eventually receive on the order of 10 non-heartbeat messages before timing out.)

In other words, set to 1000, the tests will just about always timeout and fail on the sandbox server if the heartbeat is not enabled; with heartbeats enabled, the tests will pass after a long time, but having meaningfully tested very little. Set to 10, with heartbeats not enabled, the tests will sometimes produce meaningful results after a long while, and sometimes timeout and fail; with heartbeats enabled, the tests will pass, but very likely having tested only heartbeats.

The solution to this is better tests. Meanwhile, I think enabling and testing heartbeats with a small number of encode-decode tests is the best choice among the four possible permutations on offer with the existing tests (heartbeats on or off, high or low number of test iterations). But perhaps we should just commit the fixes to the heartbeat code and leave the tests alone pending further work?

dimitri-xyz commented 4 years ago

It seems we have different use cases for the tests.

I use the tests more on the live environment (with an account with a small amount of funds) than the sandbox. In short, the tests were not just for development, they were a sanity check before deployment. To me, there's more value to running the tests there. However, I have not run this for a long time, so I can see it is possible that it will cost too much money to run the tests on the live server as they are today.

I guess the ideal solution for me would be to change the parameters so that the user can explicitly request that they be run in the live environment and with affordable parameters.

I like your last suggestion. Could we adopt the heartbeat code and leave the tests as is for now? It seems the decode-encode test as it is today really only makes sense in the live environment. This is a shame, it is a good test. I think we should just skip this test (and point this out) on the sandbox and make a distinct test for the heartbeat code.

Thoughts?

ericpashman commented 4 years ago

I'm fine with committing the fix without changing the tests, if that's what you prefer. Let's close this and resume the discussion about testing in #7.