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

Disallow tests from downloading files while running tests #1591

Merged
merged 1 commit into from
Mar 12, 2021

Conversation

djhoese
Copy link
Member

@djhoese djhoese commented Mar 9, 2021

See #1587. Basically, developers should be writing tests that mock the necessary functions so actual data downloads don't happen while tests are being run. This PR adds a check to see if we are running tests and if the file being downloaded is a README file. The auxiliary download tests themselves use a README file as the only file to actually test downloading. If we're running tests and a download is requested then a RuntimeError is raised. Offline downloads are still allowed but must also be explicitly requested (with the configuration value).

CC @joleenf who had fears about this in her own PR.

@djhoese djhoese added enhancement code enhancements, features, improvements component:tests labels Mar 9, 2021
@djhoese djhoese requested a review from mraspaud March 9, 2021 19:32
@djhoese djhoese self-assigned this Mar 9, 2021
@codecov
Copy link

codecov bot commented Mar 9, 2021

Codecov Report

Merging #1591 (4bdd41d) into master (f56a089) will decrease coverage by 0.00%.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1591      +/-   ##
==========================================
- Coverage   92.72%   92.72%   -0.01%     
==========================================
  Files         253      254       +1     
  Lines       37576    37600      +24     
==========================================
+ Hits        34844    34866      +22     
- Misses       2732     2734       +2     
Flag Coverage Δ
behaviourtests 4.43% <8.33%> (+<0.01%) ⬆️
unittests 92.86% <91.66%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
satpy/conftest.py 71.42% <71.42%> (ø)
satpy/aux_download.py 88.03% <100.00%> (+0.53%) ⬆️
satpy/tests/test_data_download.py 100.00% <100.00%> (ø)

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 f56a089...4bdd41d. Read the comment docs.

@mraspaud
Copy link
Member

Could we use the satpy configuration system to do this instead?

@djhoese
Copy link
Member Author

djhoese commented Mar 11, 2021

In what way? A new config option or reuse the download_aux one for offline downloads? In the first case, the end result would be the same code but instead of a global variable being set by the test config module it would be setting a satpy config value.

I wanted to make sure there was a specific exception (not a warning) so that anyone writing unit tests knows exactly what is going wrong here. So that's why I didn't reuse the download_aux option.

There could be a new option that defaults to not letting tests download anything, but is toggled for the aux_download tests. This would be more flexible for future tests that would need to download something, but I'm not sure we need it right now.

Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

LGTM

@mraspaud mraspaud added this to the v0.26.0 milestone Mar 12, 2021
@mraspaud mraspaud merged commit 2a507fa into pytroll:master Mar 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:tests enhancement code enhancements, features, improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't allow auxiliary downloads during tests
2 participants