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.

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

Gateway tests: remove logic downloading public DV files for testing #443

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

sbesson
Copy link
Member

@sbesson sbesson commented Jan 30, 2025

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.

@will-moore
Copy link
Member

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?

@sbesson sbesson force-pushed the gateway_tests_fake branch from 0883934 to 2fd45b5 Compare January 30, 2025 22:44
@sbesson sbesson marked this pull request as ready for review January 30, 2025 22:45
@jburel
Copy link
Member

jburel commented Jan 31, 2025

@sbesson
Copy link
Member Author

sbesson commented Jan 31, 2025

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 #443 (comment) 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?

@jburel
Copy link
Member

jburel commented Feb 5, 2025

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?

@sbesson
Copy link
Member Author

sbesson commented Feb 5, 2025

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

@jburel
Copy link
Member

jburel commented Feb 6, 2025

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

@ome ome deleted a comment from jburel Feb 18, 2025
@sbesson
Copy link
Member Author

sbesson commented Feb 19, 2025

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
Copy link
Member Author

sbesson commented Feb 19, 2025

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

'omero.gateway.scripts': ['imgs/*']},
, 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 ome/bioformats#4272.

@sbesson sbesson removed the exclude label Feb 19, 2025
@sbesson
Copy link
Member Author

sbesson commented Feb 20, 2025

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)

TDLR: 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants