-
Notifications
You must be signed in to change notification settings - Fork 33
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
Adds optional extra args to the util #241
Conversation
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.
Few minor suggestions. The location for the test seems fine. Thanks for taking this on!
@@ -512,3 +514,12 @@ def do_import(self, command_args, xargs, mode): | |||
self.args = ["mock-import", "-f", "---bulk=%s" % b] | |||
self.add_client_dir() | |||
self.cli.invoke(self.args, strict=True) | |||
|
|||
@pytest.mark.skipif(sys.platform == "win32", reason="Fails on Windows") |
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.
What's the failure on Windows? Something we can help with?
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.
The Travis traceback is here
E omero.InternalException: exception ::omero::InternalException
E {
E serverStackTrace = None
E serverExceptionClass = None
E message = 'import -f' exited with a rc=108. See console for more information
E }
Most of the tests in the class are decorated by the same skip instruction, maybe a filesystem issue?
Co-authored-by: Josh Moore <j.a.moore@dundee.ac.uk>
Co-authored-by: Josh Moore <j.a.moore@dundee.ac.uk>
I'll add test to check that the extra args are included correctly |
sorry for the messy diff, I passed black on the file automatically, I can revert that if you want. I don't know if further tests / doc are needed? |
Yeah, let's force push away the application of black here. We'll do that in concert with a GitHub Action to make sure it stays formatted. Otherwise, test looks good. Thanks. cc: @sbesson |
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.
Seconding #241 (comment), the API addition makes sense and the addition of unit tests for import_candidates
is welcome.
Just #241 (comment) needs addressing to fix the case where both readers
and extra_args
are passed to import_candidates
. Feel free to cherry-pick or adapt sbesson@4d33655 which adds some test coverage for the readers
logic and includes a failing test with the current implementation when both arguments are passed.
Thanks @glyg, merging for inclusion in the upcoming minor release of |
Great! Thanks to both of you! |
Follow up on #235
The extra test is very minimal for now, I wanted to get your input before doing more.
I choose to put it in
test_import.py
to avoid rewriting all the mock & setup machinery, but I'm not sure it's ok / good practice...