-
Notifications
You must be signed in to change notification settings - Fork 102
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
Added tests for Datasetcolumn #6300
Conversation
… to 6e03efdcc80758d7dc02d8ec3899a6ea37fc1f80
…g older h5 tables
I don't see these new tests at https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-test-integration/984/testReport/OmeroPy.test.integration.tablestest.test_backwards_compatibility/ |
@muhanadz sent you an invite to the organization so that your PRs get included by default in the future |
Thanks @sbesson, I have accepted. Do I need to reopen this PR? |
No need to re-open, it should be automatically included in the nightly CI builds. |
Hi, this seems like a fair bit of code duplication to test one more column type. Is it the lack of other tables tests that prompted the reuse of backwards-compatibility tests or do we actually need to test the backwards compatibility with 5.10.3? |
@will-moore You are absolutely correct in that it's a fair bit of code duplication. I was under the impression that the tests written for 5_3_4 were mainly done in response to new column types being added. I also found out that any modification to the written functions to test dataset column would fail since the reference h5 tables are missing the column type (specifically here). So I wrote a new test with a new reference table and labeled the version to be whatever server I was on when I generated that table. Perhaps we don't need to reuse backward compatibility, I just wasn't exactly sure where to add tests for the dataset column. |
I don't know how important it is to test the backwards compatibility here (is it useful to keep the new test)? cc @joshmoore But I imagine the code you've added could be moved to a new |
…bles.py. New script simply tests the integrity of created table with all header types. Table is no longer uploaded to server.
@will-moore cc @joshmoore I have moved my tests to This test should be easier to modify or add to in case of new column types without needing to duplicate anything. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor suggestions. Only 1 tiny code fix.
components/tools/OmeroPy/test/integration/tablestest/test_tables.py
Outdated
Show resolved
Hide resolved
components/tools/OmeroPy/test/integration/tablestest/test_tables.py
Outdated
Show resolved
Hide resolved
components/tools/OmeroPy/test/integration/tablestest/test_tables.py
Outdated
Show resolved
Hide resolved
components/tools/OmeroPy/test/integration/tablestest/test_tables.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks. LGTM 👍 .
Let's just let the tests run once more before we merge...
What this PR does
Added tests for the new Datasetcolumn in response to ome/omero-py#309 along with an h5 reference table for tests. Created a new test and reference table to not overwrite old ones.
Testing this PR
Ran all tests in
/openmicroscopy/components/tools/OmeroPy/test/integration/tablestest
with pytest and omero-py from ome/omero-py#309 installed on the server and in the local venv:2 failures are expected as they are marked with
@pytest.mark.broken
.Related reading
Link to cards, tickets, other PRs:
PR in reference: ome/omero-py#309
DatasetColumn header definition: https://github.com/ome/omero-blitz/blob/bf2802814f4a1d5056c64ad825b583339f474ede/src/main/slice/omero/Tables.ice#L62
Omero-metadata ready to handle: https://github.com/ome/omero-metadata/blob/148b6a4d4e5cf1deb78e3f087b5907024c749828/src/omero_metadata/populate.py#L457