Skip to content

Remove astroquery/utils/testing_tools.py#2287

Merged
bsipocz merged 3 commits into
astropy:mainfrom
eerovaher:rm-testing-tools
Feb 7, 2022
Merged

Remove astroquery/utils/testing_tools.py#2287
bsipocz merged 3 commits into
astropy:mainfrom
eerovaher:rm-testing-tools

Conversation

@eerovaher

Copy link
Copy Markdown
Member

The deleted file defined two pytest fixtures. Neither was used anywhere, and their functionality is provided by pytest-remotedata.

Closes #1134

@keflavich keflavich left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks. These were obsoleted by this functionality being incorporated somewhere into the astropy testing framework, though I don't remember where or when

@keflavich

Copy link
Copy Markdown
Contributor

(my approval is contingent on tests passing)


# Import MockResponse to keep the API while it's temporarily factored out to
# a separate file to avoid requiring pytest as a dependency in non-test code
from .mocks import MockResponse

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm OK with the removal of this file, if you also clean up the mock import in the code.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I ran the tests locally with

python setup.py test

and nothing failed. Running the tests with tox does lead to the same failures as were seen in CI.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think some temporary files may have been left behind when you tested locally, but not when it was a fully independent build for tox. I expect that the testing would have failed after a git clean.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I expect that the testing would have failed after a git clean.

Indeed they do.

@bsipocz

bsipocz commented Feb 7, 2022

Copy link
Copy Markdown
Member

Also a short changelog along the lines of "removal of obsolete testing tools" would be useful, added to the last changelog section.

The `MockResponse` class utilised in many tests is defined in
`astroquery.utils.mocks`, but many modules imported it via
`astroquery.utils.testing_tools`.
The deleted file defined two pytest fixtures. Neither was used anywhere,
and their functionality is provided by `pytest-remotedata`.
@codecov

codecov Bot commented Feb 7, 2022

Copy link
Copy Markdown

Codecov Report

Merging #2287 (0bd6724) into main (27515e4) will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2287      +/-   ##
==========================================
+ Coverage   62.72%   62.75%   +0.02%     
==========================================
  Files         131      130       -1     
  Lines       16854    16835      -19     
==========================================
- Hits        10572    10564       -8     
+ Misses       6282     6271      -11     
Impacted Files Coverage Δ
astroquery/utils/mocks.py 84.61% <ø> (ø)

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 27515e4...0bd6724. Read the comment docs.

@bsipocz bsipocz merged commit 08e57d7 into astropy:main Feb 7, 2022
@bsipocz

bsipocz commented Feb 7, 2022

Copy link
Copy Markdown
Member

Thanks @eerovaher!

@bsipocz bsipocz added this to the v0.4.6 milestone Feb 7, 2022
@eerovaher eerovaher deleted the rm-testing-tools branch February 21, 2022 13:09
syed-gilani pushed a commit to syed-gilani/astroquery that referenced this pull request Mar 11, 2022
mhsarmiento pushed a commit to esdc-esac-esa-int/astroquery that referenced this pull request May 24, 2022
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.

turn_on/off internet should use pytest-remotedata?

3 participants