Use cliff for CLI layer #100
Conversation
|
The coverage decreased... :( And I noticed that we couldn't see the source code on that site. For example, https://coveralls.io/builds/13385582/source?filename=stestr%2Fcommands%2Flast.py |
There was a problem hiding this comment.
So from a quick glance it looks good for the most part, I still need to do some manual testing for backwards compat (and also do some performance testing, to make sure there is little to no degradation there) The only other thing is a bunch of docs need to be updated with this. For example:
http://stestr.readthedocs.io/en/latest/internal_arch.html#cli-layer
http://stestr.readthedocs.io/en/latest/api.html#commands
You can push those doc changes up as another patch on the PR branch (to keep the doc updates separate but merge at the same time as the PR)
stestr/commands/__init__.py
Outdated
| from stestr.commands.run import run_command | ||
| from stestr.commands.slowest import slowest as slowest_command | ||
|
|
||
| __all__ = ['failing_command', 'init_command', 'last_command', |
There was a problem hiding this comment.
This is going to break backwards compatibility and external consumers. (like I'm pretty sure I used this in os-testr somewhere) We can't remove this interface to the command functions without a deprecation period.
There was a problem hiding this comment.
Oh, I got it. I see the point.
https://github.com/openstack/os-testr/blob/master/os_testr/ostestr.py#L156
setup.cfg
Outdated
| @@ -55,3 +64,6 @@ input_file = stestr/locale/stestr.pot | |||
| keywords = _ gettext ngettext l_ lazy_gettext | |||
| mapping_file = babel.cfg | |||
| output_file = stestr/locale/stestr.pot | |||
|
|
|||
| [wheel] | |||
| universal = 1 | |||
There was a problem hiding this comment.
Why is this needed? (I don't actually know what this does)
There was a problem hiding this comment.
oh, I just copy & paste from tempest code.. I should have removed this. I'll remove this.
|
OK, I'll push doc changes later. |
masayukig
force-pushed
the
use-cliff
branch
from
b83be5a
to
893f53f
Compare
|
Just wanted to let you know my thinking on the timing for this. I'm planning to push a release in the near future I'm mostly waiting on the open bug fix reviews to land and also for the --analyze-isolation bug to get fixed. After that release (which will likely be 1.1.0) I think we can start testing this for backwards compat and performance and move forward with it. |
|
Sure, yeah, this is a bit big change. I agree with you. We should really care about them for stable testing. |
|
ok, 1.1.0 is released now so this should be good to get ready to land. The issues from the preliminary review still need to be addressed as well as it needs to be rebased. |
|
I just updated the branch. I'm still not sure what is the best workflow with github.. |
This commit makes stestr use cliff[1] for CLI layer. The cliff project provides a lot of advantages for stestr cli which has subcommands. Instead of just using argparse, we should leverage cliff to provide a more polished CLI experience. [1] https://pypi.python.org/pypi/cliff Closes Issue mtreinish#62
This commit updates the docs for using cliff for CLI layer.
|
I already updated some docs and rebased on the latest master. @mtreinish, can you have a look at this? |
mtreinish
dismissed
their stale review
The lastest version has been updated to address earlier comments. Additional review still needed
|
Ok, I started doing some performance testing on things this afternoon. Using cliff seems to have a pretty noticeable performance penalty. I haven't tested that we've got 100% compatibility on all args yet, because this raised questions for me on whether this is worth the trade off of functionality for performance. The raw data from my tests is below: Using argparse: stestr --help: stestr list (stestr tests): stestr list (nova unit tests): stestr run (stestr tests): stestr run (nova unit tests): stestr last (stestr tests): stestr last (nova unit tests): stestr slowest (stestr tests): stestr slowest (nova unit tests): stestr failing (stestr tests): stestr failing (nova unit tests): With the Cliff Patch Applied stestr --help stestr list (stestr tests): stestr list (nova unit tests): stestr run (stestr tests): stestr run (nova unit tests) stestr last (stestr tests): stestr last (nova unit tests): stestr slowest (stestr tests): stestr slowest (nova unit tests): stestr failing (stestr tests): stestr failing (nova unit tests): |
|
oh, it's an interesting result. I'm thinking to do a performance test in my environment, too, just in case. |
|
The interesting thing is that the nova unit tests (which I just picked as an example for a very large test suite) didn't really show a real change in performance. Although I only did one sample because I was impatient and my laptop gets kinda hot running the tests(and occasionally thermally throttles) so it's not really reliable data. I'll try rerunning them on my desktop which is more powerful and unlikely to have cooling issues. The concerning result to me was that running stestr's tests (which is a more modestly sized test suite) took ~2x the time to finish. I think the results that are < 1 second don't really matter either way. If something goes from 0.2 seconds to 0.7 seconds I don't think that's a really big deal. |
|
I did similar tests. And I got an interesting result. The performances are almost same between applying the patch and not applying the patch. I'm not sure the reason why I got like this result, though.. |
|
Hmm, I wonder if it's the stdout redirect to /dev/null, I didn't do that in my testing. Do you get the same results with printing the output to the console? |
|
I got almost the same result with printing the output to my console, too. (I did only "stestr list" tests, though.) |
|
I'm wondering if we can see a testing time graph like on openstack-health. The data could be unstable, though. Do we need to extract data to make a graph from https://travis-ci.org/mtreinish/stestr/pull_requests manually..? |
|
We probably could build some external automation based on travis results for collecting an aggregating the data. There is an api for everything so there should be some way to trigger a worker to populate a subunit2sql db when a test job finishes. Alternatively if travis supports secrets (which I don't know if it does or not) we could just add the subunit2sql db population as a post-processing job in the definition. |
|
So the thing which is a known quantity is that there is a performance hit for stevedore to do the scan of the python namespace (so it can discover the plugins for the commands). So depending on how your python environment is setup this can take longer. Like if my python environment has a lot more packages installed the time to check them all will be longer. But, your results are significantly different from what I was seeing, I'm trying to figure out what else could be different. Were you running with py2 or py3? |
|
The above results are on py36. But I also did tests on py27, and I got a similar result to to my py36.
|
|
Ah, ok you're using real hardware for running your testing. Try it on your laptop and see if you've get similar results to me. My results might totally be invalid, but you're running things on machines with a lot more resources than my laptop. So I'm wondering if this only shows up on slower hardware. In the mean time I'll try running the tests on my bigger machines too. |
|
OK, I'll test it on my small virtualmachine later. |
I did that. So, the differences weren't that much than I expected.. |
|
Sorry this took so long for me to get back to. I did some more manual testing for backwards compat and it seems fine for the most part. There are a few small differences with exit codes which are different. There is also the shell mode difference, running stestr bare now opens up the shell mode, while doing it before printed the usage (and said there were missing arguments). So we probably should say the next release will be 2.0.0 to account for these user facing changes. As for the performance I'm still a bit concerned about it. Searching the entire python namespace for the entrypoints isn't free and I think the size of our local python environments may be accounting for the differences in our testing. I still need to do some more testing on that front, and if it becomes to be a big enough issue we can always revert this. |
|
Just running some more numbers on my desktop (which is way more powerful than my laptop) and it provides some more interesting insights: stestr unit tests: Cliff: Nova Unit Test argparse Cliff The difference in run time for the stestr test suite is still concerning to me, with the cliff patch it's ~70% slower. I'll ask some other people to do some performance comparisons to give us a better idea of what's going on. |
|
OK, thanks. I'm fine with reverting if it'll be a significant issue. |
This commit makes stestr use cliff[1] for CLI layer. The cliff project
provides a lot of advantages for stestr cli which has subcommands.
Instead of just using argparse, we should leverage cliff to provide a
more plished CLI experience.
[1] https://pypi.python.org/pypi/cliff
Closes Issue #62