-
-
Notifications
You must be signed in to change notification settings - Fork 502
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
Modify some tests so they pass on windows #6398
Modify some tests so they pass on windows #6398
Conversation
c426710
to
249be79
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @IanMichaelHarper thanks a lot for the PR!
I see two tests failed due to VCR cassettes:
FAILED rotkehlchen/tests/unit/test_general_cache.py::test_curve_cache[True-True-True] - vcr.errors.CannotOverwriteExistingCassetteException: Can't overwrite existing cassette ('/home/runner/work/rotki/rotki/test-caching/cassettes/unit/test_general_cache/test_curve_cache[True-True-True].yaml') in your current record mode ('none').
No match for the request (<Request (GET) https://api.curve.fi/api/getPools/ethereum/main>) was found.
No similar requests, that have not been played, found.
FAILED rotkehlchen/tests/unit/test_general_cache.py::test_curve_cache[True-False-True] - vcr.errors.CannotOverwriteExistingCassetteException: Can't overwrite existing cassette ('/home/runner/work/rotki/rotki/test-caching/cassettes/unit/test_general_cache/test_curve_cache[True-False-True].yaml') in your current record mode ('none').
No match for the request (<Request (GET) https://api.etherscan.io/api?module=proxy&action=eth_call&to=0x0000000022D53366457F9d5E68Ec105046FC4383&data=0x493f4f740000000000000000000000000000000000000000000000000000000000000007&apikey=8JT7WQBB2VQP5C3416Y8X3S8GBA3CVZKP4>) was found.
I will need to push the branch properly to the VCR repo, something which external contributors can't do. It's explained here: https://rotki.readthedocs.io/en/latest/contribute.html#syncing-with-the-cassettes-repository
I will do it and rebase this PR perhaps after merging the one with the DNS fix on windows.
@@ -533,15 +535,17 @@ def test_accounting_lifo_order(accountant): | |||
cost_basis.reset(DBSettings(cost_basis_method=CostBasisMethod.LIFO)) | |||
asset_events = cost_basis.get_events(asset) | |||
# first we do a simple test that from 2 events the second is used | |||
d = dt.datetime(2023, 4, 15, tzinfo=dt.timezone.utc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this change needed? Why would timestamp 1, 2 etc. not work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it doesn't have a timezone. Here's the stacktrace:
rotkehlchen\tests\unit\test_cost_basis.py:575:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
rotkehlchen\accounting\cost_basis\base.py:218: in calculate_spend_cost_basis
time=timestamp_to_date(acquisition_event.timestamp),
rotkehlchen\utils\mixins\customizable_date.py:24: in timestamp_to_date
return timestamp_to_date(
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
ts = 2, formatstr = '%d/%m/%Y %H:%M:%S %Z', treat_as_local = True
def timestamp_to_date(
ts: Timestamp,
formatstr: str = '%d/%m/%Y %H:%M:%S',
treat_as_local: bool = False,
) -> str:
"""Transforms a timestamp to a datestring depending on given formatstr and UTC/local choice"""
if treat_as_local is False:
date = datetime.datetime.fromtimestamp(ts, tz=datetime.timezone.utc).strftime(formatstr)
else: # localtime
date = datetime.datetime.fromtimestamp(
ts, # ignore below is due to: https://github.com/pjknkda/flake8-datetimez/issues/11
> tz=datetime.datetime.fromtimestamp(ts).astimezone().tzinfo, # noqa: DTZ006
).strftime(formatstr)
E OSError: [Errno 22] Invalid argument
This is the behaviour on windows:
>>> dt.datetime.fromtimestamp(1).astimezone()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
OSError: [Errno 22] Invalid argument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I am still not sure what the error is.
So dt.datetime.fromtimestamp(1)
works right? It's just that the astimezone()
doesn't? And somehow by adding d = dt.datetime(2023, 4, 15, tzinfo=dt.timezone.utc)
you initialize timezone to utc? What does dt.datetime.fromtimestamp(1)
return?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So dt.datetime.fromtimestamp(1) works right? It's just that the astimezone() doesn't?
Yes
And somehow by adding d = dt.datetime(2023, 4, 15, tzinfo=dt.timezone.utc) you initialize timezone to utc?
Yes
What does dt.datetime.fromtimestamp(1) return?
datetime.datetime(1970, 1, 1, 1, 0, 1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay so astimezone() from a timestamp without a timezone returns an error. The docs do not mention any such thing.
https://docs.python.org/3/library/datetime.html#datetime.datetime.astimezone
Without an arg it should take localtime which is what we want.
Only relevant issue I can find is this: python/cpython#80940
So on the naive timezone object, the astimezone
calls time.localtime()
and perhaps calls it with a negative argument? Could be a timezone problem that the time becomes a pre-epoch time for you if your timezone is west of UTC?
Seems like the problem is deeper than just the test and the real fix would need to be done in the function, but need to first understand why/what's happening.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to be then exatctly like this bug: python/cpython#73283
But in a different place. I don't see this bug reported so perhaps it would need to be reported. Is this in python 3.10 and 3.11? I guess then we found a bug in python in Windows and would need to report it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3.9, 3.10 and 3.11 it fails for me on windows 11
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay so then 2 questions.
-
Should I report a bug in python? Or do you wanna do it? Pointing to the previous bug and explaining that we hit the same thing with astimezone()
-
Production-wise ... what should be done here? Adding two extra ifchecks in all the
timestamp_to_date
calls just for this windows bug seems silly. Perhaps just adjust test to use timestamp 86400 and up? And then add a comment pointing to the bug and leaving the handling for the future? Rotki in its current form won't handle any such timestamps so close to the EPOCH. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I report a bug in python? Or do you wanna do it? Pointing to the previous bug and explaining that we hit the same thing with astimezone()
I'm happy to let you claim the glory of finding a bug in python if you want ;) otherwise I can do it if you want to focus on other things.
Perhaps just adjust test to use timestamp 86400 and up?
I think that makes the makes the most sense. If this is a bonafide bug in python then it will eventually get fixed upstream, and like you said no one is going to be using any dates that close to EPOCH, so I don't think there's any danger to production here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
happy to let you claim the glory of finding a bug in python if you want ;)
Made an issue here: python/cpython#107078
I think that makes the makes the most sense. If this is a bonafide bug in python then it will eventually get fixed upstream, and like you said no one is going to be using any dates that close to EPOCH, so I don't think there's any danger to production here.
All right so when you make any change please do that then in both places you added the timestamp fix.
75e133a
to
bf2f757
Compare
Codecov Report
@@ Coverage Diff @@
## develop #6398 +/- ##
===========================================
+ Coverage 75.68% 80.45% +4.76%
===========================================
Files 1109 1109
Lines 120178 120178
Branches 11403 11402 -1
===========================================
+ Hits 90961 96684 +5723
+ Misses 27592 21694 -5898
- Partials 1625 1800 +175
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a nitpick for perhaps a different PR, but will merge as it all looks good and all tests are green.
@@ -535,13 +535,13 @@ def test_accounting_lifo_order(accountant): | |||
# first we do a simple test that from 2 events the second is used | |||
event1 = AssetAcquisitionEvent( | |||
amount=ONE, | |||
timestamp=1, | |||
timestamp=1614556800, # 01/03/2021, changed from 1 for windows. See https://github.com/rotki/rotki/pull/6398#discussion_r1271323846 # noqa: E501 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: For a next PR.
You can just do this by assigning something like:
base_ts = 1614556800 # 01/03/2021, changed from 1 for windows. See https://github.com/rotki/rotki/pull/6398#discussion_r1271323846 # noqa: E501
Then use it below without repeating the comment everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course. Can't believe I didn't think of that at the time. Thanks!
rotki/test-caching/tree/fix-windows-tests-round-2 was successfully merged |
Partially closes #5168 by modifying some of the tests so they pass on windows.
Unfortunately these changes don't make the tests run any faster on windows and the test suite still takes almost twice as long to run on windows as it does on linux.
Changes
It seems some tests failed when run together but passed individually, so I added @pytest.mark.parametrize('use_clean_caching_directory', [True]) to these tests.
Windows doesn't seem to like Path's / for directory concatenation, so I replaced with os.path.join() instead.
os.chmod was used in some tests to change directory permissions, but this has no effect on windows, so I mocked them
Some new cassettes are needed so Add new episodes so Windows tests will pass test-caching#35 will need to be merged for the tests on this PR to pass
Most other tests were failing for various different reasons.
Also added some typing.