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.

Sign up for GitHub

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

tests: allow for parallel test execution #180

Closed
wants to merge 14 commits into from
Closed

tests: allow for parallel test execution #180

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
9ad5456
feat: update __init__.py to explicitly define exported members
jjjermiah Jan 17, 2025
b8c468d
refactor(tests): update test_ops.py to use tmp_path and improve outpu…
jjjermiah Jan 17, 2025
f16620e
feat(tests): enhance pytest configuration for coverage and parallel e…
jjjermiah Jan 17, 2025
86abb74
Merge remote-tracking branch 'origin/main' into jjjermiah/tests/allow…
jjjermiah Jan 17, 2025
9ce633c
chore: update coverage configuration and version to 1.14.0; add clean…
jjjermiah Jan 17, 2025
eaa9ac6
refactor(tests): update fixture scopes and improve path handling in c…
jjjermiah Jan 17, 2025
89bab4e
refactor(tests): change fixture scope to 'package' and add xdist_grou…
jjjermiah Jan 17, 2025
bf860db
refactor(tests): update pytest configuration for xdist group and impr…
jjjermiah Jan 17, 2025
02df8fe
refactor(tests): update dataset preparation fixtures and improve path…
jjjermiah Jan 17, 2025
00178ec
chore: update dependencies to include filelock version 3.16.1 and adj…
jjjermiah Jan 17, 2025
0d5199f
refactor(tests): improve test_components.py by updating path handling…
jjjermiah Jan 17, 2025
a8522b5
refactor(tests): remove temporary edge path after shape assertion in …
jjjermiah Jan 17, 2025
d7ca843
refactor(tests): enable update flag in TestComponents for dataset pro…
jjjermiah Jan 17, 2025
3d41103
refactor(tests): update file path in modalities_path function for con…
jjjermiah Jan 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion config/coverage.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ omit = [
"src/imgtools/logging/**/*.py",
"src/imgtools/cli/**/*.py",
"src/imgtools/dicom/index/**/*.py",
"tests/**/*.py",
]

[tool.coverage.report]
[tool.coverage.report]
11 changes: 10 additions & 1 deletion config/pytest.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ addopts =
--showlocals
# Generate coverage report
# Tracks code coverage during test execution
--cov=imgtools
--cov
# Output coverage report in terminal
# Provides immediate feedback on coverage
--cov-report=term-missing
Expand All @@ -27,6 +27,15 @@ addopts =
# Point to coverage config file
# Allows customization of coverage report generation
--cov-config=config/coverage.toml
# Append coverage data from previous runs
# Ensures coverage data is appended
--cov-append
# numprocessors to use for xdist plugin
# Sets number of processors to use for parallel test execution
--numprocesses=auto
# max processes
# Sets maximum number of processes to use for parallel test execution
--maxprocesses=8

# Patterns for test discovery
# Defines which files are considered test files
Expand Down
4 changes: 2 additions & 2 deletions pixi.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions pixi.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,10 @@ inputs = ["coverage-report/coverage.xml", "config/coverage.toml"]
depends-on = ["test"]
description = "Run pytest and generate coverage report"

[feature.test.tasks.clean_tests]
cmd = "rm -rf .pytest_cache ./data ./tests/temp"
description = "Clean up the test cache and data"

############################################## DOCS ###############################################
[feature.docs.dependencies]
mkdocs = "*"
Expand Down
42 changes: 41 additions & 1 deletion src/imgtools/ops/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,41 @@
from .ops import *
from .ops import (
BaseInput,
BaseOp,
BaseOutput,
CentreCrop,
ClipIntensity,
Crop,
HDF5Output,
ImageAutoInput,
ImageAutoOutput,
ImageStatistics,
MinMaxScale,
NumpyOutput,
Resample,
Resize,
StandardScale,
StructureSetToSegmentation,
WindowIntensity,
Zoom,
)

__all__ = [
"ImageAutoInput",
"ImageAutoOutput",
"StructureSetToSegmentation",
"BaseOp",
"BaseInput",
"BaseOutput",
"CentreCrop",
"ClipIntensity",
"Crop",
"HDF5Output",
"ImageStatistics",
"MinMaxScale",
"NumpyOutput",
"Resample",
"Resize",
"StandardScale",
"WindowIntensity",
"Zoom",
]
53 changes: 16 additions & 37 deletions tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,64 +8,43 @@
from imgtools.logging import logger


@pytest.fixture(scope='session')
@pytest.fixture(scope="session")
def curr_path():
return pathlib.Path(__file__).parent.parent.resolve().as_posix()


@pytest.fixture(scope='session')
@pytest.fixture(scope='package')
def dataset_path(curr_path):
quebec_path = pathlib.Path(curr_path, 'data', 'Head-Neck-PET-CT')
quebec_path = pathlib.Path(curr_path, "data", "Head-Neck-PET-CT")

if not (quebec_path.exists() and len(list(quebec_path.glob('*'))) == 2):
if not (quebec_path.exists() and len(list(quebec_path.glob("*"))) == 2):
quebec_path.mkdir(parents=True, exist_ok=True)

# Download QC dataset
logger.info('Downloading the test dataset...')
quebec_data_url = (
'https://github.com/bhklab/tcia_samples/blob/main/Head-Neck-PET-CT.zip?raw=true'
)
quebec_zip_path = pathlib.Path(quebec_path, 'Head-Neck-PET-CT.zip').as_posix()
logger.info("Downloading the test dataset...")
quebec_data_url = "https://github.com/bhklab/tcia_samples/blob/main/Head-Neck-PET-CT.zip?raw=true"
quebec_zip_path = pathlib.Path(quebec_path, "Head-Neck-PET-CT.zip").as_posix()
request.urlretrieve(quebec_data_url, quebec_zip_path)
with ZipFile(quebec_zip_path, 'r') as zipfile:
with ZipFile(quebec_zip_path, "r") as zipfile:
zipfile.extractall(quebec_path)
os.remove(quebec_zip_path)
# os.remove(quebec_zip_path)
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Remove the commented cleanup code for better code maintainability

The commented cleanup (os.remove(quebec_zip_path)) is not critical since:

  • The dataset fixture has package scope, meaning data persistence is intentional
  • The zip file is downloaded only once per test session
  • The disk space impact is minimal

Simply remove the commented line to maintain code cleanliness.

🔗 Analysis chain

Consider removing or justifying the commented cleanup code.

The commented-out cleanup code (os.remove(quebec_zip_path)) might lead to unnecessary disk space usage, especially in CI environments. If there's a specific reason for keeping the zip file, please document it; otherwise, consider removing the commented line.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if this cleanup is handled elsewhere in the codebase
rg "quebec.*\.zip.*remove" --type py

Length of output: 38

Script:

#!/bin/bash
# Get broader context around the Quebec zip handling
echo "=== Quebec zip file references ==="
rg "quebec.*\.zip" --type py -B 2 -A 2

echo -e "\n=== General zip cleanup patterns in conftest ==="
rg "\.zip.*remove|remove.*\.zip" tests/conftest.py

echo -e "\n=== Full context of the fixture ==="
cat tests/conftest.py

Length of output: 3062

else:
logger.info('Data already downloaded...')
logger.info("Data already downloaded...")

output_path = pathlib.Path(curr_path, 'tests', 'temp').as_posix()
output_path = pathlib.Path(curr_path, "tests", "temp").as_posix()
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use pytest's built-in temporary directory fixtures for better test isolation.

Instead of using a fixed 'temp' directory, consider using pytest's tmp_path or tmp_path_factory fixtures. This would provide better isolation for parallel test execution and automatic cleanup.

Example refactor:

-    output_path = pathlib.Path(curr_path, "tests", "temp").as_posix()
+    @pytest.fixture(scope="package")
+    def output_path(tmp_path_factory):
+        return tmp_path_factory.mktemp("output").as_posix()

Committable suggestion skipped: line range outside the PR's diff.

quebec_path = quebec_path.as_posix()

# Dataset name
dataset_name = os.path.basename(quebec_path)
imgtools_path = pathlib.Path(os.path.dirname(quebec_path), '.imgtools')
imgtools_path = pathlib.Path(os.path.dirname(quebec_path), ".imgtools")

# Defining paths for autopipeline and dataset component
crawl_path = pathlib.Path(imgtools_path, f'imgtools_{dataset_name}.csv').as_posix()
edge_path = pathlib.Path(imgtools_path, f'imgtools_{dataset_name}_edges.csv').as_posix()
crawl_path = pathlib.Path(imgtools_path, f"imgtools_{dataset_name}.csv").as_posix()
edge_path = pathlib.Path(
imgtools_path, f"imgtools_{dataset_name}_edges.csv"
).as_posix()
# json_path = pathlib.Path(imgtools_path, f"imgtools_{dataset_name}.json").as_posix() # noqa: F841

yield quebec_path, output_path, crawl_path, edge_path


@pytest.fixture(scope='session')
def modalities_path(curr_path):
qc_path = pathlib.Path(curr_path, 'data', 'Head-Neck-PET-CT', 'HN-CHUS-052')
assert qc_path.exists(), 'Dataset not found'

path = {}
path['CT'] = pathlib.Path(
qc_path, '08-27-1885-CA ORL FDG TEP POS TX-94629/3.000000-Merged-06362'
).as_posix()
path['RTSTRUCT'] = pathlib.Path(
qc_path,
'08-27-1885-OrophCB.0OrophCBTRTID derived StudyInstanceUID.-94629/Pinnacle POI-41418',
).as_posix()
path['RTDOSE'] = pathlib.Path(
qc_path,
'08-27-1885-OrophCB.0OrophCBTRTID derived StudyInstanceUID.-94629/11376',
).as_posix()
path['PT'] = pathlib.Path(
qc_path, '08-27-1885-CA ORL FDG TEP POS TX-94629/532790.000000-LOR-RAMLA-44600'
).as_posix()
return path
Loading
Loading