#460, adding tests #514
#460, adding tests #514
Conversation
|
CLA Assistant Lite bot: I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request |
bh2smith
left a comment
bh2smith
left a comment
There was a problem hiding this comment.
There are a few things here that, I think could be improved.
tests/unit/test_common.py
Outdated
| import datetime | ||
| from src.data_sync.common import partition_time_range | ||
|
|
||
| def test_partition_time_range(mocker: MockerFixture): |
There was a problem hiding this comment.
This mocker does not appear to be used for anything and can probably be removed.
There was a problem hiding this comment.
Thanks for the comments @bh2smith as this is still in draft I still have quite a few tests to add, so when I am done I'll clean up everything and mark it as ready. I also wanted to open up the draft PR so that you all could see I was working on it.
Does cowprotocol use draft PR's?
There was a problem hiding this comment.
Ya drafts are cool. 😎 just wanted to share some feedback on your draft before you had to go and change way more of the same stuff.
There was a problem hiding this comment.
Okay cool. Also I think we should add an additional check in the partition_time_range function as it assumes start and end times are in utc but doesn't enforce it.
When using relativedelta you can get this error:
TypeError: can't compare offset-naive and offset-aware datetimesI can add it and you can see.
tests/unit/test_common.py
Outdated
|
|
||
| # test that if they are equal or negative the function raises an error | ||
| with pytest.raises(AssertionError): | ||
| partition_time_range(start_time=start_time, end_time=start_time) |
There was a problem hiding this comment.
Nit: Given that the parameters have the same name as the expected input, I don't think this needs to be repeated.
There was a problem hiding this comment.
Not sure what you mean. This is testing the assert in the function and it's clear that the inputs are the same so it should fail.
| service_fee: Fraction, | ||
| reward_token_address: Address, | ||
| ): | ||
|
|
There was a problem hiding this comment.
this was from running black ./
| Otherwise, the range is split into n pieces of the form [(start_time, start_of_month_2), | ||
| (start_of_month_2, start_of_month_3),..., (start_of_month_n, end_time)]. | ||
| """ | ||
| assert ( |
There was a problem hiding this comment.
@bh2smith I added this to one function. What do you think?
Are you cool with me adding them to the others or would you prefer I create a utility function for this check?
Thanks for the input.
|
@bh2smith Can you please re-look at this as it's been awhile and I haven't gotten a response thanks. |
2 participants
related to this issue: #460