omero-py icon indicating copy to clipboard operation
omero-py copied to clipboard

Gateway tests: remove logic downloading public DV files for testing

Open sbesson opened this issue 11 months ago • 9 comments

Opening as a draft initially as the initial implementation will fail some integration tests and this could use feedback from the OME team.

The OMERO.py gateway tests have historically been relying on downloading public DV samples as part of the fixture set-up. Recent connections issues between OME CI and OME downloads have exposed the fragility of this requirement.

This PR proposes to update the test set-up to use fake files exclusively and remove all downloads from an external source.

Having tested it locally, most gateway tests passed except for a few specific ones which rely on the metadata fields within the sample files

FAILED test/integration/gatewaytest/test_image.py::TestImage::testProperties - assert None is not None
FAILED test/integration/gatewaytest/test_image.py::TestImage::testPixelSizeUnits - assert None == 0.10639449954032898
FAILED test/integration/gatewaytest/test_image.py::TestImage::testUnitsGetValue - AttributeError: 'NoneType' object has no attribute 'getValue'
FAILED test/integration/gatewaytest/test_image.py::TestImage::testChannelWavelengthUnits - assert None == 360.0
FAILED test/integration/gatewaytest/test_pixels.py::TestPixels::testPlaneInfo - assert 0 == ((35 * 2) * 1)
FAILED test/integration/gatewaytest/test_rdefs.py::TestRDefs::testEmissionWave - assert None == 457
  • the pixel size tests should easy to fix by specifying the correct key value pairs test image format
  • similarly, it should be possible to populate Plane elements using the PositionX metadata of similar On the other hand, there is no provisioning for storing the channel wavelength.

sbesson avatar Jan 30 '25 17:01 sbesson

Sounds like a good idea. Could we bundle a tiny text ome.xml image into the repo to test Channel wavelengths? Or maybe just skip it if no other option?

will-moore avatar Jan 30 '25 21:01 will-moore

https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-test-integration/297 3 failures this morning

jburel avatar Jan 31 '25 06:01 jburel

In addition to the 2 tests I reported initially which I was expecting to fail after my last changes, OmeroPy.test.integration.gatewaytest.test_wrapper.TestWrapper.testAllObjectsWrapped is also attempting to retrieve metadata (an instrument) which does not exist in these fake files.

I could use some guidance on how you want to proceed with next steps. Translating the DV into OME-XML as suggested in https://github.com/ome/omero-py/pull/443#issuecomment-2625658138 is an interesting idea as it would preserve the metadata. I had initially tried this route but realised these OME-XML files must be packaged as part of omero-py. Is this something you would consider and if so, should these be addedunder omero/gateway/scripts or elsewhere?

sbesson avatar Jan 31 '25 07:01 sbesson

Adding a test resources similar to the one in omero-common could be an option but that means that we will have 2 packages providing test files. In terms of maintenance this is not ideal How much work will it be to expand fake reader?

jburel avatar Feb 05 '25 13:02 jburel

Fine to exclude for now.

How much work will it be to expand fake reader?

Extending fake reader is doable but it would be good to define exactly what needs to happen before committing to any work. One quick thought is that the reader could make additional use of the XMLMockObjects API to create richer OME-XML metadata. XMLMockObjects.createInstrument() and XMLMockObjects.createChannel() would likely populate all the elements required for the last 3 failing integration tests but some of their assumptions might be incompatible with other options passed to the reader. Let's discuss on Monday at the Formats meeting

sbesson avatar Feb 05 '25 16:02 sbesson

This is in the same spirit that what has been done in https://github.com/ome/openmicroscopy/blob/develop/components/tools/OmeroJava/test/integration/ModelMockFactory.java Another could be to handle an "xml" key in the name of the fake file i.e. testimg&xml=BlobOfOMEXML.fake This could eventually replace the various existing conditions but that will be a broken change and probably not worth the effort

jburel avatar Feb 06 '25 08:02 jburel

https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-test-integration/lastCompletedBuild/testReport/

failed on setup with "OSError: [Errno 36] File name too long

I'll convert these into INI files and rework the logic

sbesson avatar Feb 19 '25 08:02 sbesson

After a bit of investigation, I decided to store the OME-XML metadata of the public sample DV files used for the gateway tests under src/omero/gateway/scripts/imgs. Together with the logic in https://github.com/ome/omero-py/blob/05d52412ff7d6c4e3a515717d2807b1a21bf4291/setup.py#L202, this files should be included in the package and can be used for import by the integration tests.

This means that this PR no longer depend on the new feature introduced in https://github.com/ome/bioformats/pull/4272.

sbesson avatar Feb 19 '25 10:02 sbesson

More failing tests in https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-test-integration/314/testReport/. Apart from OmeroPy.test.integration.gatewaytest.test_wrapper.TestWrapper.testAllObjectsWrapper which is a simply check on the format, half of the tests are failing due to checks on the channel start/end windows and the other half is failing due the unclosed services (as per the failures)

Using metadata-only OME-XML files as in ead90d4 is not sufficient for these rdef datasets. Converting the testimg into OME-XML with bfconvert results in a 42M image so that's not an option. I tried to convert tinyTest.d3d.dv into OME-XML including binary data and update the tests with the fixture but there are other assumptions in the integration tests about channel numbers which are now broken

Excluding again. Options to consider:

  • convert the testimg DV as OME-XML with BinaryData and update the rdef tests
  • use fake image with metadata for the rdef tests
  • close this PR and capture as an issue

sbesson avatar Feb 20 '25 08:02 sbesson

The latest version of these changes alongside https://github.com/ome/openmicroscopy/pull/6438 has been successfully executed as part of the nightly integration tests - https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-test-integration/496/.

The description has been updated to reflect the latest changes and this is now ready for another round of review

sbesson avatar Sep 22 '25 07:09 sbesson