Skip to content

Add support for TimestampAnnotation#99

Merged
erickmartins merged 2 commits intoome:mainfrom
jmuhlich:timestamp-annotations
Apr 15, 2025
Merged

Add support for TimestampAnnotation#99
erickmartins merged 2 commits intoome:mainfrom
jmuhlich:timestamp-annotations

Conversation

@jmuhlich
Copy link
Contributor

No description provided.

@jmuhlich jmuhlich force-pushed the timestamp-annotations branch from 91da59c to 3edb24c Compare February 26, 2025 15:22
@erickmartins
Copy link
Collaborator

This looks pretty straightforward - can you add a test or two just to make sure it's working as intended?

@jmuhlich
Copy link
Contributor Author

jmuhlich commented Mar 6, 2025

Sure, where should I add the tests? There are a lot of moving parts to this testing infrastructure! I assume I should test both packing and unpacking a TimestampAnnotation.

@jmuhlich
Copy link
Contributor Author

jmuhlich commented Mar 6, 2025

I should add that I can successfully run the tests locally with .omero/docker, so I just need some guidance on where to add my new tests and how to make use of your existing fixtures etc.

@erickmartins
Copy link
Collaborator

I'd recommend adding a TimestampAnnotation to the pack in test/data/valid_single_image and adjusting test_unpack_folder under test/integration/test_transfer.py to check it's there - that should test at least the unpack side. The pack side testing infra is still not very good, but you can implicitly test for it by generating what you're adding to test/data/valid_single_image with your current pack version.

@jmuhlich
Copy link
Contributor Author

jmuhlich commented Mar 6, 2025

Thanks. Do you have a way to achieve a quicker edit/test iteration loop? Waiting for the entire .omero/docker cli setup+run+teardown for just one test is agonizing. I found out about .omero/docker dev and worked out a hacky way to run a single test repeatedly with a persistent container fleet, but I feel like I'm still missing something.

@erickmartins
Copy link
Collaborator

my solution is also hacky: manually edit the pytest command in .omeroci/cli-build. 🙃

@joshmoore
Copy link
Member

omero-test-infra is definitely for bots and not mere mortals, but there are some proto-docs on developer mode -- https://github.com/ome/omero-test-infra#developer-mode

But essentially it's just docker with call backs. If you don't want to keep copying the edits & tests into the docker, add them as a mount into your image, then edit on the non-docker side and pytest within the container.

@jmuhlich
Copy link
Contributor Author

jmuhlich commented Mar 6, 2025

My solution so far is to first run this once to establish a persistent set of containers:

.omero/docker dev start_up
.omero/docker dev install
.omero/docker dev run py common
.omero/docker dev run py setup

Then run this every time I've made changes I want to test:

.omero/docker dev install && .omero/docker dev run py check && .omero/docker dev run --user cli build

Although the only way to run specific limited tests with this approach is to edit .omeroci/cli-build as Erick suggested.

I can also run .omero/docker dev sh and then paste some selected key lines from .omeroci/cli-build to get into a state where I can run pytest with whatever test names I want. (In theory I should also be able to run .omero/docker dev install in another terminal to update the code after making edits, as yet untested)

However I'm running into a bigger issue now: These tests might not be idempotent or fully independent of each other. I'm experiencing unexpected failures when running certain individual tests by themselves or when running the same test repeatedly.

@joshmoore
Copy link
Member

I'm experiencing unexpected failures when running certain individual tests by themselves or when running the same test repeatedly.

Assumptions, e.g., on the ID numbers? The only thing I can imagine is for the suite to first load the lowest ID and build from there, but that's going to be very finicky.

@jmuhlich
Copy link
Contributor Author

jmuhlich commented Mar 6, 2025

No, weird stuff like the implicit omero session not being available if I run an arbitrary test:

pytest test/integration/test_transfer.py::TestTransfer::test_unpack_folder
...
stdin is not a terminal: cannot request server
...
1 failed

But it works if I also include the first test in this suite in the same run:

pytest test/integration/test_transfer.py::TestTransfer::test_non_existing_object[imageid] test/integration/test_transfer.py::TestTransfer::test_unpack_folder
...
2 passed

@jmuhlich jmuhlich force-pushed the timestamp-annotations branch from cea4dc2 to 091a35c Compare March 6, 2025 21:58
@joshmoore
Copy link
Member

Ah, ok. That seems like it should be more straight-forward, like a module setup is needed.

(Launched workflows)

@erickmartins erickmartins merged commit c2e221b into ome:main Apr 15, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants