Skip to content
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

ALMA integration tests fix #2224

Merged
merged 9 commits into from
Nov 23, 2021
Merged

ALMA integration tests fix #2224

merged 9 commits into from
Nov 23, 2021

Conversation

andamian
Copy link

@andamian andamian commented Nov 20, 2021

Updates to the ALMA integration tests to use ALMA production servers.

A fix for #2157

@pep8speaks
Copy link

pep8speaks commented Nov 20, 2021

Hello @andamian! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-11-23 02:41:59 UTC

@andamian andamian marked this pull request as draft November 20, 2021 06:53
Copy link
Contributor

@keflavich keflavich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

Could you please add some documentation about the --alma-site testing to https://astroquery.readthedocs.io/en/latest/testing.html ?

@codecov
Copy link

codecov bot commented Nov 22, 2021

Codecov Report

Merging #2224 (b2bd97c) into main (cbec82e) will increase coverage by 0.15%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2224      +/-   ##
==========================================
+ Coverage   61.81%   61.96%   +0.15%     
==========================================
  Files         129      129              
  Lines       16295    16297       +2     
==========================================
+ Hits        10072    10099      +27     
+ Misses       6223     6198      -25     
Impacted Files Coverage Δ
astroquery/alma/core.py 41.35% <60.00%> (+4.69%) ⬆️
astroquery/conftest.py 92.30% <100.00%> (+0.64%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cbec82e...b2bd97c. Read the comment docs.

@andamian andamian marked this pull request as ready for review November 22, 2021 17:50
@andamian
Copy link
Author

@keflavich , @bsipocz - I think this is ready. Please have a look. Thanks

Copy link
Contributor

@keflavich keflavich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, just have one URL-related question

astroquery/alma/tests/test_alma_remote.py Outdated Show resolved Hide resolved
Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick, except that we shouldn't disable rst doctesting. I'll add that commit, and then merge.

"""
Returns an alma client class. `--alma-site` pytest option can be used
to have the client run against a specific site
:param request: pytest request fixture
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't matter here as we don't docstring test functions, but normally we use numpydoc style docstring formatting



def pytest_addoption(parser):
parser.addoption(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, this is nit pick, we don't use 'black', formatting so no need to put every arg in a new line 😄

setup.cfg Outdated
@@ -32,7 +32,7 @@ astropy_header = true
text_file_format = rst
xfail_strict = true
remote_data_strict = true
addopts = --doctest-rst
#addopts = --doctest-rst
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this comment, we started to doctest narrative docs for a couple of modules and eventually would like to see all of them being tested

setup.cfg Outdated Show resolved Hide resolved
@bsipocz
Copy link
Member

bsipocz commented Nov 23, 2021

I've also rebased to be able to add a changelog for the new pytest option.

@bsipocz
Copy link
Member

bsipocz commented Nov 23, 2021

Thanks @andamian!

@bsipocz bsipocz mentioned this pull request Nov 23, 2021
28 tasks
@andamian andamian deleted the almafix branch November 23, 2021 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants