-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
refactor: move date_parser to unittest #18810
Conversation
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.
Thanks for this migration! One comment about the mock, other than that LGTM
def mock_parse_human_datetime(s: str) -> Optional[datetime]: | ||
if s == "now": | ||
return datetime(2016, 11, 7, 9, 30, 10) | ||
elif s == "2018": | ||
return datetime(2018, 1, 1) | ||
elif s == "2018-9": | ||
return datetime(2018, 9, 1) | ||
elif s == "today": | ||
return datetime(2016, 11, 7) | ||
elif s == "yesterday": | ||
return datetime(2016, 11, 6) | ||
elif s == "tomorrow": | ||
return datetime(2016, 11, 8) | ||
elif s == "Last year": | ||
return datetime(2015, 11, 7) | ||
elif s == "Last week": | ||
return datetime(2015, 10, 31) | ||
elif s == "Last 5 months": | ||
return datetime(2016, 6, 7) | ||
elif s == "Next 5 months": | ||
return datetime(2017, 4, 7) | ||
elif s in ["5 days", "5 days ago"]: | ||
return datetime(2016, 11, 2) | ||
elif s == "2018-01-01T00:00:00": | ||
return datetime(2018, 1, 1) | ||
elif s == "2018-12-31T23:59:59": | ||
return datetime(2018, 12, 31, 23, 59, 59) | ||
else: | ||
return None |
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.
I wonder if we could use freeze_time
from freezegun
here to avoid hardcoding?
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.
Thanks, Ville! Unfortunately, I'm stuck when migrate to freeze_time
. I leave some notes for this.
#@freeze_time('2016-11-07T9:30:10Z'))
@freeze_time(datetime(2016, 11, 7, 9, 30, 10))
def test_get_since_until() -> None:
result: Tuple[Optional[datetime], Optional[datetime]]
expected: Tuple[Optional[datetime], Optional[datetime]]
# result = get_since_until()
# expected = None, datetime(2016, 11, 7, tzinfo=pytz.utc)
# assert result == expected
result = get_since_until(" : now")
# expected = None, datetime(2016, 11, 7, 9, 30, 10)
print(result)
# print(expected)
(superset) yongjie.zhao@:incubator-superset$ pytest -s --disable-warnings tests/unit_tests/utils/date_parser_tests.py::test_get_since_until
============================================================================= test session starts ==============================================================================
platform darwin -- Python 3.8.6, pytest-6.2.4, py-1.10.0, pluggy-0.13.1
rootdir: /Users/yongjie.zhao/workspace/warehouse/superset/incubator-superset, configfile: pytest.ini
plugins: cov-2.12.1, anyio-2.2.0, celery-4.4.7, mock-3.6.1, pyfakefs-4.5.0
collected 1 item
tests/unit_tests/utils/date_parser_tests.py now
2016-11-07 17:30:10+00:00
(None, FakeDatetime(2016, 11, 7, 17, 30, 10))
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.
Ok, thanks for investigating - we can leave this for a follow-up 👍
Codecov Report
@@ Coverage Diff @@
## master #18810 +/- ##
=======================================
Coverage 66.31% 66.31%
=======================================
Files 1620 1620
Lines 63088 63088
Branches 6372 6372
=======================================
Hits 41839 41839
Misses 19592 19592
Partials 1657 1657
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
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.
LGTM
SUMMARY
move date parser test from integration to unit test and use functional test instead of class base test.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
CI
ADDITIONAL INFORMATION