| Message ID | 20181109221213.7310-1-crosa@redhat.com |
|---|---|
| Headers | show |
| Series | Acceptance tests for qemu-img | expand |
Am 09.11.2018 um 23:12 hat Cleber Rosa geschrieben: > The initial goal of this RFC is to get feedback on tests not specific > to the QEMU main binary, but specific to other components such as > qemu-img. > > For this experiment, a small issue with the zero and negative number > of I/O operations given to the bench command was chosen. Any reason why this shouldn't be in qemu-iotests? Kevin
On 11/12/18 5:49 AM, Kevin Wolf wrote: > Am 09.11.2018 um 23:12 hat Cleber Rosa geschrieben: >> The initial goal of this RFC is to get feedback on tests not specific >> to the QEMU main binary, but specific to other components such as >> qemu-img. >> >> For this experiment, a small issue with the zero and negative number >> of I/O operations given to the bench command was chosen. > > Any reason why this shouldn't be in qemu-iotests? > > Kevin > Hi Kevin, This is indeed one of the comments I was expecting to receive. AFAIK, there's nothing that prevents such a *simple* test to be written as a qemu-iotest. Having said that, one of the things we're trying to achieve with "tests/acceptance" is that a individual developer or maintainer, should be able to run a subset of tests that he/she cares about. Suppose that this developer is working on a "snapshot" related feature, and wants to run tests that cover both "qemu-img snapshot" and then tests interacting with a guest running on a snapshotted image. By using the tags mechanism, one could run: $ avocado run -t snapshot tests/acceptance And run all tests related to snapshot. This is one of the reasons for maybe allowing the type of test proposed here to live under "tests/acceptance". Others include: * No numbering conflicts when naming tests * More descriptive tests names and metadata * No "context switch" for people also writing acceptance tests * The various utility APIs available in both the Test class and on avocado.utils BTW, since most tests Today exist outside of "tests/acceptance", that may be also be solved in a great part by adding support in the (Avocado) test runner about some metadata in tests such qemu-iotests. Cheers, - Cleber.
On Mon, Nov 12, 2018 at 09:59:56AM -0500, Cleber Rosa wrote: > > On 11/12/18 5:49 AM, Kevin Wolf wrote: > > Am 09.11.2018 um 23:12 hat Cleber Rosa geschrieben: > >> The initial goal of this RFC is to get feedback on tests not specific > >> to the QEMU main binary, but specific to other components such as > >> qemu-img. > >> > >> For this experiment, a small issue with the zero and negative number > >> of I/O operations given to the bench command was chosen. > > > > Any reason why this shouldn't be in qemu-iotests? > > > > Kevin > > > > Hi Kevin, > > This is indeed one of the comments I was expecting to receive. AFAIK, > there's nothing that prevents such a *simple* test to be written as a > qemu-iotest. > > Having said that, one of the things we're trying to achieve with > "tests/acceptance" is that a individual developer or maintainer, should > be able to run a subset of tests that he/she cares about. > > Suppose that this developer is working on a "snapshot" related feature, > and wants to run tests that cover both "qemu-img snapshot" and then > tests interacting with a guest running on a snapshotted image. By using > the tags mechanism, one could run: > > $ avocado run -t snapshot tests/acceptance > > And run all tests related to snapshot. This is one of the reasons for > maybe allowing the type of test proposed here to live under > "tests/acceptance". Others include: > > * No numbering conflicts when naming tests > * More descriptive tests names and metadata I've long thought we should change the naming for tests/qemu-iotests to have descriptive names instead just forever clashing 3-digit numbers that generate needless merge conflits We also already have a tagging concept for iotests in the "groups" file. Thus far we've only used a few generic names, but we could expand that at will, so no reason why we couldn't have a "snapshot" group listed there. Personally I'd get rid of the groups file though, and just use magic comments at the top of each test. This would again reduce needless merge conflicts. If people are doing work on the block layer I think they'll already be used to runnng qemu-iotests as their acceptance test check. There are already many there which only focus on qemu-img/qemu-io, and not the QEMU system emulators > * No "context switch" for people also writing acceptance tests > * The various utility APIs available in both the Test class and on > avocado.utils > > BTW, since most tests Today exist outside of "tests/acceptance", that > may be also be solved in a great part by adding support in the (Avocado) > test runner about some metadata in tests such qemu-iotests. Regards, Daniel
Am 12.11.2018 um 15:59 hat Cleber Rosa geschrieben: > > On 11/12/18 5:49 AM, Kevin Wolf wrote: > > Am 09.11.2018 um 23:12 hat Cleber Rosa geschrieben: > >> The initial goal of this RFC is to get feedback on tests not specific > >> to the QEMU main binary, but specific to other components such as > >> qemu-img. > >> > >> For this experiment, a small issue with the zero and negative number > >> of I/O operations given to the bench command was chosen. > > > > Any reason why this shouldn't be in qemu-iotests? > > > > Kevin > > > > Hi Kevin, > > This is indeed one of the comments I was expecting to receive. I expected that you should expect this question. So it surprised me to see that the cover letter didn't address it at all. > AFAIK, there's nothing that prevents such a *simple* test to be > written as a qemu-iotest. Tests for qemu-img are pretty much by definition simple, just because qemu-img isn't very complex (in particular, it doesn't run guests which could introduce arbitrary complexity). Can you give an example of what you would consider a not simple test for qemu-img? > Having said that, one of the things we're trying to achieve with > "tests/acceptance" is that a individual developer or maintainer, should > be able to run a subset of tests that he/she cares about. > > Suppose that this developer is working on a "snapshot" related feature, > and wants to run tests that cover both "qemu-img snapshot" and then > tests interacting with a guest running on a snapshotted image. By using > the tags mechanism, one could run: > > $ avocado run -t snapshot tests/acceptance You mean like './check -g snapshot'? (It would be more useful if we had cared enough to actually assign that group to some of the newer tests, but it exists...) > And run all tests related to snapshot. This is one of the reasons for > maybe allowing the type of test proposed here to live under > "tests/acceptance". Others include: > > * No numbering conflicts when naming tests > * More descriptive tests names and metadata Test numbering and metadata - sure, we can change that in qemu-iotests. Should be a lot easier than adding a whole new second infrastructure for block tests. > * No "context switch" for people also writing acceptance tests There are no people writing "acceptance tests" for the block layer yet. The context switch comes only with your patches, since you are introducing a second competing framework for the same task, without even giving a clear path of how to integrate or convert the existing tests so we could get back to a unified world. I really don't think fragmenting the test infrastructure is a good idea, especially for rather superficial advantages. > * The various utility APIs available in both the Test class and on > avocado.utils Can you give examples? Are those utility APIs actually worth losing the existing iotests.py functions that provide stuff that is pretty specific to the QEMU and the block layer? > BTW, since most tests Today exist outside of "tests/acceptance", that > may be also be solved in a great part by adding support in the (Avocado) > test runner about some metadata in tests such qemu-iotests. Sure, feel free to wrap qemu-iotests as much as you want. Trivial wrappers don't sound like a big maintenance burden. I just think that the block layer tests themselves should still keep living in a single place. The obvious one is qemu-iotests. If you want to change that, your cover letter shouldn't be quite as terse. The least I would expect is an elaborate answer to: 1. Why is a change to something completely new useful and worth the effort? We don't generally rewrite QEMU or the kernel if some parts of it are ugly. We incrementally improve it instead. Exceptions need good justification because rewrites come with costs, especially if they can't offer feature parity. 2. How do we migrate the existing tests to the new infrastructure to avoid fragmentation? Kevin
On 11/12/18 11:00 AM, Kevin Wolf wrote: > Am 12.11.2018 um 15:59 hat Cleber Rosa geschrieben: >> >> On 11/12/18 5:49 AM, Kevin Wolf wrote: >>> Am 09.11.2018 um 23:12 hat Cleber Rosa geschrieben: >>>> The initial goal of this RFC is to get feedback on tests not specific >>>> to the QEMU main binary, but specific to other components such as >>>> qemu-img. >>>> >>>> For this experiment, a small issue with the zero and negative number >>>> of I/O operations given to the bench command was chosen. >>> >>> Any reason why this shouldn't be in qemu-iotests? >>> >>> Kevin >>> >> >> Hi Kevin, >> >> This is indeed one of the comments I was expecting to receive. > > I expected that you should expect this question. So it surprised me to > see that the cover letter didn't address it at all. > I hope you don't blame me for trying to have the advantage of the counter answer. :) >> AFAIK, there's nothing that prevents such a *simple* test to be >> written as a qemu-iotest. > > Tests for qemu-img are pretty much by definition simple, just because > qemu-img isn't very complex (in particular, it doesn't run guests which > could introduce arbitrary complexity). > > Can you give an example of what you would consider a not simple test for > qemu-img? > This is a hard question for me to answer since I haven't written that many qemu-img tests. Thinking of hypothetical situations, the Avocado libraries contain utilities for logical volumes[1], so it'd naturally be easier to write a test for qemu-img + LVs. >> Having said that, one of the things we're trying to achieve with >> "tests/acceptance" is that a individual developer or maintainer, should >> be able to run a subset of tests that he/she cares about. >> >> Suppose that this developer is working on a "snapshot" related feature, >> and wants to run tests that cover both "qemu-img snapshot" and then >> tests interacting with a guest running on a snapshotted image. By using >> the tags mechanism, one could run: >> >> $ avocado run -t snapshot tests/acceptance > > You mean like './check -g snapshot'? (It would be more useful if we had > cared enough to actually assign that group to some of the newer tests, > but it exists...) > Yes, but also something equivalent to './check -g snapshot,live -g quick,-privileged', meaning tests that are tagged both with "snapshot and live", in addition to tests that are quick and don't require super user privilege. Don't get me wrong, I wouldn't expect "check" to implement that logic. Like I said before, one way to solve that is to add leave qemu-iotests untouched, and add support on the Avocado runner to understand other tests' metadata. >> And run all tests related to snapshot. This is one of the reasons for >> maybe allowing the type of test proposed here to live under >> "tests/acceptance". Others include: >> >> * No numbering conflicts when naming tests >> * More descriptive tests names and metadata > > Test numbering and metadata - sure, we can change that in qemu-iotests. > Should be a lot easier than adding a whole new second infrastructure for > block tests. > My impression is that the "infrastructure for block tests" is not that different from the infrastructure needed by other tests, specially other QEMU tests. The point I'm trying to make here is that, adding a feature such as metadata parsing/selection to tests looks much more like a *test infrastructure* issue than a "block test" issue, right? Having access to local or remote files (images?), preparing the environment (creating those images?), accepting parameters from the user (which image format to use?) are all examples of test infrastructure problems applied to the block tests. Logging and formatting the results are other examples of *test* infrastructure problems that "check" has had to deal with itself, and is quite understandably limited. >> * No "context switch" for people also writing acceptance tests > > There are no people writing "acceptance tests" for the block layer yet. > The context switch comes only with your patches, since you are > introducing a second competing framework for the same task, without even > giving a clear path of how to integrate or convert the existing tests so > we could get back to a unified world. > You're absolutely right, and it's quite obvious that there's no one writing "acceptance tests" for the block layer yet. There's a subtle but important difference here though: this initiative is trying to allow people to write tests generic enough, for various QEMU subsystems (that's why maybe it's badly named as "acceptance"). It's really about trying to avoid context switches that may occur when developers and maintainers from those various subsystems (hopefully) start to write and review tests. So no, this is not an attempt to cause disruption, fragmentation and separate worlds. It's quite the contrary. And please excuse me from not writing a "how to migrate qemu-iotests" -- I don't even want to think about that if the block layer maintainers do not see any value in that. If you believe that handing off some of the infrastructure problems that qemu-iotests have to a common tool, and that it may be a good idea to have qemu-iotests become more like "qemu tests that happen to exercise the block layer", then we can push such an initiative forward. > I really don't think fragmenting the test infrastructure is a good idea, > especially for rather superficial advantages. > >> * The various utility APIs available in both the Test class and on >> avocado.utils > > Can you give examples? > I believe ended up giving some examples before, but for ease reading, let me duplicate them here: * avocado.Test: "Having access to local or remote files (images?), preparing the environment (creating those images?), accepting parameters from the user (which image format to use?) are all examples of test infrastructure problems applied to the block tests." * avocado.utils: "Thinking of hypothetical situations, the Avocado libraries contain utilities for logical volumes[1], so it'd naturally be easier to write a test for qemu-img + LVs." > Are those utility APIs actually worth losing the existing iotests.py > functions that provide stuff that is pretty specific to the QEMU and the > block layer? > There's no reason to lose iotests.py. Even the current acceptance tests are based on the principle of reusing the code that a lot of the iotests use (scripts/qemu.py and scripts/qmp/*). What I'm aiming for is that QEMU developers can write *tests*, and have a simple (and hopefully a common) way of running them. >> BTW, since most tests Today exist outside of "tests/acceptance", that >> may be also be solved in a great part by adding support in the (Avocado) >> test runner about some metadata in tests such qemu-iotests. > > Sure, feel free to wrap qemu-iotests as much as you want. Trivial > wrappers don't sound like a big maintenance burden. > > I just think that the block layer tests themselves should still keep > living in a single place. The obvious one is qemu-iotests. If you want > to change that, your cover letter shouldn't be quite as terse. The least > I would expect is an elaborate answer to: > > 1. Why is a change to something completely new useful and worth the > effort? We don't generally rewrite QEMU or the kernel if some parts > of it are ugly. We incrementally improve it instead. Exceptions need > good justification because rewrites come with costs, especially if > they can't offer feature parity. > I'll tell you a "shocking secret" (please take that with a grain of salt): I have no use case myself for any of the QEMU tests. I don't rely on them. No QEMU test make a difference in my work. That's why I may miss some of the obvious points, but at the same time, maybe I can look at things from a different perspective. Because incrementally improving the *overall* testing of QEMU is indeed one of my goals, this RFC was a "provocation" for change. Had I written a Python script with a very similar content, named it "233", we'd have missed this discussion. > 2. How do we migrate the existing tests to the new infrastructure to > avoid fragmentation? > Again, I haven't even thought of proposing that. This is so dependent on so many other aspects, including this initial feeling and feedback from the maintainers. > Kevin > Thanks a lot for the feedback so far, - Cleber. [1] https://avocado-framework.readthedocs.io/en/65.0/api/utils/avocado.utils.html#module-avocado.utils.lv_utils
Drive-by comment... Cleber Rosa <crosa@redhat.com> writes: [...] > My impression is that the "infrastructure for block tests" is not that > different from the infrastructure needed by other tests, specially other > QEMU tests. [...] Yes. The actual reason for having a completely separate testing infrastructure for block tests is that it predates testing infrastructure for anything else. Moving the tests to common infrastructure would be a sizable one-time effort we can ill afford. Maintaining multiple testing infrastructures is an ongoing effort we can also ill afford. Perhaps moving to common infrastructure is impractical. I don't know. What I do know is beware of temporal discounting. [...]
Am 12.11.2018 um 18:36 hat Cleber Rosa geschrieben: > I hope you don't blame me for trying to have the advantage of the > counter answer. :) Thanks for being so honest, but do you actually need this advantage when you have good technical arguments in favour of your proposal? > >> And run all tests related to snapshot. This is one of the reasons for > >> maybe allowing the type of test proposed here to live under > >> "tests/acceptance". Others include: > >> > >> * No numbering conflicts when naming tests > >> * More descriptive tests names and metadata > > > > Test numbering and metadata - sure, we can change that in qemu-iotests. > > Should be a lot easier than adding a whole new second infrastructure for > > block tests. > > > > My impression is that the "infrastructure for block tests" is not that > different from the infrastructure needed by other tests, specially other > QEMU tests. The point I'm trying to make here is that, adding a feature > such as metadata parsing/selection to tests looks much more like a *test > infrastructure* issue than a "block test" issue, right? It depends on what you include in "infrastructure". qemu-iotests has some functionality that is probably common to all test frameworks. But it also has some functionality that is very specific to QEMU and the block layer. As a rough guideline, anything that ./check does outside the actual test case is probably mostly generic; anything that is included in the test case files (such as most common.* files and iotests.py) are domain specific. So yes, there are parts of qemu-iotests that could be covered by another tool, and probably better, but there are also parts worth of keeping. > >> * No "context switch" for people also writing acceptance tests > > > > There are no people writing "acceptance tests" for the block layer yet. > > The context switch comes only with your patches, since you are > > introducing a second competing framework for the same task, without even > > giving a clear path of how to integrate or convert the existing tests so > > we could get back to a unified world. > > > > You're absolutely right, and it's quite obvious that there's no one > writing "acceptance tests" for the block layer yet. There's a subtle > but important difference here though: this initiative is trying to allow > people to write tests generic enough, for various QEMU subsystems > (that's why maybe it's badly named as "acceptance"). It's really about > trying to avoid context switches that may occur when developers and > maintainers from those various subsystems (hopefully) start to write and > review tests. > > So no, this is not an attempt to cause disruption, fragmentation and > separate worlds. It's quite the contrary. And please excuse me from > not writing a "how to migrate qemu-iotests" -- I don't even want to > think about that if the block layer maintainers do not see any value in > that. [ Insert that "how standards proliferate" xkcd here ] I see value in having a unified test infrastructure. However, I don't think committing a new Hello World grade qemu-img test case using different tooling than the rest of the qemu-img test cases is contributing much to this goal. We have so many unfinished conversions inside QEMU, and this looks like the approach you would take to add another one. > If you believe that handing off some of the infrastructure problems that > qemu-iotests have to a common tool, and that it may be a good idea to > have qemu-iotests become more like "qemu tests that happen to exercise > the block layer", then we can push such an initiative forward. Fine with me. But let's push it forward in a different way then. My primary concern is not being able to write new tests in a new environment. Consistency between block tests is more important to me than consistency of new block tests with tests for other parts of QEMU. So my primary concern is how to make the existing roughly 232 test cases in qemu-iotests compatible with the way the other parts use. And in fact, as those other parts don't exist yet (other than simple examples), maybe it would actually be worth discussing whether they shouldn't use the same structure and tooling as qemu-iotests (which can obviously be improved at the same time, but my point is about being in sync) instead of setting a new standard without considering the existing test cases and then trying to move qemu-iotests to it. > > Are those utility APIs actually worth losing the existing iotests.py > > functions that provide stuff that is pretty specific to the QEMU and the > > block layer? > > There's no reason to lose iotests.py. Even the current acceptance tests > are based on the principle of reusing the code that a lot of the iotests > use (scripts/qemu.py and scripts/qmp/*). > > What I'm aiming for is that QEMU developers can write *tests*, and have > a simple (and hopefully a common) way of running them. We already do write tests. It's not like you're starting from zero, even if your approach is as if you did. Anyway, one specific concern about the "simple way" I have is that we're adding a hard dependency on an external package (Avocado) that isn't usually installed anyway on developer machines. Maintainers will of course just install it. But will this reduce the amount of tests that contributors run, and increase the amount of untested patches on the mailing list? Maybe we can keep a simple in-tree runner like ./check that doesn't have any external dependencies and runs all of those tests that don't make use of Avocado utility functions etc.? And you'd use Avocado when you want to run all tests or use advanced test harness options. > > 1. Why is a change to something completely new useful and worth the > > effort? We don't generally rewrite QEMU or the kernel if some parts > > of it are ugly. We incrementally improve it instead. Exceptions need > > good justification because rewrites come with costs, especially if > > they can't offer feature parity. > > > > I'll tell you a "shocking secret" (please take that with a grain of > salt): I have no use case myself for any of the QEMU tests. I don't > rely on them. No QEMU test make a difference in my work. That's why I > may miss some of the obvious points, but at the same time, maybe I can > look at things from a different perspective. > > Because incrementally improving the *overall* testing of QEMU is indeed > one of my goals, this RFC was a "provocation" for change. Had I written > a Python script with a very similar content, named it "233", we'd have > missed this discussion. I would still have had input for you, which I will now put here, because I want to make sure that way we would write the test case will actually be well supported (having to care about this would be avoided if we started from "use the qemu-iotests test harness for everything" and then incrementally improved it to make it suit better for other cases and to make it usable from Avocado - in fact, the existing acceptance tests could trivially fit in there without any necessary changes to the harness). So for a long time, the only Python test cases we had looked much like your test case: Just run some code with lots of asserts in it, and at the end print something like "42 test cases passed"; this or an exception stack trace is the only output you get. Turns out that this is really hard to debug when a test case starts to fail. Our shell based test cases, on the other hand, have always printed the output and then we diffed it against a reference output. When it fails, you usually can see immediately what happens. The lesson we learned is that most of our newer Python test cases are much more verbose and make more use of diffing aginst a reference output instead of asserting. This is what your qemu-img test should probably do as well. And in fact, if it doesn't involve interacting with QMP in complex ways that actually need some programming to deal with the return values of the monitor, we wouldn't even write it in Python, but just in bash. > > 2. How do we migrate the existing tests to the new infrastructure to > > avoid fragmentation? > > Again, I haven't even thought of proposing that. This is so dependent > on so many other aspects, including this initial feeling and feedback > from the maintainers. But this is the most important point if we don't want to cause fragmentation of the tests! The answer to it decides whether doing any of this makes sense. Kevin
On Tue, Nov 13, 2018 at 01:18:36PM +0100, Kevin Wolf wrote: [...] > Anyway, one specific concern about the "simple way" I have is that we're > adding a hard dependency on an external package (Avocado) that isn't > usually installed anyway on developer machines. Maintainers will of > course just install it. But will this reduce the amount of tests that > contributors run, and increase the amount of untested patches on the > mailing list? > > Maybe we can keep a simple in-tree runner like ./check that doesn't have > any external dependencies and runs all of those tests that don't make > use of Avocado utility functions etc.? And you'd use Avocado when you > want to run all tests or use advanced test harness options. What problems you are trying to address here, exactly? If you don't have Avocado installed in your system, all you need is Python 3, an internet connection, and the ability to type "make check-acceptance" on your keyboard.
On Tue, Nov 13, 2018 at 10:39:57AM +0100, Markus Armbruster wrote: > Drive-by comment... > > Cleber Rosa <crosa@redhat.com> writes: > > [...] > > My impression is that the "infrastructure for block tests" is not that > > different from the infrastructure needed by other tests, specially other > > QEMU tests. > [...] > > Yes. The actual reason for having a completely separate testing > infrastructure for block tests is that it predates testing > infrastructure for anything else. > > Moving the tests to common infrastructure would be a sizable one-time > effort we can ill afford. Maintaining multiple testing infrastructures > is an ongoing effort we can also ill afford. If we do want to move to a common infrastructure, then IMHO this patch series should do the work to convert qemu-iotests to use it. Currently this is simply introducing a 2nd way to write block tests. We know from bitter experiance that when we introduce a new system in QEMU without converting the old system, the old system will live forever increasing our maint burden & creatnig confusion for developers about which is the preferred approach. Regards, Daniel
Am 13.11.2018 um 14:26 hat Eduardo Habkost geschrieben: > On Tue, Nov 13, 2018 at 01:18:36PM +0100, Kevin Wolf wrote: > [...] > > Anyway, one specific concern about the "simple way" I have is that we're > > adding a hard dependency on an external package (Avocado) that isn't > > usually installed anyway on developer machines. Maintainers will of > > course just install it. But will this reduce the amount of tests that > > contributors run, and increase the amount of untested patches on the > > mailing list? > > > > Maybe we can keep a simple in-tree runner like ./check that doesn't have > > any external dependencies and runs all of those tests that don't make > > use of Avocado utility functions etc.? And you'd use Avocado when you > > want to run all tests or use advanced test harness options. > > What problems you are trying to address here, exactly? > > If you don't have Avocado installed in your system, all you need > is Python 3, an internet connection, and the ability to type > "make check-acceptance" on your keyboard. Thanks, didn't know that one. Apparently you don't only need to have Python 3 available on the system, but also explicitly use it for ./configure? $ LANG=C make check-acceptance /home/kwolf/source/qemu/tests/Makefile.include:930: *** "venv directory for tests requires Python 3". Stop. While this doesn't make the tests available automatically for everyone, we'll get there when we finally make Python 3 the default (hopefully soon), which is already a lot better than what docs/devel/testing.rst promises: These tests are written using the Avocado Testing Framework (which must be installed separately) [...] Maybe time to update the docs to match the improved situation? :-) Kevin
On Tue, Nov 13, 2018 at 02:51:16PM +0100, Kevin Wolf wrote: > Am 13.11.2018 um 14:26 hat Eduardo Habkost geschrieben: > > On Tue, Nov 13, 2018 at 01:18:36PM +0100, Kevin Wolf wrote: > > [...] > > > Anyway, one specific concern about the "simple way" I have is that we're > > > adding a hard dependency on an external package (Avocado) that isn't > > > usually installed anyway on developer machines. Maintainers will of > > > course just install it. But will this reduce the amount of tests that > > > contributors run, and increase the amount of untested patches on the > > > mailing list? > > > > > > Maybe we can keep a simple in-tree runner like ./check that doesn't have > > > any external dependencies and runs all of those tests that don't make > > > use of Avocado utility functions etc.? And you'd use Avocado when you > > > want to run all tests or use advanced test harness options. > > > > What problems you are trying to address here, exactly? > > > > If you don't have Avocado installed in your system, all you need > > is Python 3, an internet connection, and the ability to type > > "make check-acceptance" on your keyboard. > > Thanks, didn't know that one. Apparently you don't only need to have > Python 3 available on the system, but also explicitly use it for > ./configure? > > $ LANG=C make check-acceptance > /home/kwolf/source/qemu/tests/Makefile.include:930: *** "venv directory for tests requires Python 3". Stop. I suggested in another thread that we should simply use "python3" instead of $(PYTHON) here, to solve that problem. I think we can use you reply as evidence that this is really the right thing to do. :) > > While this doesn't make the tests available automatically for everyone, > we'll get there when we finally make Python 3 the default (hopefully > soon), which is already a lot better than what docs/devel/testing.rst > promises: > > These tests are written using the Avocado Testing Framework (which > must be installed separately) [...] > > Maybe time to update the docs to match the improved situation? :-) Absolutely. Thanks for the feedback!
On 11/13/18 7:18 AM, Kevin Wolf wrote: > Am 12.11.2018 um 18:36 hat Cleber Rosa geschrieben: >> I hope you don't blame me for trying to have the advantage of the >> counter answer. :) > > Thanks for being so honest, but do you actually need this advantage when > you have good technical arguments in favour of your proposal? > The smile should have given away that I was joking. An advantage point over others is not needed when those are hoping to work together, and not trying to win an argument. >>>> And run all tests related to snapshot. This is one of the reasons for >>>> maybe allowing the type of test proposed here to live under >>>> "tests/acceptance". Others include: >>>> >>>> * No numbering conflicts when naming tests >>>> * More descriptive tests names and metadata >>> >>> Test numbering and metadata - sure, we can change that in qemu-iotests. >>> Should be a lot easier than adding a whole new second infrastructure for >>> block tests. >>> >> >> My impression is that the "infrastructure for block tests" is not that >> different from the infrastructure needed by other tests, specially other >> QEMU tests. The point I'm trying to make here is that, adding a feature >> such as metadata parsing/selection to tests looks much more like a *test >> infrastructure* issue than a "block test" issue, right? > > It depends on what you include in "infrastructure". qemu-iotests > has some functionality that is probably common to all test frameworks. > But it also has some functionality that is very specific to QEMU and the > block layer. > > As a rough guideline, anything that ./check does outside the actual test > case is probably mostly generic; anything that is included in the test > case files (such as most common.* files and iotests.py) are domain > specific. > > So yes, there are parts of qemu-iotests that could be covered by another > tool, and probably better, but there are also parts worth of keeping. > Agreed. >>>> * No "context switch" for people also writing acceptance tests >>> >>> There are no people writing "acceptance tests" for the block layer yet. >>> The context switch comes only with your patches, since you are >>> introducing a second competing framework for the same task, without even >>> giving a clear path of how to integrate or convert the existing tests so >>> we could get back to a unified world. >>> >> >> You're absolutely right, and it's quite obvious that there's no one >> writing "acceptance tests" for the block layer yet. There's a subtle >> but important difference here though: this initiative is trying to allow >> people to write tests generic enough, for various QEMU subsystems >> (that's why maybe it's badly named as "acceptance"). It's really about >> trying to avoid context switches that may occur when developers and >> maintainers from those various subsystems (hopefully) start to write and >> review tests. >> >> So no, this is not an attempt to cause disruption, fragmentation and >> separate worlds. It's quite the contrary. And please excuse me from >> not writing a "how to migrate qemu-iotests" -- I don't even want to >> think about that if the block layer maintainers do not see any value in >> that. > > [ Insert that "how standards proliferate" xkcd here ] > > I see value in having a unified test infrastructure. However, I don't > think committing a new Hello World grade qemu-img test case using > different tooling than the rest of the qemu-img test cases is > contributing much to this goal. > There are a few ways I take this response. First one is that my attempt to make this *RFC* an attempt to discuss and eventually collaborate, is more a miss than a hit. Second, it makes me wonder whether you'd prefer to take a patch without any test, or a test using a "different tooling" (I'm considering you do take patches without tests, if you don't, then please disregard this point). > We have so many unfinished conversions inside QEMU, and this looks like > the approach you would take to add another one. > Wrong, there's no conversion attempted here. >> If you believe that handing off some of the infrastructure problems that >> qemu-iotests have to a common tool, and that it may be a good idea to >> have qemu-iotests become more like "qemu tests that happen to exercise >> the block layer", then we can push such an initiative forward. > > Fine with me. But let's push it forward in a different way then. > > My primary concern is not being able to write new tests in a new > environment. Consistency between block tests is more important to me > than consistency of new block tests with tests for other parts of QEMU. > So my primary concern is how to make the existing roughly 232 test cases > in qemu-iotests compatible with the way the other parts use. > OK. > And in fact, as those other parts don't exist yet (other than simple > examples), maybe it would actually be worth discussing whether they > shouldn't use the same structure and tooling as qemu-iotests (which can > obviously be improved at the same time, but my point is about being in Feel free to start the discussion on why they should, and how. I'm really receptive to discussing any ideas. > sync) instead of setting a new standard without considering the existing > test cases and then trying to move qemu-iotests to it. > OK, "new" having a bad connotation is quite common on your reply. I with I had succeeded at passing the idea that I valued qemu-iotests so much that I was excited about hearing your thoughts about this *possible* "new" thing. "Without considering" is an unfair statement when you get CC'd and when expected feedback is anticipated and valued. >>> Are those utility APIs actually worth losing the existing iotests.py >>> functions that provide stuff that is pretty specific to the QEMU and the >>> block layer? >> >> There's no reason to lose iotests.py. Even the current acceptance tests >> are based on the principle of reusing the code that a lot of the iotests >> use (scripts/qemu.py and scripts/qmp/*). >> >> What I'm aiming for is that QEMU developers can write *tests*, and have >> a simple (and hopefully a common) way of running them. > > We already do write tests. It's not like you're starting from zero, even > if your approach is as if you did. > I'm really disappointed that I have to explain this, because I'm either failing miserably at communicating, or there's some other worse issues going on here. Anyway, when I used *tests* without a qualifier, I'm really talking about lowering the specialization factors on the test infrastructure. The giveaway is in adjectives such as "simple" and "common". Please, pretty please, assume positive intentions: I never intended or claimed to be the pioneer of tests in QEMU. > Anyway, one specific concern about the "simple way" I have is that we're > adding a hard dependency on an external package (Avocado) that isn't > usually installed anyway on developer machines. Maintainers will of > course just install it. But will this reduce the amount of tests that > contributors run, and increase the amount of untested patches on the > mailing list? > We've opted for a "opt-in" approach. If you want to run tests that depend on this external package, you go ahead and pay the data costs associated with that download. We did pack all those steps in a single "make check-acceptance" command for everyone's convenience, though. > Maybe we can keep a simple in-tree runner like ./check that doesn't have > any external dependencies and runs all of those tests that don't make > use of Avocado utility functions etc.? And you'd use Avocado when you > want to run all tests or use advanced test harness options. > Sure, this is the current state of affairs. And yes, one way of integrating the execution of test from different "silos" is by adding support on the runner, as mentioned before. I hope to work on this some time soon. >>> 1. Why is a change to something completely new useful and worth the >>> effort? We don't generally rewrite QEMU or the kernel if some parts >>> of it are ugly. We incrementally improve it instead. Exceptions need >>> good justification because rewrites come with costs, especially if >>> they can't offer feature parity. >>> >> >> I'll tell you a "shocking secret" (please take that with a grain of >> salt): I have no use case myself for any of the QEMU tests. I don't >> rely on them. No QEMU test make a difference in my work. That's why I >> may miss some of the obvious points, but at the same time, maybe I can >> look at things from a different perspective. >> >> Because incrementally improving the *overall* testing of QEMU is indeed >> one of my goals, this RFC was a "provocation" for change. Had I written >> a Python script with a very similar content, named it "233", we'd have >> missed this discussion. > > I would still have had input for you, which I will now put here, because > I want to make sure that way we would write the test case will actually > be well supported (having to care about this would be avoided if we > started from "use the qemu-iotests test harness for everything" and then > incrementally improved it to make it suit better for other cases and to > make it usable from Avocado - in fact, the existing acceptance tests > could trivially fit in there without any necessary changes to the > harness). > Would you believe that most of the tests that a QE team runs (look at Avocado-VT tests) would require no changes to the "check" harness or no significant amount of supporting code? It's easy to take into account the existing few and simple acceptance tests, and disregard the infrastructure needed, but we do have a road map that was part of the original series cover letter. We do have WiP versions of tests that configure and boot various guests and interact with them. It's tricky to get all of them at once at the quality level for upstream, so what you see it's just the start, not the end goal. With that in mind, we consciously recognized that "./check" was not enough, and it made no sense to put all that supporting code in QEMU itself. > So for a long time, the only Python test cases we had looked much like > your test case: Just run some code with lots of asserts in it, and at > the end print something like "42 test cases passed"; this or an > exception stack trace is the only output you get. Turns out that this is > really hard to debug when a test case starts to fail. > > Our shell based test cases, on the other hand, have always printed the > output and then we diffed it against a reference output. When it fails, > you usually can see immediately what happens. The lesson we learned is > that most of our newer Python test cases are much more verbose and make > more use of diffing aginst a reference output instead of asserting. This > is what your qemu-img test should probably do as well. > > And in fact, if it doesn't involve interacting with QMP in complex ways > that actually need some programming to deal with the return values of > the monitor, we wouldn't even write it in Python, but just in bash. > Right, you'd review this new qemu-iotest, but the overall discussion about an eventual larger collaboration would be missed. >>> 2. How do we migrate the existing tests to the new infrastructure to >>> avoid fragmentation? >> >> Again, I haven't even thought of proposing that. This is so dependent >> on so many other aspects, including this initial feeling and feedback >> from the maintainers. > > But this is the most important point if we don't want to cause > fragmentation of the tests! > > The answer to it decides whether doing any of this makes sense. > > Kevin > Sorry but I can't commit at this point to writing such a proposal. Maybe at a later time, priorities and common interests will better align and we can revisit this topic. I appreciate your time and feedback here though. Thanks, - Cleber.
On 11/13/18 8:51 AM, Kevin Wolf wrote: > Am 13.11.2018 um 14:26 hat Eduardo Habkost geschrieben: >> On Tue, Nov 13, 2018 at 01:18:36PM +0100, Kevin Wolf wrote: >> [...] >>> Anyway, one specific concern about the "simple way" I have is that we're >>> adding a hard dependency on an external package (Avocado) that isn't >>> usually installed anyway on developer machines. Maintainers will of >>> course just install it. But will this reduce the amount of tests that >>> contributors run, and increase the amount of untested patches on the >>> mailing list? >>> >>> Maybe we can keep a simple in-tree runner like ./check that doesn't have >>> any external dependencies and runs all of those tests that don't make >>> use of Avocado utility functions etc.? And you'd use Avocado when you >>> want to run all tests or use advanced test harness options. >> >> What problems you are trying to address here, exactly? >> >> If you don't have Avocado installed in your system, all you need >> is Python 3, an internet connection, and the ability to type >> "make check-acceptance" on your keyboard. > > Thanks, didn't know that one. Apparently you don't only need to have > Python 3 available on the system, but also explicitly use it for > ./configure? > > $ LANG=C make check-acceptance > /home/kwolf/source/qemu/tests/Makefile.include:930: *** "venv directory for tests requires Python 3". Stop. > > While this doesn't make the tests available automatically for everyone, > we'll get there when we finally make Python 3 the default (hopefully > soon), which is already a lot better than what docs/devel/testing.rst > promises: > > These tests are written using the Avocado Testing Framework (which > must be installed separately) [...] > > Maybe time to update the docs to match the improved situation? :-) > > Kevin > The docs were updated when "make check-acceptance" was used. That sentence is still true, Avocado must be installed separately, it's just that "check-venv", used by "check-acceptance" does just that. It's documented as: Running tests ------------- You can run the acceptance tests simply by executing: .. code:: make check-acceptance - Cleber.
On Tue, Nov 13, 2018 at 09:20:11AM -0500, Cleber Rosa wrote: > > > On 11/13/18 8:51 AM, Kevin Wolf wrote: > > Am 13.11.2018 um 14:26 hat Eduardo Habkost geschrieben: > >> On Tue, Nov 13, 2018 at 01:18:36PM +0100, Kevin Wolf wrote: > >> [...] > >>> Anyway, one specific concern about the "simple way" I have is that we're > >>> adding a hard dependency on an external package (Avocado) that isn't > >>> usually installed anyway on developer machines. Maintainers will of > >>> course just install it. But will this reduce the amount of tests that > >>> contributors run, and increase the amount of untested patches on the > >>> mailing list? > >>> > >>> Maybe we can keep a simple in-tree runner like ./check that doesn't have > >>> any external dependencies and runs all of those tests that don't make > >>> use of Avocado utility functions etc.? And you'd use Avocado when you > >>> want to run all tests or use advanced test harness options. > >> > >> What problems you are trying to address here, exactly? > >> > >> If you don't have Avocado installed in your system, all you need > >> is Python 3, an internet connection, and the ability to type > >> "make check-acceptance" on your keyboard. > > > > Thanks, didn't know that one. Apparently you don't only need to have > > Python 3 available on the system, but also explicitly use it for > > ./configure? > > > > $ LANG=C make check-acceptance > > /home/kwolf/source/qemu/tests/Makefile.include:930: *** "venv directory for tests requires Python 3". Stop. > > > > While this doesn't make the tests available automatically for everyone, > > we'll get there when we finally make Python 3 the default (hopefully > > soon), which is already a lot better than what docs/devel/testing.rst > > promises: > > > > These tests are written using the Avocado Testing Framework (which > > must be installed separately) [...] > > > > Maybe time to update the docs to match the improved situation? :-) > > > > Kevin > > > > The docs were updated when "make check-acceptance" was used. That > sentence is still true, Avocado must be installed separately, it's just > that "check-venv", used by "check-acceptance" does just that. With check-venv, we made "installing avocado" a small implementation detail that people don't need to care about when running the tests. I believe the sentence "which must be installed separately" is now confusing and misleading. It can be easily interpreted as "must be installed manually by the user", which is not case. > [...]
On 11/13/18 8:50 AM, Daniel P. Berrangé wrote: > On Tue, Nov 13, 2018 at 10:39:57AM +0100, Markus Armbruster wrote: >> Drive-by comment... >> >> Cleber Rosa <crosa@redhat.com> writes: >> >> [...] >>> My impression is that the "infrastructure for block tests" is not that >>> different from the infrastructure needed by other tests, specially other >>> QEMU tests. >> [...] >> >> Yes. The actual reason for having a completely separate testing >> infrastructure for block tests is that it predates testing >> infrastructure for anything else. >> >> Moving the tests to common infrastructure would be a sizable one-time >> effort we can ill afford. Maintaining multiple testing infrastructures >> is an ongoing effort we can also ill afford. > > If we do want to move to a common infrastructure, then IMHO this patch > series should do the work to convert qemu-iotests to use it. > > Currently this is simply introducing a 2nd way to write block tests. > > We know from bitter experiance that when we introduce a new system in > QEMU without converting the old system, the old system will live forever > increasing our maint burden & creatnig confusion for developers about > which is the preferred approach. > > > Regards, > Daniel > Right, I don't think there's enough momentum or interest in doing that right now. As myself and Kevin suggested, the best solution at this point may be wrapping the qemu-iotests without touching them. Regards, - Cleber.
On 11/13/18 9:32 AM, Eduardo Habkost wrote: > On Tue, Nov 13, 2018 at 09:20:11AM -0500, Cleber Rosa wrote: > > With check-venv, we made "installing avocado" a small > implementation detail that people don't need to care about when > running the tests. > > I believe the sentence "which must be installed separately" is > now confusing and misleading. It can be easily interpreted as > "must be installed manually by the user", which is not case. > Yeah, it can be confusing. Maybe something like: These tests are written using the Avocado Testing Framework (which must either be manually installed, or automatically as part of ``make check-acceptance``)... - Cleber.
On Tue, Nov 13, 2018 at 09:43:49AM -0500, Cleber Rosa wrote: > > On 11/13/18 9:32 AM, Eduardo Habkost wrote: > > On Tue, Nov 13, 2018 at 09:20:11AM -0500, Cleber Rosa wrote: > > > > With check-venv, we made "installing avocado" a small > > implementation detail that people don't need to care about when > > running the tests. > > > > I believe the sentence "which must be installed separately" is > > now confusing and misleading. It can be easily interpreted as > > "must be installed manually by the user", which is not case. > > > > Yeah, it can be confusing. Maybe something like: > > These tests are written using the Avocado Testing Framework (which must > either be manually installed, or automatically as part of ``make > check-acceptance``)... If it doesn't require any action from the user, why bother mentioning it at the beginning of the document? Avocado installation can be a small footnote below the "running tests" section, for people interested in implementation details.
Hi Cleber, I will shorten this email a lot while replying because I have the impression that most of the discussion isn't actually as productive as it could be. I'm not trying to evade on any point that I'm cutting out, so if there is something specific in the part I'm removing that you would like to get answer for, please let me know and I'll follow up. Am 13.11.2018 um 15:15 hat Cleber Rosa geschrieben: > >> So no, this is not an attempt to cause disruption, fragmentation and > >> separate worlds. It's quite the contrary. And please excuse me from > >> not writing a "how to migrate qemu-iotests" -- I don't even want to > >> think about that if the block layer maintainers do not see any value in > >> that. > > > > [ Insert that "how standards proliferate" xkcd here ] > > > > I see value in having a unified test infrastructure. However, I don't > > think committing a new Hello World grade qemu-img test case using > > different tooling than the rest of the qemu-img test cases is > > contributing much to this goal. > > There are a few ways I take this response. First one is that my attempt > to make this *RFC* an attempt to discuss and eventually collaborate, is > more a miss than a hit. Second, it makes me wonder whether you'd prefer > to take a patch without any test, or a test using a "different tooling" > (I'm considering you do take patches without tests, if you don't, then > please disregard this point). As for the actual patch series at hand, if you want it to get into 3.1, the best way would be to split code change and test into two different patches and make a traditional qemu-iotests case out of it. But I think that wasn't the main point of the exercise, right? For the broader topic of testing and the block layer, I believe the following paragraph shows a fundamental misunderstanding. Maybe I'm wrong, but that's my impression. This one: > > We have so many unfinished conversions inside QEMU, and this looks > > like the approach you would take to add another one. > > Wrong, there's no conversion attempted here. I get the impression that you think not attempting any conversion is something that should make it easier for me to agree with the approach. The opposite is true: Introducing a second way to do things and not converting existing test cases to it so that the first way could go away is the fragmentation that I really want to avoid. I don't mind too much which way we use, but I do care a lot that it's a single way. So when I said "let's push it forward in a different way", what I had in mind was starting with the existing qemu-iotests: Currently, if you run avocado in tests/qemu-iotests/, it doesn't do anything. I imagine it shouldn't be very hard to make it run at least the Python-based unit test style cases, which look very similar to what we have in tests/acceptance/. In the next step, we could do the Python-based tests that work with diffing a reference output. And then shell scripts (which need a very specific environment to run and might require the most changes). And finally, we would make sure that every option in ./check has a translation to the new style and start making use of the new features that Avocado brings in. There are questions to answer on the way that don't seem completely clear to me, but that's probably just because I don't know Avocado very well. For example, while tests like the VNC one run exactly once and don't require a prepared environment, ./check from qemu-iotests takes a parameter for the image format and protocol to use for the test case. This mechanism would have to be translated to Avocado somehow. I think if you (or someone else who know Avocado well) try to do this change, you'll figure out solutions quickly. This is where we need your help because the block layer people don't know how this stuff works (at least I don't). Or if you don't have the time to actually do the patches yourself, tell us how we should do it. I'm not sure I can find the time for this, but I can try. So if the most important part for you is not the qemu-img bench fix, but the approach to testing in general, I think this would be a much more useful way to attack it. > > And in fact, as those other parts don't exist yet (other than simple > > examples), maybe it would actually be worth discussing whether they > > shouldn't use the same structure and tooling as qemu-iotests (which can > > obviously be improved at the same time, but my point is about being in > > Feel free to start the discussion on why they should, and how. I'm > really receptive to discussing any ideas. Maybe it's actually too late for that, considering that you said that many test cases are already in development that use the newly introduced acceptance test way. But it would have essentially been: 1. move the qemu-iotests infrastructure up to tests/ 2. if you already want to add vnc tests, you can add them to that 3. do all of the conversion steps I outlined above to make sure that we don't lose features compared to the old infrastructure. 4. you get a generic infrastructure that looks much like what you're creating inside tests/acceptance, only that it's used for everything We can still do it out of order, but we have two parts to integrate now instead of just one that needs to be evolved. (By the way, why is tests/acceptance/ a separate directory? If we want it to be _the_ way to do tests, wouldn't it make more sense to have it on the top level and then use subdirectories only to categorise test cases?) > > sync) instead of setting a new standard without considering the existing > > test cases and then trying to move qemu-iotests to it. > > > > OK, "new" having a bad connotation is quite common on your reply. To me, "new" has a bad connotation as long as it implies that it coexists with "old" instead of replacing it. This is the part that I really want to fix, and then we can happily go ahead with "new" (and only "new"). Experience says that the easiest way to get there is by simply evolving "old" into "new", but there are other ways. Maybe the coexisting "new" is already developed far enough that we will have to use other ways this time. But then let's use those ways and not put up with "old" and "new" coexisting. > > Anyway, one specific concern about the "simple way" I have is that we're > > adding a hard dependency on an external package (Avocado) that isn't > > usually installed anyway on developer machines. Maintainers will of > > course just install it. But will this reduce the amount of tests that > > contributors run, and increase the amount of untested patches on the > > mailing list? > > > > We've opted for a "opt-in" approach. If you want to run tests that > depend on this external package, you go ahead and pay the data costs > associated with that download. We did pack all those steps in a single > "make check-acceptance" command for everyone's convenience, though. > > > Maybe we can keep a simple in-tree runner like ./check that doesn't have > > any external dependencies and runs all of those tests that don't make > > use of Avocado utility functions etc.? And you'd use Avocado when you > > want to run all tests or use advanced test harness options. > > > > Sure, this is the current state of affairs. And yes, one way of > integrating the execution of test from different "silos" is by adding > support on the runner, as mentioned before. I hope to work on this some > time soon. Actually, with Eduardo's hint that you don't need to manually install Avocado any more, but we automatically download everything that is needed, I consider this concern mostly addressed. We need to make sure that ./configure without any options allows to run it (currently you need to explicitly specify Python 3), but that's it. Then a separate in-tree runner isn't really needed either. > > So for a long time, the only Python test cases we had looked much like > > your test case: Just run some code with lots of asserts in it, and at > > the end print something like "42 test cases passed"; this or an > > exception stack trace is the only output you get. Turns out that this is > > really hard to debug when a test case starts to fail. > > > > Our shell based test cases, on the other hand, have always printed the > > output and then we diffed it against a reference output. When it fails, > > you usually can see immediately what happens. The lesson we learned is > > that most of our newer Python test cases are much more verbose and make > > more use of diffing aginst a reference output instead of asserting. This > > is what your qemu-img test should probably do as well. > > > > And in fact, if it doesn't involve interacting with QMP in complex ways > > that actually need some programming to deal with the return values of > > the monitor, we wouldn't even write it in Python, but just in bash. > > Right, you'd review this new qemu-iotest, but the overall discussion > about an eventual larger collaboration would be missed. This was not at all about the harness, but specific feedback how I would like a simple qemu-img test case to work. The implied message, if any, was that I'd like to see an example for how to implement a diff-based test (ideally in bash) in the Avocado-based test framework. At the same time, this would be the prototype for converting existing qemu-iotests cases. Kevin