Conversation
…module. Refactored test cases and classes in the script to map the API of image.py. Updated test_image_xds.py with new testing functionality.
Updated README.md.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
|
|
@smcastro I went ahead and added copilot as a reviewer since you seemed interested in doing that. |
There was a problem hiding this comment.
Pull request overview
Refactors the image unit tests to remove duplicated helper code and centralize reusable image-test utilities under xradio.testing.image, aligning with the approach requested in Issue #564 for reuse across xradio and external repositories.
Changes:
- Reworked
tests/unit/image/test_image.pyinto pytest-style classes and replaced in-file helpers with imports fromxradio.testing.image. - Simplified
test_image_xds.pyby reusing shared empty-image generation logic and improving parametrization. - Added a shared
conftest.pyfixture for the Dask client and introduced a newxradio.testing.imagehelper package (IO, generators, assertions) with documentation.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/image/test_image.py | Converts to pytest classes and consumes newly centralized image test helpers. |
| tests/unit/image/test_image_xds.py | Removes duplicated empty-image args; reuses shared generator and improves parametrized coverage. |
| tests/unit/image/conftest.py | Adds module-scoped Dask client fixture shared by image tests. |
| src/xradio/testing/README.md | Documents the new testing.image submodule and its public helper API. |
| src/xradio/testing/image/io.py | Adds download/open/remove helpers for image test assets. |
| src/xradio/testing/image/generators.py | Adds empty-image and FITS guard test generators. |
| src/xradio/testing/image/assertions.py | Adds image-specific comparison helpers used by tests/benchmarks. |
| src/xradio/testing/image/init.py | Re-exports the public API for xradio.testing.image. |
| src/xradio/testing/init.py | Exposes xradio.testing.image at the package level. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/xradio/testing/image/io.py
Outdated
| def download_and_open_image(fname: str, directory: str | Path = "."): | ||
| """Download an image asset and return it as an opened ``xr.Dataset``. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| fname : str | ||
| Name of the image asset to download. | ||
| directory : str or Path, optional | ||
| Target directory. Defaults to the current working directory. | ||
|
|
||
| Returns | ||
| ------- | ||
| xr.Dataset | ||
| The opened image dataset. | ||
| """ | ||
| from xradio.image import open_image | ||
|
|
||
| path = download_image(fname, directory=directory) | ||
| return open_image(str(path)) |
There was a problem hiding this comment.
download_and_open_image is part of the new public testing helpers but it lacks a return type annotation (unlike download_image / download_measurement_set). Adding -> xr.Dataset (possibly via a TYPE_CHECKING import to avoid importing xarray at module import time) would improve type-checking and API clarity.
src/xradio/testing/__init__.py
Outdated
| __all__ = [ | ||
| "assert_attrs_dicts_equal", | ||
| "assert_xarray_datasets_equal", | ||
| # image sub-package (imported by name so external projects can use it) | ||
| "image", | ||
| ] | ||
|
|
||
| import xradio.testing.image # noqa: F401, E402 – register the sub-package |
There was a problem hiding this comment.
xradio.testing.__all__ exports image, but the module uses import xradio.testing.image with noqa flags to make it available. This style is inconsistent with other package initializers in the repo (which use relative imports) and it also introduces a xradio name in this module’s namespace. Prefer from . import image # noqa: F401 (or similar) so the exported name is bound explicitly without needing E402 suppression.
|
Hi @smcastro |
Copilot review 1. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There is a more detailed explanation of what has changed in this branch in the GitHub issue #564