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

Test cli #5548

Merged
merged 2 commits into from
Nov 23, 2017
Merged

Test cli #5548

merged 2 commits into from
Nov 23, 2017

Conversation

jburel
Copy link
Member

@jburel jburel commented Oct 13, 2017

What this PR does

  • Add cli.py to testlib
  • Deprecate cli.py in test/clitest/cli.py
  • Update the tests to use testlib/cli.py

Testing this PR

Make sure the tests are green

Related reading

ome/omero-cli-render#1

cc @joshmoore

@sbesson
Copy link
Member

sbesson commented Oct 16, 2017

Following my comment on #5499 (comment), if we are moving these specialized ITest classes to the omero.testlib component and making them part of the official test API, would it be the occasion to review the AbstractCLITest > {CLITest,RootCLITest} hierarchy to see if we could reduce it to one class only.

@jburel
Copy link
Member Author

jburel commented Oct 16, 2017

@sbesson I missed your comment on the other PR, it makes sense.
I was going for the "less" disruptive option but better to take the time to clean it up
Note that the same will be done for "script.py" so we can run the tests for scripts in travis too

@dominikl
Copy link
Member

So the integration tests stay in components/tools/OmeroPy/test/integration/clitest/ but this cli.py file goes from components/tools/OmeroPy/test/integration/clitest/ to components/tools/OmeroPy/src/omero/testlib/, what's the reason behind that?

@jburel
Copy link
Member Author

jburel commented Oct 17, 2017

so it can be used by any CLI plugin e.g.
ome/omero-cli-render#1
In the PR above I had to copy cli.py in the https://github.com/ome/omero-cli-render repo in order to run the integration tests in travis
This is impractical since the same file will need to be present in all plugins in order to run the tests

@@ -21,6 +21,7 @@


import pytest
import warnings
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than warning, this file could likely just import the testlib names as aliases.

@snoopycrimecop
Copy link
Member

snoopycrimecop commented Nov 20, 2017

Conflicting PR. Removed from build OMERO-DEV-merge-push#859. See the console output for more details.
Possible conflicts:

--conflicts Conflict resolved in build OMERO-push#2. See the console output for more details.

@joshmoore joshmoore changed the base branch from develop to clisplit November 21, 2017 11:06
@joshmoore
Copy link
Member

Merging to clitest. TODOs captured in https://trello.com/c/pCaBmwTV/41-pre-merge-review-test-class-hierarchy

I'll remove cli.py from omero-cli-render and see if I can't get it passing.

@joshmoore joshmoore merged commit 54d6f3b into ome:clisplit Nov 23, 2017
@joshmoore joshmoore added this to the 5.4.2 milestone Jan 16, 2018
@jburel jburel deleted the test-cli branch September 25, 2018 13:47
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.

None yet

5 participants