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

Recommend a better way to mark remote data tests in remote test modules #2502

Merged
merged 2 commits into from
Sep 11, 2022

Conversation

eerovaher
Copy link
Member

Currently many test modules define a class that contains the test functions as methods. This has been convenient because marking the class with the pytest.mark.remote_data decorator applies the mark to all its methods, removing the need to mark each function individually. However, if all remote tests are in a separate module like the documentation already recommends then the pytestmark global variable could be used instead. The classes in remote test modules would not be needed and removing them would save one level of indentation.

This pull request updates the documentation to recommend pytestmark, and introduces it in the atomic sub-package as an example.

The testing documentation already recommends placing all remote tests in
a separate test module. This allows applying the `remote-data` mark to
all the test functions through the `pytestmark` global variable instead
of applying a decorator to each function separately.
The individual `@pytest.mark.remote_data` decorators have been removed
from `atomic` remote tests and are replaced with the `pytestmark` global
variable. This serves as an example other sub-packages can follow.
@codecov
Copy link

codecov bot commented Aug 25, 2022

Codecov Report

Merging #2502 (361b242) into main (0883e92) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2502   +/-   ##
=======================================
  Coverage   62.99%   62.99%           
=======================================
  Files         133      133           
  Lines       17291    17291           
=======================================
  Hits        10892    10892           
  Misses       6399     6399           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

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. We can slowly refactor other modules to follow this example in subsequent PRs

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.

This looks good. My only reservation is that a few modules have a bit of a hit-and-miss for separating online and local tests, and using this approach may skip more tests than is absolutely necessary.
The workaround for that is to be more watchful in review and spot when a test is being put in the incorrect test file.

@bsipocz bsipocz merged commit 1ff3b7f into astropy:main Sep 11, 2022
@eerovaher eerovaher deleted the remote-test-modules branch September 12, 2022 12:22
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.

None yet

3 participants