-
-
Notifications
You must be signed in to change notification settings - Fork 573
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
Added mocking for test_eve.py #2961
Conversation
1. Used mocking to simulate online test 2. Online test function to cross-check the mocking function 3. Used parametrization wherever possible
Hello @yashrsharma44! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2019-03-31 19:06:58 UTC |
Thanks for the pull request @yashrsharma44! Everything looks great! |
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'm so sorry for the style nits. 😅
'physobs': 'irradiance', | ||
'provider': 'LASP' | ||
} | ||
st_datetime = datetime.strptime(start_date, '%Y/%m/%d') |
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.
IMO, you shouldn't use datetime
in the codebase XD (Because we have switched to astropy.time
for our time related stuff and it can do everything datetime
can).
But maybe @Cadair and @nabobalis might have a different opinion.
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 tried parsing the date in the %Y/%m/%d
format in astropy.time
if I remember it correctly. But it wasn't working in the way as expected. So I switched to datetime
.
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.
If it fails with astropy.time.Time
it would be good to know, it might be a bug upstream. I would have thought that that would have worked with astropy.time.Time
. But I have no real issue with using datetime
in the tests.
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.
Let me try that, I would update you with the working.
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 got this error while parsing -
>>> from astropy import time
>>> a = time.Time('2012/10/4')
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/home/yash/sunpydev/vnv37/lib/python3.7/site-packages/astropy/time/core.py", line 399, in __init__
precision, in_subfmt, out_subfmt)
File "/home/yash/sunpydev/vnv37/lib/python3.7/site-packages/astropy/time/core.py", line 454, in _init_from_vals
precision, in_subfmt, out_subfmt)
File "/home/yash/sunpydev/vnv37/lib/python3.7/site-packages/astropy/time/core.py", line 505, in _get_time_fmt
raise ValueError('Input values did not match {0}'.format(err_msg))
ValueError: Input values did not match any of the formats where the format keyword is optional ['datetime', 'iso', 'isot', 'yday', 'datetime64', 'fits', 'byear_str', 'jyear_str']
if level is 0: | ||
resp.client = LCClient | ||
else: | ||
resp.client = VSOClient() |
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.
Is it VSOClient
or VSOClient()
?
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.
We are using an object of VSOClient()
This pull request has been automatically marked as stale because it has not had any activity for the past five months. It will be closed if no further activity occurs. If the ideas in this pull request are still worth implementing, please make sure you open an issue to keep track of that idea! |
This pull request has been automatically closed since there was no activity for a month since it was marked as stale. If the ideas in this pull request are still worth implementing, please make sure you open an issue to keep track of that idea! |
Partial work for #2874