Improved external test data file handling#1828
Improved external test data file handling#1828bourque merged 8 commits intoIMAP-Science-Operations-Center:devfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
Moves the growing list of external test data file paths into a dedicated config file and updates fixtures and tests to use it.
- Extracted the external test data list into
external_test_data_config.py - Updated
_download_external_datafixture inconftest.pyto import and iterate overEXTERNAL_TEST_DATA - Cleaned up
test_codice_l1a.pyby removing manual download calls and adjusting the fixture return type
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| imap_processing/tests/external_test_data_config.py | New config module listing external test data files |
| imap_processing/tests/conftest.py | Fixture now uses EXTERNAL_TEST_DATA, removed old helper method |
| imap_processing/tests/codice/test_codice_l1a.py | Fixture signature updated to return list[xr.Dataset] |
Comments suppressed due to low confidence (2)
imap_processing/tests/conftest.py:110
- The docstring still refers to a
test_data_pathsparameter which no longer exists. Update it to describe thatEXTERNAL_TEST_DATAis used directly.
"""This fixture downloads externally-located test data files into a specific
imap_processing/tests/codice/test_codice_l1a.py:121
- [nitpick] Ensure your project runs on a Python version that supports built-in generic annotations (
list[xr.Dataset]), or switch tofrom typing import Listif compatibility is needed.
def test_l1a_data() -> list[xr.Dataset]:
tmplummer
left a comment
There was a problem hiding this comment.
I am happy with this as is, but made one recommendation for a slight tweak to further reduce clutter in the new config file.
| EXTERNAL_TEST_DATA = [ | ||
|
|
||
| # CoDICE | ||
| ("imap_codice_l0_raw_20241110_v001.pkts", imap_module_directory / "tests" / "codice" / "data" / "imap_codice_l0_raw_20241110_v001.pkts"), |
There was a problem hiding this comment.
Overall, this looks like a nice improvement. I'm wondering if it would be worth taking it a step further to make the tuple for a single file contain (<s3_filename>, <parent_test_dir_relative_to_imap_module_dir>). That would clean up this configuration by removing the imap_module_directory as well as the target path filename. The later part would require that the filename remains the same when downloaded.
For this line, it would look like:
| ("imap_codice_l0_raw_20241110_v001.pkts", imap_module_directory / "tests" / "codice" / "data" / "imap_codice_l0_raw_20241110_v001.pkts"), | |
| ("imap_codice_l0_raw_20241110_v001.pkts", "tests/codice/data"), |
There was a problem hiding this comment.
Good idea, I like it.
This PR changes how external test data files are defined -- I moved the list (which has grown quite large and may continue to grow) to its own file which is then imported into
conftest.py. I also added some CoDICE validation files so that they can be stored in S3 instead of within the repo.Closes #1537