Fixes in JWST login #2807
Fixes in JWST login #2807
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2807 +/- ##
==========================================
+ Coverage 66.34% 66.48% +0.13%
==========================================
Files 235 235
Lines 18077 18077
==========================================
+ Hits 11994 12019 +25
+ Misses 6083 6058 -25
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
While it's very unlikely that we would routinely find any issues with the login machinery using tests (as we don't have any login credentials in CI, etc.), but I would generally in favour of having some test coverage that directly uses the remote service, rather than fully rely on mocking the expected responses.
This, and the rest of the in-line comments are just comments.
| >>> print(*(column.name for column in table.columns), sep="\n") # doctest: +IGNORE_OUTPUT | ||
| "public" |
There was a problem hiding this comment.
Hmm, I would leave this, at least this passes both locally, but also on CI, they way it was.
There was a problem hiding this comment.
I skipped these tests, as they will fail when we release the new version. I will create an additional PR when we released, to make them available again.
astroquery/esa/jwst/core.py
Outdated
| @@ -898,8 +898,7 @@ def get_product(self, *, artifact_id=None, file_name=None): | |||
| output_file_name = self._query_get_product(artifact_id=artifact_id) | |||
| err_msg = str(artifact_id) | |||
| except Exception as exx: | |||
| raise ValueError('Cannot retrieve product for artifact_id ' | |||
| + artifact_id + ': %s' % str(exx)) | |||
| raise ValueError('Cannot retrieve product for artifact_id ' + artifact_id + ': %s' % str(exx)) | |||
There was a problem hiding this comment.
you've already cleaned up many of these error messages by switching to f-strings, maybe a few more could be done? (your choice, it isn't holding up the PR)
There was a problem hiding this comment.
Yes, I have applied the same to other strings.
astroquery/esa/jwst/core.py
Outdated
| @@ -1086,7 +1082,7 @@ def __set_additional_parameters(self, param_dict, cal_level, | |||
|
|
|||
| def __get_quantity_input(self, value, msg): | |||
| if value is None: | |||
| raise ValueError("Missing required argument: '"+str(msg)+"'") | |||
| raise ValueError(f"Missing required argument: '{str(msg)}'") | |||
There was a problem hiding this comment.
fyi, no need for the str() any more
There was a problem hiding this comment.
Thanks for catching this
|
I agree that it would be a good idea to include remote tests for the login, but we need to find a secure way of including this test user. Anyway, I think we can include it in a different PR. I will increase the codecov/patch today. |
|
Ok, finally all the checks have passed! |
|
Hi all, any news on this? Is it necessary to include more improvements? If not, can we merge it? On the other hand, I think the pre-release procedure is not working as expected, can we create a new version after this is merged? Thanks in advance! |
There was a problem hiding this comment.
@bsipocz lgtm. There's one minor unaddressed comment in the docs, but I think since tests are passing, it is an acceptable change. Mostly this is fixing a real issue, iiuc, and adding tests, with many scattered style changes mixed in.
|
Oh, I'm sorry this has fallen through the cracks. Last time I checked something didn't really work with the remote-data part, let me get back to that today, maybe push a commit and get this merged. |
Ok! just executed the remote tests and it seems to be working... Anyway, please let me know if you need anything else. Related to the pre-released version, is this working? Could you please create a new version when merging this PR? thanks in advance! |
There was a problem hiding this comment.
One minor fix, and this is good to go. Thanks @jespinosaar for the patience.
| @pytest.mark.remote_data | ||
| class RemoteData: | ||
|
|
||
| def test_login_error(self): | ||
| jwst = JwstClass() | ||
| with pytest.raises(HTTPError) as err: | ||
| jwst.login(user="dummy", password="dummy") | ||
| assert "Unauthorized" in err.value.args[0] |
There was a problem hiding this comment.
This class is not being picked up for testing.
(I'll push a fix for it)
bsipocz
force-pushed
the
ESA_ejwst-login_fixes
branch
from
a881554
to
40c4098
Compare
|
I also needed to rebase this, so fixes to the docs builds are being picked up. |
|
Many thanks for your support @bsipocz, as always! and for the pre-release! |
Dear Astroquery team,
We realized the login feature in ESA TAP module was not working, as we are now using kwargs. This pull request aims to fix this issue. Thanks in advance for your support!
cc @esdc-esac-esa-int