-
Notifications
You must be signed in to change notification settings - Fork 10
Initial addition of integration tests #171
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
Conversation
ba6fd8e to
a01052a
Compare
|
@jburel @joshmoore Any ideas how I install |
|
Since you're running |
|
I'm looking at https://github.com/ome/omero-test-infra?tab=readme-ov-file#hooks and the scripts I see there don't include any that start with |
|
Nope, it didn't work. The file was copied over: but the test still failed as before: At this point it's probably easier for me to copy over the |
| # with this program; if not, write to the Free Software Foundation, Inc., | ||
| # 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. | ||
|
|
||
| # Code copied from omero-rois as I could not find a way to install it |
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.
This is still the case, despite cli-deps? Should I give it a try?
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.
@joshmoore Yes, please try it. Or just check I've got it right at 55e5624 above (and see the corresponding test failure)
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.
Ok. So I can get to:
test/integration/clitest/test_export.py::TestRender::test_export_zarr PASSED [ 33%]
test/integration/clitest/test_export.py::TestRender::test_export_plate PASSED [ 66%]
test/integration/clitest/test_export.py::TestRender::test_export_masks PASSED [100%]
with the following:
- remove the omero_rois file
- add omero_rois in setup.py as a main dependency
The reason that cli-deps didn't help is that https://github.com/ome/omero-test-infra/blob/master/docker#L213-L224 doesn't run "cli deps".
Either in cli-build or py-setup we need a command that installs the test dependencies. (Or we update the cli target to run cli-deps or similar)
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.
OK, thanks for testing.
I was reluctant to add omero_rois as a main dependency when it's only used for setting up tests.
But can do if that's preferable to the copying over the code I needed?
I opened a PR at ome/omero-test-infra#76 to add run cli deps but don't know if that helps immediately?
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.
Probably easiest is to just override one of the py-* commands with whatever would install the additional test requirement.
|
Conflicting PR. Removed from build OMERO-plugins-push#377. See the console output for more details.
|
|
Great, gha has stopped caching yesterday! https://gh.io/gha-cache-sunset |
|
Did you possible not add the |
|
@joshmoore - oops! Yes, thanks I missed that. Checked-out omero-test-infra into Not sure that the tests actually ran, or if they did, what the results were? |
|
Pushed a fix of |
|
Green! |
|
Wow, thanks @joshmoore! Any feedback on the tests themselves, or can we just get them in and go from there? There's one fix in the code was for exporting masks -> labels when theC is set on a mask - now theC is ignored (instead of exporting labels that have a sizeC > 1. Also this PR fixes handling of output path with |
joshmoore
left a comment
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.
Barring the one minor catch, I'd say let's get these in. Then we can build on top of them, e.g., as we move towards OME-Zarr 0.5 😄
| url="https://github.com/ome/omero-cli-zarr/", | ||
| setup_requires=["setuptools_scm==7.1.0"], | ||
| use_scm_version={"write_to": "src/omero_zarr/_version.py"}, | ||
| tests_require=["omero-py>=5.18.0", "pytest", "omero-rois"], |
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.
Oops. I think we wanted to omit this omero-rois, right?
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.
I'm happy to remove it. Could remove the whole line as it's not being used? Unless it's useful to someone running the tests locally to know that omero-rois is required?
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.
Good point. Let's leave it for local development. We can always review later. 👍
WIP - adding more tests...