From 52cfa7eb85a71f65ae4b88bc539fd05c96f079ed Mon Sep 17 00:00:00 2001 From: mrbean-bremen Date: Wed, 31 Jan 2024 19:11:55 +0100 Subject: [PATCH] Fixed exception on parsing some older DICOM standard versions - this happened at least for revisions from 2015 to 2019 - add some parsing tests for older revision --- .github/workflows/testsuite.yml | 18 ++-- CHANGES.md | 5 ++ dicom_validator/spec_reader/part3_reader.py | 2 + dicom_validator/tests/spec_reader/conftest.py | 25 ++++-- .../tests/spec_reader/test_part3_reader.py | 90 +++++++++++++++---- dicom_validator/tests/test_cmdline_tools.py | 9 +- requirements-dev.txt | 1 + 7 files changed, 113 insertions(+), 37 deletions(-) diff --git a/.github/workflows/testsuite.yml b/.github/workflows/testsuite.yml index 6722453..54d1e9c 100644 --- a/.github/workflows/testsuite.yml +++ b/.github/workflows/testsuite.yml @@ -14,7 +14,6 @@ jobs: matrix: os: [ubuntu-latest, macOS-latest, windows-latest] python-version: ["3.8", "3.9", "3.10", "3.11", "3.12"] - dicom-version: ["2023c"] steps: - uses: actions/checkout@v4 - name: Set up Python ${{ matrix.python-version }} @@ -22,25 +21,26 @@ jobs: with: python-version: ${{ matrix.python-version }} - - name: Install dependencies - run: | - python -m pip install --upgrade pip - pip install -r requirements-dev.txt - pip install -e . - - name: Cache DICOM standard id: cache-dicom uses: actions/cache@v4 with: path: dicom_validator/tests/fixtures/standard - key: ${{ matrix.dicom-version }} + key: "2015b_2023c" enableCrossOsArchive: true - name: Download DICOM standard if: steps.cache-dicom.outputs.cache-hit != 'true' run: | pip install -e . - python .github/workflows/get_revision.py ${{ matrix.dicom-version }} "`pwd`/dicom_validator/tests/fixtures/standard" + python .github/workflows/get_revision.py 2015b "`pwd`/dicom_validator/tests/fixtures/standard" + python .github/workflows/get_revision.py 2023c "`pwd`/dicom_validator/tests/fixtures/standard" + + - name: Install dependencies + run: | + python -m pip install --upgrade pip + pip install -r requirements-dev.txt + pip install -e . - name: Run tests with coverage run: | diff --git a/CHANGES.md b/CHANGES.md index 85f22d5..09254d9 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,6 +1,11 @@ # Dicom Validator Release Notes The released versions correspond to PyPi releases. +## Unreleased + +### Fixes +* fixed exception on parsing some older DICOM standard versions + ## [Version 0.5.0](https://pypi.python.org/pypi/dicom-validator/0.5.0) (2024-01-25) Adds enum checks and fixes a regression. diff --git a/dicom_validator/spec_reader/part3_reader.py b/dicom_validator/spec_reader/part3_reader.py index e4202f9..b9c64ef 100644 --- a/dicom_validator/spec_reader/part3_reader.py +++ b/dicom_validator/spec_reader/part3_reader.py @@ -301,6 +301,8 @@ def _get_tag_name_and_level( # Pop off (delete) the last level_delta items. level_delta = current_level - level del current_descriptions[-level_delta:] + if not current_descriptions: + current_descriptions.append({}) return tag_name, level def _collect_modules(self, table_section, check_rowspan): diff --git a/dicom_validator/tests/spec_reader/conftest.py b/dicom_validator/tests/spec_reader/conftest.py index 4581210..14d235e 100644 --- a/dicom_validator/tests/spec_reader/conftest.py +++ b/dicom_validator/tests/spec_reader/conftest.py @@ -24,15 +24,29 @@ def standard_path(fixture_path): @pytest.fixture(scope="session") -def spec_fixture_path(standard_path): - yield standard_path / CURRENT_REVISION / "docbook" +def revision(request): + if hasattr(request, "param"): + revision = request.param + else: + revision = CURRENT_REVISION + yield revision @pytest.fixture(scope="session") -def dict_info(standard_path): +def revision_path(standard_path, revision): + yield standard_path / revision + + +@pytest.fixture(scope="session") +def spec_fixture_path(revision_path): + yield revision_path / "docbook" + + +@pytest.fixture(scope="session") +def dict_info(revision_path): from dicom_validator.spec_reader.edition_reader import EditionReader - json_fixture_path = standard_path / CURRENT_REVISION / "json" + json_fixture_path = revision_path / "json" with open(json_fixture_path / EditionReader.dict_info_json) as info_file: info = json.load(info_file) yield info @@ -40,7 +54,8 @@ def dict_info(standard_path): @pytest.fixture(scope="module") def spec_path(fs_module, spec_fixture_path): - fs_module.add_real_directory(spec_fixture_path) + if not fs_module.exists(spec_fixture_path): + fs_module.add_real_directory(spec_fixture_path) yield spec_fixture_path diff --git a/dicom_validator/tests/spec_reader/test_part3_reader.py b/dicom_validator/tests/spec_reader/test_part3_reader.py index 006efd5..6b7e0d6 100644 --- a/dicom_validator/tests/spec_reader/test_part3_reader.py +++ b/dicom_validator/tests/spec_reader/test_part3_reader.py @@ -1,6 +1,6 @@ -from xml.etree import ElementTree from pathlib import Path from unittest.mock import patch +from xml.etree import ElementTree import pytest @@ -48,24 +48,37 @@ def test_read_incomplete_doc_file(self, fs): with pytest.raises(SpecReaderParseError): reader.iod_description("A.6") - def test_lookup_sop_class(self, reader): + @pytest.mark.parametrize( + "revision,iod_name", + [("2015b", "Computed Tomography Image IOD"), ("2023c", "CT Image IOD")], + indirect=["revision"], + scope="session", + ) + def test_lookup_sop_class(self, reader, iod_name): with pytest.raises(SpecReaderLookupError): reader.iod_description("A.0") description = reader.iod_description(chapter="A.3") assert description is not None assert "title" in description - assert description["title"] == "CT Image IOD" - - def test_get_iod_modules(self, reader): + assert description["title"] == iod_name + + @pytest.mark.parametrize( + "revision,module_nr", + [("2015b", 27), ("2023c", 28)], + indirect=["revision"], + scope="session", + ) + def test_get_iod_modules(self, reader, module_nr): description = reader.iod_description(chapter="A.38.1") assert "modules" in description modules = description["modules"] - assert len(modules) == 28 + assert len(modules) == module_nr assert "General Equipment" in modules module = modules["General Equipment"] assert module["ref"] == "C.7.5.1" assert module["use"] == "M" + @pytest.mark.parametrize("revision", ["2015b", "2023c"], indirect=True) def test_optional_iod_module(self, reader): description = reader.iod_description(chapter="A.38.1") assert "modules" in description @@ -75,18 +88,30 @@ def test_optional_iod_module(self, reader): assert module["ref"] == "C.7.1.3" assert module["use"] == "U" - def test_iod_descriptions(self, reader): + @pytest.mark.parametrize( + "revision,desc_nr", + [("2015b", 110), ("2023c", 151)], + indirect=["revision"], + scope="session", + ) + def test_iod_descriptions(self, reader, desc_nr): descriptions = reader.iod_descriptions() - assert len(descriptions) == 151 + assert len(descriptions) == desc_nr assert "A.3" in descriptions assert "A.18" in descriptions assert "A.38.1" in descriptions - def test_group_macros(self, reader): + @pytest.mark.parametrize( + "revision,macro_nr", + [("2015b", 24), ("2023c", 27)], + indirect=["revision"], + scope="session", + ) + def test_group_macros(self, reader, macro_nr): descriptions = reader.iod_descriptions() assert not descriptions["A.3"]["group_macros"] enhanced_ct_macros = descriptions["A.38.1"]["group_macros"] - assert len(enhanced_ct_macros) == 27 + assert len(enhanced_ct_macros) == macro_nr pixel_measures = enhanced_ct_macros.get("Pixel Measures") assert pixel_measures assert pixel_measures["ref"] == "C.7.6.16.2.1" @@ -110,18 +135,30 @@ def test_group_macros(self, reader): assert condition.tag == "(0008,0008)" assert condition.values == ["ORIGINAL", "MIXED"] - def test_module_description(self, reader): + @pytest.mark.parametrize( + "revision,desc_nr", + [("2015b", 9), ("2023c", 9)], + indirect=["revision"], + scope="session", + ) + def test_module_description(self, reader, desc_nr): with pytest.raises(SpecReaderLookupError): reader.module_description("C.9.9.9") description = reader.module_description("C.7.1.3") - assert len(description) == 9 + assert len(description) == desc_nr assert "(0012,0031)" in description assert description["(0012,0031)"]["name"] == "Clinical Trial Site Name" assert description["(0012,0031)"]["type"] == "2" - def test_sequence_inside_module_description(self, reader): + @pytest.mark.parametrize( + "revision,desc_nr", + [("2015b", 3), ("2023c", 6)], + indirect=["revision"], + scope="session", + ) + def test_sequence_inside_module_description(self, reader, desc_nr): description = reader.module_description("C.7.2.3") - assert len(description) == 6 + assert len(description) == desc_nr assert "(0012,0083)" in description assert "items" in description["(0012,0083)"] sequence_description = description["(0012,0083)"]["items"] @@ -144,17 +181,29 @@ def test_referenced_macro(self, reader): assert len(description) == 20 assert "(0028,0103)" in description - def test_module_descriptions(self, reader): + @pytest.mark.parametrize( + "revision,desc_nr", + [("2015b", 451), ("2023c", 572)], + indirect=["revision"], + scope="session", + ) + def test_module_descriptions(self, reader, desc_nr): descriptions = reader.module_descriptions() - assert len(descriptions) == 572 - - def test_conditional_include_in_sr_module(self, reader): + assert len(descriptions) == desc_nr + + @pytest.mark.parametrize( + "revision,include_nr", + [("2015b", 9), ("2023c", 10)], + indirect=["revision"], + scope="session", + ) + def test_conditional_include_in_sr_module(self, reader, include_nr): description = reader.module_description("C.17.3") assert "include" in description assert "C.17-5" in [d["ref"] for d in description["include"]] description = reader.module_description("C.17-5") assert "include" in description - assert len(description["include"]) == 10 + assert len(description["include"]) == include_nr # all macros are included conditionally assert all("cond" in d for d in description["include"]) @@ -176,6 +225,9 @@ def test_parsed_enum_values(self, reader): enums = description["(0082,0036)"]["enums"] assert enums == ["FAILURE", "WARNING", "INFORMATIVE"] + @pytest.mark.parametrize( + "revision", ["2015b", "2023c"], indirect=True, scope="session" + ) def test_linked_enum_values(self, reader): description = reader.module_description("10.24") assert "(300A,0450)" in description # Device Motion Control Sequence diff --git a/dicom_validator/tests/test_cmdline_tools.py b/dicom_validator/tests/test_cmdline_tools.py index f4354d1..f24e550 100644 --- a/dicom_validator/tests/test_cmdline_tools.py +++ b/dicom_validator/tests/test_cmdline_tools.py @@ -6,8 +6,6 @@ from dicom_validator.spec_reader.enum_parser import EnumParser from dicom_validator.validate_iods import main -CURRENT_REVISION = "2023c" - @pytest.fixture(scope="session") def fixture_path(): @@ -24,7 +22,10 @@ def dicom_fixture_path(fixture_path): yield fixture_path / "dicom" -def test_validate_sr(caplog, standard_path, dicom_fixture_path): +@pytest.mark.order(0) +@pytest.mark.parametrize("revision", ["2015b", "2023c"]) +def test_validate_sr(revision, caplog, standard_path, dicom_fixture_path): + # test also for 2015b to test an issue causing an exception rtdose_path = dicom_fixture_path / "rtdose.dcm" # recreate json files to avoid getting the cached ones # relies on the fact that this test is run first @@ -32,7 +33,7 @@ def test_validate_sr(caplog, standard_path, dicom_fixture_path): "-src", str(standard_path), "-r", - CURRENT_REVISION, + revision, "--recreate-json", str(rtdose_path), ] diff --git a/requirements-dev.txt b/requirements-dev.txt index 6b2e5d3..df6c9f4 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -2,3 +2,4 @@ pydicom>=2.3.0 pyfakefs>=3.4.1 pytest pytest-cov +pytest-order