pydicom / dicom-validator

Simple DICOM validator based on DocBook DICOM specs
MIT License
25 stars 11 forks source link

Validate entire dataset #12

Closed feher-melos closed 2 years ago

feher-melos commented 2 years ago

This PR:

mrbean-bremen commented 2 years ago

Thanks for that! I didn't have a closer look yet (not sure if I'll get to it before the weekend), but it sounds good. I also may need some time to get back to this, as I wrote most of this several years ago...

Currently, there are several tests failing, I'm not sure if this a test problem, or if your fixes introduced regressions. Please add some tests for your fixes, too, if possible.

feher-melos commented 2 years ago

Where does the run_tests.py come from? I see that tox.ini refers that.

feher-melos commented 2 years ago

I'm not sure how to run the unit tests in the "official" way. Right now, I locally "fixed" each by adding the import unittest and the unittest.main() calls, and then I just run them directly from cmd line. They pass for me like that. So I wonder...

Also, I wonder if the jsons in dicom_validator/tests/fixtures/2021d/json should be updated, since the module_info.json was broken. Strangely, the unit tests seem to run fine locally for me with the current fixtures. Maybe accident.

mrbean-bremen commented 2 years ago

I'm not sure how to run the unit tests in the "official" way.

The tests are run just by running pytest top-level (as is done in the GitHub workflow), so no unittest.main() is needed. While almost all tests are written as unittest test cases, this is not needed anymore, and you can write your own tests without deriving from TestCase, and use pytest features, if you want.

Also, I wonder if the jsons in dicom_validator/tests/fixtures/2021d/json should be updated,

These are just some excerpts from the real data, and from an older standard (probably from 2016, when I wrote this) - I did this to save time and space for the tests / test fixtures, but I think this can be changed now. It could make sense to test against the specs from different years to catch any regressions due to changes in the standard. Maybe it would also make sense to download large test data from elsewhere, as it is done in pydicom, to avoid to blow up the repository size.

mrbean-bremen commented 2 years ago

Though I think it would make sense to limit the PR to only a few changes, and make changes for the test data etc. separately if needed - I'm generally a fan of incremental changes.

feher-melos commented 2 years ago

Though I think it would make sense to limit the PR to only a few changes, and make changes for the test data etc. separately if needed - I'm generally a fan of incremental changes.

Yes, same here. I try not to squeeze more irrelevant fixes into this PR :)

codecov-commenter commented 2 years ago

Codecov Report

Merging #12 (8bcb4aa) into main (6643237) will increase coverage by 0.29%. The diff coverage is 92.03%.

:exclamation: Current head 8bcb4aa differs from pull request most recent head ff34da9. Consider uploading reports for the commit ff34da9 to get more accurate results Impacted file tree graph

@@            Coverage Diff             @@
##             main      #12      +/-   ##
==========================================
+ Coverage   82.52%   82.81%   +0.29%     
==========================================
  Files          14       14              
  Lines        1196     1263      +67     
==========================================
+ Hits          987     1046      +59     
- Misses        209      217       +8     
Impacted Files Coverage Δ
dicom_validator/validator/iod_validator.py 91.69% <91.26%> (-1.46%) :arrow_down:
dicom_validator/spec_reader/edition_reader.py 74.50% <100.00%> (ø)
dicom_validator/spec_reader/part3_reader.py 91.57% <100.00%> (+0.04%) :arrow_up:
dicom_validator/validate_iods.py 90.62% <100.00%> (+0.30%) :arrow_up:
dicom_validator/validator/dicom_file_validator.py 97.43% <100.00%> (+0.13%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 6643237...ff34da9. Read the comment docs.

mrbean-bremen commented 2 years ago

Let me know when this is ready for review.