Make Downloader Test Suite Resilient to ReadTimeout

[JaydenR2305]

Resolves requests.exceptions.ReadTimeout errors originating from downloader-related tests

Upon merging #2555 it was discovered that one of the associated tests can be flaky (see https://github.com/PlasmaPy/PlasmaPy/actions/runs/9349808820/job/25731939682?pr=2709#step:6:310). I propose we increase the timeout of the get request to 40 seconds to give the server ample time to reply.

Additionally, @pheuer suggested implementing a check to ensure if a connection can be established to the server within some timeframe, and simply @pytest.mark.skip any relevant tests if (here for my reference) a connection cannot be established.

Resolves #2711

[github-actions]

Thank you for submitting a pull request (PR) to PlasmaPy! ✨ The future of the project depends on contributors like you, so we deeply appreciate it! 🌱

Our contributor guide has information on:

• Getting ready to contribute and the code contribution workflow
• Using astropy.units and plasmapy.particles
• Coding tips and guidelines
• Writing documentation (in reStructuredText with numpydoc style docstrings).
• Writing tests using pytest
• Adding a changelog entry (needed except for minor changes)

Important

PlasmaPy recently switched to an src layout. The source code previously in plasmapy/ is now in src/plasmapy/. Tests are now in tests/. If you have previously done an editable installation, it will likely need to be re-done (i.e., with pip install -e .[tests,docs] in the top-level directory of the repository). The former plasmapy/ directory will need to be deleted manually in old clones because git does not track directories.

The bottom of this page shows several checks that are run for every PR. Don't worry if something broke! We break stuff all the time. 😺 Click on "Details" to learn why a check didn't pass. Please also feel free to ask for help. We do that all the time as well. 🌸 You can find us in our chat room or weekly community meeting & office hours. Here are some tips:

• Try fixing CI / Python 3.12 test failures first.
• Most pre-commit.ci - pr failures can be automagically fixed by commenting pre-commit.ci autofix below, followed by a git pull to bring the changes back to your computer. Please also see our pre-commit troubleshooting guide.
• If pre-commit.ci - pr says that a function is too long or complex, try breaking up that function into multiple short functions that each do one thing. See also these tips on writing clean scientific software.
• If the CI / Documentation check ends with a cryptic error message, check out our documentation troubleshooting guide.
• For a documentation preview, click on Details next to docs/readthedocs.org:plasmapy.

If this PR is marked as ready for review, someone should stop by to provide a code review and offer suggestions soon. ✅ If you don't get a review within a few days, please feel free to send us a reminder.

Please also use SI units within PlasmaPy, except when there is strong justification otherwise or in some examples.

We thank you once again!

[codecov]

Codecov Report

Attention: Patch coverage is 72.72727% with 3 lines in your changes missing coverage. Please review.

Project coverage is 95.11%. Comparing base (c39ccee) to head (4d08880).

❗ Current head 4d08880 differs from pull request most recent head dba85a6

Please upload reports for the commit dba85a6 to get more accurate results.

Files	Patch %	Lines
src/plasmapy/utils/data/downloader.py	62.50%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2710      +/-   ##
==========================================
- Coverage   95.14%   95.11%   -0.03%     
==========================================
  Files         107      107              
  Lines        9411     9417       +6     
  Branches     2168     2168              
==========================================
+ Hits         8954     8957       +3     
- Misses        276      279       +3     
  Partials      181      181              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[pheuer]

@JaydenR2305 Can you try leaving the timeout at 10s and instead doing something like

@pytest.fixture
def check_github_connection():
"""returns True if good connection to github, False otherwise"

@pytest.skipif(check_github_connection) 
def test_any_test_using_api_calls

My concern with increasing the timeout so long is that it will make CI take that much longer to run, and if a connection really takes > 10 s I'm not optimistic it will complete in 40s?

[JaydenR2305]

@pheuer Any idea what's going on with the test_floating_potential test? I feel like I've seen it error like this before out of the blue.

[pheuer]

@pheuer Any idea what's going on with the test_floating_potential test? I feel like I've seen it error like this before out of the blue.

I don't think so - is that in the Langmuir module? You might open an issue about that. Maybe there's a random component in the test (or using hypothesis) that is failing occasionally on some edge value?

[pheuer]

I suppose this has to be in this main file rather than in the test file since multiple different test files (anything using downloader) will need to access it?

[JaydenR2305]

Yep, we just import it from there into the test modules that use the downloader directly/indirectly. It would also theoretically be possible to define the connection skipif decorator in that module as well 🤔

[JaydenR2305]

PlasmaPy/src/plasmapy/utils/data/downloader.py

Lines 29 to 32 in 4d08880

	except (requests.exceptions.ConnectionError, requests.exceptions.ReadTimeout) as e:
	# TODO: logging library when??
	print(f"Failed to connect to GitHub API:\n{e}") # noqa: T201
	_API_CONNECTION_ESTABLISHED = False

Would it be fine if I just # coverage: ignore this?

[pheuer]

PlasmaPy/src/plasmapy/utils/data/downloader.py

Lines 29 to 32 in 4d08880

	except (requests.exceptions.ConnectionError, requests.exceptions.ReadTimeout) as e:
	# TODO: logging library when??
	print(f"Failed to connect to GitHub API:\n{e}") # noqa: T201
	_API_CONNECTION_ESTABLISHED = False

Would it be fine if I just # coverage: ignore this?

Yes, I don't see a good way to test this (other than simulating a lost connection somehow, but that seems unnecessarily complicated)

[pheuer]

@JaydenR2305 This all looks good to me - ready to merge?