PIXL pipeline: make integration tests more realistic

[skeating]

Definition of Done / Acceptance Criteria

• test.dcm replaced with an image that can be fully processed and persisted by orthanc anon. Currently not an X-ray so being dropped
• refactor shell script for running integration tests (in test top level dir) to be pytests with package scoped fixtures for setup and teardown of docker. will be covered in Replace flaky ftp server in system tests #293
• check_entry_in_orthanc_anon.sh replaced by a test that tests that the orthanc anon does have the study persisted instead of checking that it received the message
• check_max_storage_in_orthanc_raw.sh replaced by a test that tests that older studies are removed from orthanc-raw when the max storage size is exceeded (Set orthanc raw maximum storage in Dockerfile #179)
• Test that messages are correctly deserialised in the RabbitMQ subscriber (from Refactor message serialisation and deserialisation #197)

Testing

New integration test should cover the whole pipeline.

Documentation

No response

Dependencies

Access to GAE05 UCLH-Foundry/the-rolling-skeleton#86

Details and Comments

From UCLH-Foundry/the-rolling-skeleton#54

Current integrations tests are in test, where data/test.dcm is only checked to have reached orthanc anon, but the image is never persisted because it doesn't have the correct dicom tags for it to be successfully processed.

We should replace shell scripts with pytests so that we can have consistent testing framework and make it easier to start testing other aspects e2e.

[jeremyestein]

Regarding replacing the shell scripts, I quite liked how we did it for Emap hoover:
A docker-compose.fake_services.yml file defining the fake services required for the tests, which you could rebuild and re-run as needed independently from running the actual tests. Then a separate test runner in your IDE. This makes it much quicker to run and re-run the tests (takes seconds) without bringing the required services up and down unnecessarily (can take many minutes).

I think we already have the former in the form of pixl_ehr/tests/docker-compose.yml and others.

I'd like to have a more general discussion on how our tests should be structured and when they should be run (full system tests for each PR commit seems kind of excessive).

[jstutters]

test.dcm refers to an image added to the fake VNA by test_processing.add_image_to_fake_vna(). add_image_to_fake_vna is getting CT_small.dcm from pydicom's test data. CT_small.dcm has the CT modality.

test/resources/Dicom{1,2}.dcm have the DX modality so they are potentially suitable for use in full system tests.

[jstutters]

test.dcm removed in commit ad83c16.

[jstutters]

Test of orthanc-raw storage management would appear to require fake DICOM data (unless anyone has a better idea?) so I'm back to working on that today.

[milanmlft]

Test of orthanc-raw storage management would appear to require fake DICOM data (unless anyone has a better idea?) so I'm back to working on that today.

Yeah I think it's worth actually checking if the recycling works as we expect, i.e. oldest files being removed in favour of newer ones. So ideally we would have a few DICOM datasets (3?) to test the recycling, where the storage limit is set so that only 2 can be retained, and then check if the oldest ones gets removed, the middle gets retained and the newest gets added.

[stefpiatek]

Yeah I think it's worth actually checking if the recycling works as we expect, i.e. oldest files being removed in favour of newer ones. So ideally we would have a few DICOM datasets (3?) to test the recycling, where the storage limit is set so that only 2 can be retained, and then check if the oldest ones gets removed, the middle gets retained and the newest gets added.

Veering into testing a library's own code territory here. Though have tested in production with a 100 mb and its doing as we'd expect!

[jstutters]

#278 tests the recycling and adds some hopefully useful functionality for generating test DICOMs that we can make use of elsewhere.

[milanmlft]

Think we can close this one, no?

[stefpiatek]

Agreed!