fix issue #55 missing User-Agent header in API calls and issue #58 getFill API change #57
Conversation
onyxcoyote
changed the title
| NewMarketOrderSingle marketOrder = createNewMarketOrder("BTC-USD", "buy", new BigDecimal(0.01)); | ||
| Order order = orderService.createOrder(marketOrder); | ||
|
|
||
| assertTrue(order != null); //make sure we created an order |
There was a problem hiding this comment.
assertion seems redundant or possibly we need better tests for createOrder() method being called so we don't need to have assertions on preconditions which make the code look relatively untidy/big.
There was a problem hiding this comment.
Perhaps a mocked response on createOrder would work here?
There was a problem hiding this comment.
I think all the extra asserts can be removed. I was not able to get them to fail at all. Even in the case of insufficient test funds, it still returns a valid orderId (though insufficient funds causes this test and one other test to fail due to error).
The ideal situation for testing getFillByOrderId is that we have a valid orderId, which was also filled (which is pretty much guaranteed with a market order it seems). Though alternatively, it would be possible to pass in an invalid orderId and just expect 0 fills in response, which would still test the actual API call, just not the "place an Order an retrieve it's Fill" path. I'm open to trying out some other alternatives, though.
What do you think of the following?
@Test
public void canMakeMarketOrderThenGetFillByOrderId() {
NewMarketOrderSingle marketOrder = createNewMarketOrder("BTC-USD", "buy", new BigDecimal(0.01));
Order order = orderService.createOrder(marketOrder);
List<Fill> fills = orderService.getFillByOrderId(order.getId(), 100);
assertTrue(fills.size() == 1);
}
There was a problem hiding this comment.
Thanks! This update has been committed.
| Order filledOrder = orderService.getOrder(orderId); | ||
| assertTrue(filledOrder != null); //ensure our order hit the system | ||
| assertTrue(new BigDecimal(filledOrder.getSize()).compareTo(BigDecimal.ZERO) > 0); //ensure we got a fill | ||
| log.info("Order opened and filled: " + filledOrder.getSize() + " @ " + filledOrder.getExecuted_value() |
There was a problem hiding this comment.
I don't usually see log statements in tests due to them being largely redundant. If something goes wrong, the assertion should fail for the relevant condition.
| + " at the cost of " + filledOrder.getFill_fees()); | ||
|
|
||
| List<Fill> fills = orderService.getFillByOrderId(orderId, 100); | ||
| assertTrue(fills.size() >= 0); |
There was a problem hiding this comment.
Yes, good point. I will change to == 1 because it should be always 1 in this situation.
| String orderId = order.getId(); | ||
| assertTrue(orderId.length() > 0); //ensure we have an actual orderId | ||
| Order filledOrder = orderService.getOrder(orderId); | ||
| assertTrue(filledOrder != null); //ensure our order hit the system |
There was a problem hiding this comment.
I believe assertNotNull is available for use here (or something very similar)
There was a problem hiding this comment.
I will remove filledOrder entirely. It is actually not necessary.
|
|
||
| assertTrue(order != null); //make sure we created an order | ||
| String orderId = order.getId(); | ||
| assertTrue(orderId.length() > 0); //ensure we have an actual orderId |
There was a problem hiding this comment.
perhaps assertNotNull works better here - so long as we get back something then we don't really care about the value?
There was a problem hiding this comment.
Agree, will change
There was a problem hiding this comment.
Correction. It looks like that can be removed. Even a failed Order still gets a valid orderId.
| assertTrue(fills.size() >= 0); | ||
| } | ||
|
|
||
| @Test | ||
| public void createMarketOrderBuyThenGetFillByOrderId() { |
There was a problem hiding this comment.
naming should follow format of should do something When X happens
There was a problem hiding this comment.
How about canMakeMarketOrderThenGetFillByOrderId?
The intention of the method is primarily to test GetFillByOrderId, but it ideally would place an Order first, then test getFillByOrderId.
There was a problem hiding this comment.
shouldGetFilledByOrderIdWhenMakingMarketOrderBuy()
There was a problem hiding this comment.
Ahh... Ok, method has been renamed as suggested.
|
Merged |
Fixes issue where cloudflare blocks API requests with no User-Agent header. Based on the comment by alistairrutherford, the value of the User-Agent does not seem to matter, as long as it's present.
"Access denied | api-public.sandbox.pro.coinbase.com used Cloudflare to restrict access"