-
-
Notifications
You must be signed in to change notification settings - Fork 396
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
ESO: fixes to retrieve_data() and improvements in login() #420
Conversation
@@ -539,8 +541,7 @@ def retrieve_data(self, datasets, cache=True): | |||
" perhaps the requested file could not be found?") | |||
log.info("Downloading files...") | |||
for fileId in root.select('input[name=fileId]'): | |||
fileLink = fileId.attrs['value'].split()[1] | |||
fileLink = fileLink.replace("/api", "").replace("https://", "http://") | |||
fileLink = "http://dataportal.eso.org/dataPortal"+fileId.attrs['value'].split()[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.
Is this universally correct for all ESO data products (if you know)? Should this be configurable or hard-coded as is?
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.
Don't know. The full file link is also available but I am having a hard time extracting it intelligently. I would say, for now, it just works... (tm)
Could you add a remote test that will catch this? i.e., the query you performed where you got unexpected results? |
Right away... ;) |
This is going to be difficult as we need to authenticate first. Maybe we should support |
Testing for the data download will be difficult, I agree - perhaps we can ask the ESO archivists for a special account for testing that would be granted access only to public data, and store that password publicly? But, testing for the query should not require login |
But none of the failing issues were in the non-authenticated zone. Marching forward... |
Wouldn't the checkbox issue show up in the non-authenticated zone? If not, I guess we need to find a way to test when authenticated. |
And it does not work. Running tests and raw_input don't go together. I should use a username configuration item instead... |
And the above works when having your username configured. But the test suite does not pick-up your configuration... I need help from an astropy ConfigItem guru... |
So you put the line |
Yes, not picked-up when the test suite is run with: |
@astrofrog - can you comment here? Are configuration files ignored during testing? |
Just in case, does not work either with:
|
@@ -104,3 +104,10 @@ def test_list_instruments(self): | |||
# data_files = eso.retrieve_data([data_product_id]) | |||
# # How do we know if we're going to get .fits or .fits.Z? | |||
# assert 'AMBER.2006-03-14T07:40:03.741.fits' in data_files[0] | |||
|
|||
def test_retrieve_data(self): |
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.
This still fails now, right? Could you mark this as failing; @pytest.mark.xfail
I think? Then I'll merge this and we can open a fresh issue for the tests.
Reject when not checked!
You will be asked for it…
MIDI is my favorite of the moment, sorry AMBER!
e0159db
to
921a1f6
Compare
Rebased against master... should be all good now. Except for the |
With change log included! |
This is the reason why the local configuration file is not used when running tests: |
Ah, you have one of the |
Done by |
Yes. I think it's the only way we'll be able to run login tests. |
With the above, you can now omit PS: I need to update CHANGES and documentation. |
@keflavich - Good for review. Let's address the xfail at a later stage. |
@jwoillez - OK, passes review. Let's use |
ESO: fixes to retrieve_data() and improvements in login()
This |
Fixes:
Improvements:
username
optional inlogin()
username