[libvirt] [PATCH] test_driver: implement virDomainSendKey
Validate @keycodes and sleep for @holdtime before successfully returning. Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- src/test/test_driver.c | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 2f58a1da95..51ded256be 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -64,6 +64,7 @@ #include "virinterfaceobj.h" #include "virhostcpu.h" #include "virdomainsnapshotobjlist.h" +#include "virkeycode.h" #define VIR_FROM_THIS VIR_FROM_TEST @@ -5980,6 +5981,44 @@ testDomainScreenshot(virDomainPtr dom ATTRIBUTE_UNUSED, return ret; } +static int +testDomainSendKey(virDomainPtr domain, + unsigned int codeset, + unsigned int holdtime, + unsigned int *keycodes, + int nkeycodes, + unsigned int flags) +{ + int ret = -1; + size_t i; + virDomainObjPtr vm = NULL; + + virCheckFlags(0, -1); + + if (!(vm = testDomObjFromDomain(domain))) + goto cleanup; + + if (virDomainObjCheckActive(vm) < 0) + goto cleanup; + + for (i = 0; i < nkeycodes; i++) { + if (virKeycodeValueTranslate(codeset, codeset, keycodes[i]) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid keycode %u of %s codeset"), + keycodes[i], + virKeycodeSetTypeToString(codeset)); + goto cleanup; + } + } + + usleep(holdtime * 1000); + ret = 0; + + cleanup: + virDomainObjEndAPI(&vm); + return ret; +} + static int testConnectGetCPUModelNames(virConnectPtr conn ATTRIBUTE_UNUSED, const char *archName, @@ -7000,6 +7039,7 @@ static virHypervisorDriver testHypervisorDriver = { .nodeGetCPUMap = testNodeGetCPUMap, /* 1.0.0 */ .domainRename = testDomainRename, /* 4.1.0 */ .domainScreenshot = testDomainScreenshot, /* 1.0.5 */ + .domainSendKey = testDomainSendKey, /* 5.5.0 */ .domainGetMetadata = testDomainGetMetadata, /* 1.1.3 */ .domainSetMetadata = testDomainSetMetadata, /* 1.1.3 */ .connectGetCPUModelNames = testConnectGetCPUModelNames, /* 1.1.3 */ -- 2.21.0
On Sat, Jun 01, 2019 at 02:46:56PM +0200, Ilias Stamatis wrote:
Validate @keycodes and sleep for @holdtime before successfully returning.
Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- src/test/test_driver.c | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 2f58a1da95..51ded256be 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -64,6 +64,7 @@ #include "virinterfaceobj.h" #include "virhostcpu.h" #include "virdomainsnapshotobjlist.h" +#include "virkeycode.h"
#define VIR_FROM_THIS VIR_FROM_TEST
@@ -5980,6 +5981,44 @@ testDomainScreenshot(virDomainPtr dom ATTRIBUTE_UNUSED, return ret; }
+static int +testDomainSendKey(virDomainPtr domain, + unsigned int codeset, + unsigned int holdtime, + unsigned int *keycodes, + int nkeycodes, + unsigned int flags) +{ + int ret = -1; + size_t i; + virDomainObjPtr vm = NULL; + + virCheckFlags(0, -1); + + if (!(vm = testDomObjFromDomain(domain))) + goto cleanup; + + if (virDomainObjCheckActive(vm) < 0) + goto cleanup; + + for (i = 0; i < nkeycodes; i++) { + if (virKeycodeValueTranslate(codeset, codeset, keycodes[i]) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid keycode %u of %s codeset"), + keycodes[i], + virKeycodeSetTypeToString(codeset)); + goto cleanup; + } + } + + usleep(holdtime * 1000);
I think this API should be merely about translating the value, because this way you're just blocking a synchronous API for no apparent benefit. I believe it should return instantaneously. On the other hand, I'm just wondering what value it brings having an API translating keycodes, but I guess for consistency reasons, we can happily introduce it. Otherwise looking good. Erik
On Mon, Jun 03, 2019 at 08:23:57AM +0200, Erik Skultety wrote:
On Sat, Jun 01, 2019 at 02:46:56PM +0200, Ilias Stamatis wrote:
Validate @keycodes and sleep for @holdtime before successfully returning.
Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- src/test/test_driver.c | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 2f58a1da95..51ded256be 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -64,6 +64,7 @@ #include "virinterfaceobj.h" #include "virhostcpu.h" #include "virdomainsnapshotobjlist.h" +#include "virkeycode.h"
#define VIR_FROM_THIS VIR_FROM_TEST
@@ -5980,6 +5981,44 @@ testDomainScreenshot(virDomainPtr dom ATTRIBUTE_UNUSED, return ret; }
+static int +testDomainSendKey(virDomainPtr domain, + unsigned int codeset, + unsigned int holdtime, + unsigned int *keycodes, + int nkeycodes, + unsigned int flags) +{ + int ret = -1; + size_t i; + virDomainObjPtr vm = NULL; + + virCheckFlags(0, -1); + + if (!(vm = testDomObjFromDomain(domain))) + goto cleanup; + + if (virDomainObjCheckActive(vm) < 0) + goto cleanup; + + for (i = 0; i < nkeycodes; i++) { + if (virKeycodeValueTranslate(codeset, codeset, keycodes[i]) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid keycode %u of %s codeset"), + keycodes[i], + virKeycodeSetTypeToString(codeset)); + goto cleanup; + } + } + + usleep(holdtime * 1000);
I think this API should be merely about translating the value, because this way you're just blocking a synchronous API for no apparent benefit. I believe it should return instantaneously. On the other hand, I'm just wondering what value it brings having an API translating keycodes, but I guess for consistency reasons, we can happily introduce it.
The benefit is to simulate the holdtime as for example hyperv driver is doing, in QEMU we just pass everything to the QEMU itself where that one will probably do similar operation. The translation is just a way how to validate whether the keycode was correct or not as the code translates to the same codeset. Pavel
On Mon, Jun 03, 2019 at 08:40:13AM +0200, Pavel Hrdina wrote:
On Mon, Jun 03, 2019 at 08:23:57AM +0200, Erik Skultety wrote:
On Sat, Jun 01, 2019 at 02:46:56PM +0200, Ilias Stamatis wrote:
Validate @keycodes and sleep for @holdtime before successfully returning.
Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- src/test/test_driver.c | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 2f58a1da95..51ded256be 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -64,6 +64,7 @@ #include "virinterfaceobj.h" #include "virhostcpu.h" #include "virdomainsnapshotobjlist.h" +#include "virkeycode.h"
#define VIR_FROM_THIS VIR_FROM_TEST
@@ -5980,6 +5981,44 @@ testDomainScreenshot(virDomainPtr dom ATTRIBUTE_UNUSED, return ret; }
+static int +testDomainSendKey(virDomainPtr domain, + unsigned int codeset, + unsigned int holdtime, + unsigned int *keycodes, + int nkeycodes, + unsigned int flags) +{ + int ret = -1; + size_t i; + virDomainObjPtr vm = NULL; + + virCheckFlags(0, -1); + + if (!(vm = testDomObjFromDomain(domain))) + goto cleanup; + + if (virDomainObjCheckActive(vm) < 0) + goto cleanup; + + for (i = 0; i < nkeycodes; i++) { + if (virKeycodeValueTranslate(codeset, codeset, keycodes[i]) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid keycode %u of %s codeset"), + keycodes[i], + virKeycodeSetTypeToString(codeset)); + goto cleanup; + } + } + + usleep(holdtime * 1000);
I think this API should be merely about translating the value, because this way you're just blocking a synchronous API for no apparent benefit. I believe it should return instantaneously. On the other hand, I'm just wondering what value it brings having an API translating keycodes, but I guess for consistency reasons, we can happily introduce it.
The benefit is to simulate the holdtime as for example hyperv driver is doing, in QEMU we just pass everything to the QEMU itself where that one will probably do similar operation.
Both hyperv and vbox have to do it because they don't support it, so they do usleep before sending down and up keycodes respectively, QEMU does it iternally. In test driver, you're not testing any true codes, so doing plain usleep si IMHO wrong, from app dev's perspective you're only testing whether usleep works by providing arbitrary time to wait, which raises the obvious question "why". Erik
On Mon, Jun 03, 2019 at 08:52:55 +0200, Erik Skultety wrote:
On Mon, Jun 03, 2019 at 08:40:13AM +0200, Pavel Hrdina wrote:
On Mon, Jun 03, 2019 at 08:23:57AM +0200, Erik Skultety wrote:
On Sat, Jun 01, 2019 at 02:46:56PM +0200, Ilias Stamatis wrote:
[...]
I think this API should be merely about translating the value, because this way you're just blocking a synchronous API for no apparent benefit. I believe it should return instantaneously. On the other hand, I'm just wondering what value it brings having an API translating keycodes, but I guess for consistency reasons, we can happily introduce it.
The benefit is to simulate the holdtime as for example hyperv driver is doing, in QEMU we just pass everything to the QEMU itself where that one will probably do similar operation.
Both hyperv and vbox have to do it because they don't support it, so they do usleep before sending down and up keycodes respectively, QEMU does it iternally. In test driver, you're not testing any true codes, so doing plain usleep si IMHO wrong, from app dev's perspective you're only testing whether usleep works by providing arbitrary time to wait, which raises the obvious question "why".
I think the problem here is that it's unclear what the purpose of the test driver is supposed to be. Clarifying the purpose first might have positive impact on the design decisions here. So what's the point of the test driver? Is it meant for human interaction, e.g. for application developers or users wanting to test stuff? In that case you probably want to simulate the "worst" behaviour we have, which would be the blocking timeout so that users/devs can see the worst case implications of running such a command. Alternatively the purpose may be to allow some kind of "unit" testing for APPs using libvirt. (I specifically used "unit" testing, because any kind of sane real integration testing will not use the test driver because it would not accomplish much). In such case the timeout is a hassle because it just unnecessarily prolongs test runs and does not add much value. Any other ideas? I don't have any more. Frankly even the two use cases above aren't something I'd recommend to the users. In case of user/dev testing, checking against a real VM makes more sense and the same goes for real integration testing.
On Mon, Jun 03, 2019 at 12:31:56PM +0200, Peter Krempa wrote:
On Mon, Jun 03, 2019 at 08:52:55 +0200, Erik Skultety wrote:
On Mon, Jun 03, 2019 at 08:40:13AM +0200, Pavel Hrdina wrote:
On Mon, Jun 03, 2019 at 08:23:57AM +0200, Erik Skultety wrote:
On Sat, Jun 01, 2019 at 02:46:56PM +0200, Ilias Stamatis wrote:
[...]
I think this API should be merely about translating the value, because this way you're just blocking a synchronous API for no apparent benefit. I believe it should return instantaneously. On the other hand, I'm just wondering what value it brings having an API translating keycodes, but I guess for consistency reasons, we can happily introduce it.
The benefit is to simulate the holdtime as for example hyperv driver is doing, in QEMU we just pass everything to the QEMU itself where that one will probably do similar operation.
Both hyperv and vbox have to do it because they don't support it, so they do usleep before sending down and up keycodes respectively, QEMU does it iternally. In test driver, you're not testing any true codes, so doing plain usleep si IMHO wrong, from app dev's perspective you're only testing whether usleep works by providing arbitrary time to wait, which raises the obvious question "why".
I think the problem here is that it's unclear what the purpose of the test driver is supposed to be. Clarifying the purpose first might have positive impact on the design decisions here.
So what's the point of the test driver?
Is it meant for human interaction, e.g. for application developers or users wanting to test stuff? In that case you probably want to simulate the "worst" behaviour we have, which would be the blocking timeout so that users/devs can see the worst case implications of running such a command.
I agree and disagree at the same time. The part I disagree with is that anyone would be truly interested in seeing how much the API blocks, they'd just want to job done and as such the wait doesn't bring any value to that and I agree I'd also recommend doing anything related to ^this to be done on dummy VMs as you mentioned below, but then again, we're focusing on test driver coverage, so from coverage perspective, we probably want to cover this API too - to some degree. Thanks, Erik
Alternatively the purpose may be to allow some kind of "unit" testing for APPs using libvirt. (I specifically used "unit" testing, because any kind of sane real integration testing will not use the test driver because it would not accomplish much). In such case the timeout is a hassle because it just unnecessarily prolongs test runs and does not add much value.
Any other ideas? I don't have any more.
Frankly even the two use cases above aren't something I'd recommend to the users. In case of user/dev testing, checking against a real VM makes more sense and the same goes for real integration testing.
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Jun 03, 2019 at 01:13:14PM +0200, Erik Skultety wrote:
On Mon, Jun 03, 2019 at 12:31:56PM +0200, Peter Krempa wrote:
On Mon, Jun 03, 2019 at 08:52:55 +0200, Erik Skultety wrote:
On Mon, Jun 03, 2019 at 08:40:13AM +0200, Pavel Hrdina wrote:
On Mon, Jun 03, 2019 at 08:23:57AM +0200, Erik Skultety wrote:
On Sat, Jun 01, 2019 at 02:46:56PM +0200, Ilias Stamatis wrote:
[...]
I think this API should be merely about translating the value, because this way you're just blocking a synchronous API for no apparent benefit. I believe it should return instantaneously. On the other hand, I'm just wondering what value it brings having an API translating keycodes, but I guess for consistency reasons, we can happily introduce it.
The benefit is to simulate the holdtime as for example hyperv driver is doing, in QEMU we just pass everything to the QEMU itself where that one will probably do similar operation.
Both hyperv and vbox have to do it because they don't support it, so they do usleep before sending down and up keycodes respectively, QEMU does it iternally. In test driver, you're not testing any true codes, so doing plain usleep si IMHO wrong, from app dev's perspective you're only testing whether usleep works by providing arbitrary time to wait, which raises the obvious question "why".
I think the problem here is that it's unclear what the purpose of the test driver is supposed to be. Clarifying the purpose first might have positive impact on the design decisions here.
So what's the point of the test driver?
Is it meant for human interaction, e.g. for application developers or users wanting to test stuff? In that case you probably want to simulate the "worst" behaviour we have, which would be the blocking timeout so that users/devs can see the worst case implications of running such a command.
I agree and disagree at the same time. The part I disagree with is that anyone would be truly interested in seeing how much the API blocks, they'd just want to job done and as such the wait doesn't bring any value to that and I agree I'd also recommend doing anything related to ^this to be done on dummy VMs as you mentioned below, but then again, we're focusing on test driver coverage, so from coverage perspective, we probably want to cover this API too - to some degree.
Don't forget that while testing of management applications there is usually some UI involved and the timeout might be relevant to UI if it want's to somehow inform the user that the operation is still in progress so it might make sense for apps to set some holdtime if they want to test any specific UI message or similar and for the "unit" testing where nobody cares about the time they can pass 0 as holdtime. I don't see any problem with having the sleep there to "emulate" the holdtime behavior in order to give users of libvirt test driver the possibility to test it especially if you can simply pass 0 there. But as I've wrote if you are against it feel free to remove it. Pavel
On Mon, Jun 03, 2019 at 13:13:14 +0200, Erik Skultety wrote:
On Mon, Jun 03, 2019 at 12:31:56PM +0200, Peter Krempa wrote:
On Mon, Jun 03, 2019 at 08:52:55 +0200, Erik Skultety wrote:
[...]
I think the problem here is that it's unclear what the purpose of the test driver is supposed to be. Clarifying the purpose first might have positive impact on the design decisions here.
So what's the point of the test driver?
Is it meant for human interaction, e.g. for application developers or users wanting to test stuff? In that case you probably want to simulate the "worst" behaviour we have, which would be the blocking timeout so that users/devs can see the worst case implications of running such a command.
I agree and disagree at the same time. The part I disagree with is that anyone would be truly interested in seeing how much the API blocks, they'd just want
Well if you are designing an user interface around this it might be useful to know and experience that this API will block for a while in certain cases. Ideally you should be able to figure that out from the docs. There are also other APIs which can have interresting semantics in some cases. E.g. in case of the qemu driver all APIs which use the guest agent may block or fail if the guest agent is stuck or not installed. Obviously covering all the cases may be hard, thus we want to limit the scope ...
to job done and as such the wait doesn't bring any value to that and I agree I'd also recommend doing anything related to ^this to be done on dummy VMs as you mentioned below, but then again, we're focusing on test driver coverage, so
How something is covered really depends on the purpose and that's why I asked about the purpose. To me it's not clear what the test driver is supposed to achieve and thus it's hard to determine whether a given mock approach makes sense.
from coverage perspective, we probably want to cover this API too - to some degree.
Sure we want to cover it, but to which degree? That's what I want to clarify. If we do that, answering questions how to do things should be easier.
On Mon, Jun 03, 2019 at 01:48:55PM +0200, Peter Krempa wrote:
On Mon, Jun 03, 2019 at 13:13:14 +0200, Erik Skultety wrote:
On Mon, Jun 03, 2019 at 12:31:56PM +0200, Peter Krempa wrote:
On Mon, Jun 03, 2019 at 08:52:55 +0200, Erik Skultety wrote:
[...]
I think the problem here is that it's unclear what the purpose of the test driver is supposed to be. Clarifying the purpose first might have positive impact on the design decisions here.
So what's the point of the test driver?
Is it meant for human interaction, e.g. for application developers or users wanting to test stuff? In that case you probably want to simulate the "worst" behaviour we have, which would be the blocking timeout so that users/devs can see the worst case implications of running such a command.
I agree and disagree at the same time. The part I disagree with is that anyone would be truly interested in seeing how much the API blocks, they'd just want
Well if you are designing an user interface around this it might be useful to know and experience that this API will block for a while in certain cases.
Ideally you should be able to figure that out from the docs.
There are also other APIs which can have interresting semantics in some cases. E.g. in case of the qemu driver all APIs which use the guest agent may block or fail if the guest agent is stuck or not installed.
Obviously covering all the cases may be hard, thus we want to limit the scope ...
to job done and as such the wait doesn't bring any value to that and I agree I'd also recommend doing anything related to ^this to be done on dummy VMs as you mentioned below, but then again, we're focusing on test driver coverage, so
How something is covered really depends on the purpose and that's why I asked about the purpose. To me it's not clear what the test driver is supposed to achieve and thus it's hard to determine whether a given mock approach makes sense.
from coverage perspective, we probably want to cover this API too - to some degree.
Sure we want to cover it, but to which degree? That's what I want to clarify. If we do that, answering questions how to do things should be easier.
To answer your scope question this is what virt-manager is using test driver for: - It's used by CLI commands to validate our functionality whether we can generate correct XMLs and use the specific libvirt features, this is probably what you've called "unit" testing. - We have some basich UI testing in virt-manager that would ideally cover all UI elements and that one may be considered more like CI testing even though it uses test-driver as well - During development sometimes we use test driver to roughly check some feature before it will be tested using real driver with proper host and VM setup where that setup might not be easy or the specific HW required for some feature is not that easily available. In libvirt-dbus we use test driver as well to validate that APIs provided over DBus actually works but again it's more like the "unit" testing where we would not probably care about any specific blocking behavior. The idea of the whole project to implement test driver is to have full coverage which would ideally make the test driver implementation mandatory for everyone introducing new API. It probably doesn't answer what scope should be covered by test driver, I'm just writing down some thoughts and the usecase some projects have for test driver. Pavel
On Mon, Jun 03, 2019 at 14:16:15 +0200, Pavel Hrdina wrote:
On Mon, Jun 03, 2019 at 01:48:55PM +0200, Peter Krempa wrote:
[...]
Sure we want to cover it, but to which degree? That's what I want to clarify. If we do that, answering questions how to do things should be easier.
To answer your scope question this is what virt-manager is using test driver for:
- It's used by CLI commands to validate our functionality whether we can generate correct XMLs and use the specific libvirt features, this is probably what you've called "unit" testing.
This is an interresting point. Havin a way to validate XMLs seems useful. In this case we should e.g. make sure that the APIs taking XMLs do schema validation maybe even when not asked for via the API flag. For normal drivers this could create regressions but this way you force users to check their XMLs in the test driver, which should be okay in this case and would actually add some value.
- We have some basich UI testing in virt-manager that would ideally cover all UI elements and that one may be considered more like CI testing even though it uses test-driver as well
I'd be cautious calling it CI testing. The test driver is equivalent of the mock data we use in libvirt, which is uncoupled from reality at this point. It partially approaches CI territory in terms of XML validation (if used with the schema checker flag), but most other APIs are copypaste of code from the drivers which is not updated usually when doing changes in the driver itself.
- During development sometimes we use test driver to roughly check some feature before it will be tested using real driver with proper host and VM setup where that setup might not be easy or the specific HW required for some feature is not that easily available.
So this is a one-off effort mostly. And in the end you still need to check it against a real hypervisor anyways.
In libvirt-dbus we use test driver as well to validate that APIs provided over DBus actually works but again it's more like the "unit" testing where we would not probably care about any specific blocking behavior.
So in this case it's again mostly unit testing so that the libvirt-dbus project does not need to simulate the backend. The expected returns are mostly success as it in fact wants to test the transport and not the APIs themself. This scenario would make simulating the delay in the sendkey API mostly counterproductive as you are not very likely to hit threading problems with a fixed delay and in the end it will simulate a well known situation.
The idea of the whole project to implement test driver is to have full coverage which would ideally make the test driver implementation mandatory for everyone introducing new API.
Here we run again into the problem of how much time to spend on the test driver. Implementing some APIs might not be as fun as it seems. The most of the stats API is simple because you make up something based on some present data. The problems start with the complex API which change state. APIs for snapshots and block jobs e.g. have prety severe implications and basically would require copying most of the qemu code except for the monitor commands. Or the other option is to cheat and skip most of it and just return success or something. Also don't forget that having an API in the test driver is not the end of the story. If you look at the snapshot implementation you can see that external snapshots are not supported. Thus it does "something" but was never extended later on. So the 'mandatory' part might be hard to enforce.
It probably doesn't answer what scope should be covered by test driver, I'm just writing down some thoughts and the usecase some projects have for test driver.
Pavel
On Mon, Jun 03, 2019 at 03:51:29PM +0200, Peter Krempa wrote:
On Mon, Jun 03, 2019 at 14:16:15 +0200, Pavel Hrdina wrote:
On Mon, Jun 03, 2019 at 01:48:55PM +0200, Peter Krempa wrote:
[...]
Sure we want to cover it, but to which degree? That's what I want to clarify. If we do that, answering questions how to do things should be easier.
To answer your scope question this is what virt-manager is using test driver for:
- It's used by CLI commands to validate our functionality whether we can generate correct XMLs and use the specific libvirt features, this is probably what you've called "unit" testing.
This is an interresting point. Havin a way to validate XMLs seems useful.
In this case we should e.g. make sure that the APIs taking XMLs do schema validation maybe even when not asked for via the API flag.
For normal drivers this could create regressions but this way you force users to check their XMLs in the test driver, which should be okay in this case and would actually add some value.
- We have some basich UI testing in virt-manager that would ideally cover all UI elements and that one may be considered more like CI testing even though it uses test-driver as well
I'd be cautious calling it CI testing. The test driver is equivalent of the mock data we use in libvirt, which is uncoupled from reality at this point. It partially approaches CI territory in terms of XML validation (if used with the schema checker flag), but most other APIs are copypaste of code from the drivers which is not updated usually when doing changes in the driver itself.
- During development sometimes we use test driver to roughly check some feature before it will be tested using real driver with proper host and VM setup where that setup might not be easy or the specific HW required for some feature is not that easily available.
So this is a one-off effort mostly. And in the end you still need to check it against a real hypervisor anyways.
In libvirt-dbus we use test driver as well to validate that APIs provided over DBus actually works but again it's more like the "unit" testing where we would not probably care about any specific blocking behavior.
So in this case it's again mostly unit testing so that the libvirt-dbus project does not need to simulate the backend. The expected returns are mostly success as it in fact wants to test the transport and not the APIs themself.
This scenario would make simulating the delay in the sendkey API mostly counterproductive as you are not very likely to hit threading problems with a fixed delay and in the end it will simulate a well known situation.
I guess it's time to make a conclusion. There were arguments supporting either of the approaches, however (at least to me) leaning a bit more towards dropping the sleep as currently the usage is mostly in automated manner. Therefore, I'll proceed with dropping the delay. If you strongly feel like it indeed should be in place, feel free to post a follow-up patch. Erik
On Fri, Jun 7, 2019 at 12:25 PM Erik Skultety <eskultet@redhat.com> wrote:
On Mon, Jun 03, 2019 at 03:51:29PM +0200, Peter Krempa wrote:
On Mon, Jun 03, 2019 at 14:16:15 +0200, Pavel Hrdina wrote:
On Mon, Jun 03, 2019 at 01:48:55PM +0200, Peter Krempa wrote:
[...]
Sure we want to cover it, but to which degree? That's what I want to clarify. If we do that, answering questions how to do things should be easier.
To answer your scope question this is what virt-manager is using test driver for:
- It's used by CLI commands to validate our functionality whether we can generate correct XMLs and use the specific libvirt features, this is probably what you've called "unit" testing.
This is an interresting point. Havin a way to validate XMLs seems useful.
In this case we should e.g. make sure that the APIs taking XMLs do schema validation maybe even when not asked for via the API flag.
For normal drivers this could create regressions but this way you force users to check their XMLs in the test driver, which should be okay in this case and would actually add some value.
- We have some basich UI testing in virt-manager that would ideally cover all UI elements and that one may be considered more like CI testing even though it uses test-driver as well
I'd be cautious calling it CI testing. The test driver is equivalent of the mock data we use in libvirt, which is uncoupled from reality at this point. It partially approaches CI territory in terms of XML validation (if used with the schema checker flag), but most other APIs are copypaste of code from the drivers which is not updated usually when doing changes in the driver itself.
- During development sometimes we use test driver to roughly check some feature before it will be tested using real driver with proper host and VM setup where that setup might not be easy or the specific HW required for some feature is not that easily available.
So this is a one-off effort mostly. And in the end you still need to check it against a real hypervisor anyways.
In libvirt-dbus we use test driver as well to validate that APIs provided over DBus actually works but again it's more like the "unit" testing where we would not probably care about any specific blocking behavior.
So in this case it's again mostly unit testing so that the libvirt-dbus project does not need to simulate the backend. The expected returns are mostly success as it in fact wants to test the transport and not the APIs themself.
This scenario would make simulating the delay in the sendkey API mostly counterproductive as you are not very likely to hit threading problems with a fixed delay and in the end it will simulate a well known situation.
I guess it's time to make a conclusion. There were arguments supporting either of the approaches, however (at least to me) leaning a bit more towards dropping the sleep as currently the usage is mostly in automated manner. Therefore, I'll proceed with dropping the delay. If you strongly feel like it indeed should be in place, feel free to post a follow-up patch.
Erik
I'm also fine with dropping the sleep, and I was a bit skeptical about adding it in the first place. Ilias
On Mon, Jun 03, 2019 at 12:31:56PM +0200, Peter Krempa wrote:
On Mon, Jun 03, 2019 at 08:52:55 +0200, Erik Skultety wrote:
On Mon, Jun 03, 2019 at 08:40:13AM +0200, Pavel Hrdina wrote:
On Mon, Jun 03, 2019 at 08:23:57AM +0200, Erik Skultety wrote:
On Sat, Jun 01, 2019 at 02:46:56PM +0200, Ilias Stamatis wrote:
[...]
I think this API should be merely about translating the value, because this way you're just blocking a synchronous API for no apparent benefit. I believe it should return instantaneously. On the other hand, I'm just wondering what value it brings having an API translating keycodes, but I guess for consistency reasons, we can happily introduce it.
The benefit is to simulate the holdtime as for example hyperv driver is doing, in QEMU we just pass everything to the QEMU itself where that one will probably do similar operation.
Both hyperv and vbox have to do it because they don't support it, so they do usleep before sending down and up keycodes respectively, QEMU does it iternally. In test driver, you're not testing any true codes, so doing plain usleep si IMHO wrong, from app dev's perspective you're only testing whether usleep works by providing arbitrary time to wait, which raises the obvious question "why".
I think the problem here is that it's unclear what the purpose of the test driver is supposed to be. Clarifying the purpose first might have positive impact on the design decisions here.
So what's the point of the test driver?
Is it meant for human interaction, e.g. for application developers or users wanting to test stuff? In that case you probably want to simulate the "worst" behaviour we have, which would be the blocking timeout so that users/devs can see the worst case implications of running such a command.
Alternatively the purpose may be to allow some kind of "unit" testing for APPs using libvirt. (I specifically used "unit" testing, because any kind of sane real integration testing will not use the test driver because it would not accomplish much). In such case the timeout is a hassle because it just unnecessarily prolongs test runs and does not add much value.
Any other ideas? I don't have any more.
The "test" driver essentially exist to allow application developers to do automated functional testing of their applications. ie it should allow them to exercise any libvirt API calls they make and get some kind of interesting results back, without needing full VM actually running. It is not expected to perfectly replicate what you'd see when using the QEMU driver with a real VM in terms of performance / scalability, nor expose every possible error condition / behaviour. If that's what you are expecting, then you need full stack integration testing, not functional testing. Essentially think of the test driver as allowing app developers to do a basic evaluation to validate their app isn't stupidly broken in some way. The reason why you'd use the test driver for functional tests instead doing a full integration test is that it has no further external dependancies, either software or hardware. This makes it something you can always run at any time, any place. Full integration testing is better but you'll need a much more defined stack of softrware & hardware that can't be assumed to be always present. With this in mind I do *not* think it is needed to actually have a sleep() to match the "hold time". If you want that kind of accuracy of testing, do integration tests with a real VM running. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Jun 03, 2019 at 08:40:13AM +0200, Pavel Hrdina wrote:
On Mon, Jun 03, 2019 at 08:23:57AM +0200, Erik Skultety wrote:
On Sat, Jun 01, 2019 at 02:46:56PM +0200, Ilias Stamatis wrote:
+ for (i = 0; i < nkeycodes; i++) { + if (virKeycodeValueTranslate(codeset, codeset, keycodes[i]) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid keycode %u of %s codeset"), + keycodes[i], + virKeycodeSetTypeToString(codeset)); + goto cleanup; + } + } + + usleep(holdtime * 1000);
I think this API should be merely about translating the value, because this way you're just blocking a synchronous API for no apparent benefit. I believe it should return instantaneously. On the other hand, I'm just wondering what value it brings having an API translating keycodes, but I guess for consistency reasons, we can happily introduce it.
The benefit is to simulate the holdtime as for example hyperv driver is doing, in QEMU we just pass everything to the QEMU itself where that one will probably do similar operation.
But the QEMU API returns immediately. I don't see a reason to arbitrarily delay the return of the API if it has no functional purpose. Also, relying on measuring execution time to figure out whether the test works or not seems unreliable. Jano
The translation is just a way how to validate whether the keycode was correct or not as the code translates to the same codeset.
Pavel
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Jun 03, 2019 at 08:56:31AM +0200, Ján Tomko wrote:
On Mon, Jun 03, 2019 at 08:40:13AM +0200, Pavel Hrdina wrote:
On Mon, Jun 03, 2019 at 08:23:57AM +0200, Erik Skultety wrote:
On Sat, Jun 01, 2019 at 02:46:56PM +0200, Ilias Stamatis wrote:
+ for (i = 0; i < nkeycodes; i++) { + if (virKeycodeValueTranslate(codeset, codeset, keycodes[i]) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid keycode %u of %s codeset"), + keycodes[i], + virKeycodeSetTypeToString(codeset)); + goto cleanup; + } + } + + usleep(holdtime * 1000);
I think this API should be merely about translating the value, because this way you're just blocking a synchronous API for no apparent benefit. I believe it should return instantaneously. On the other hand, I'm just wondering what value it brings having an API translating keycodes, but I guess for consistency reasons, we can happily introduce it.
The benefit is to simulate the holdtime as for example hyperv driver is doing, in QEMU we just pass everything to the QEMU itself where that one will probably do similar operation.
But the QEMU API returns immediately. I don't see a reason to arbitrarily delay the return of the API if it has no functional purpose.
Also, relying on measuring execution time to figure out whether the test works or not seems unreliable.
If you are both strongly against that sure, I personally don't care. Pavel
On Sat, Jun 01, 2019 at 02:46:56PM +0200, Ilias Stamatis wrote:
Validate @keycodes and sleep for @holdtime before successfully returning.
Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> ---
Reviewed-by: Erik Skultety <eskultet@redhat.com>
src/test/test_driver.c | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 2f58a1da95..51ded256be 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -64,6 +64,7 @@ #include "virinterfaceobj.h" #include "virhostcpu.h" #include "virdomainsnapshotobjlist.h" +#include "virkeycode.h"
#define VIR_FROM_THIS VIR_FROM_TEST
@@ -5980,6 +5981,44 @@ testDomainScreenshot(virDomainPtr dom ATTRIBUTE_UNUSED, return ret; }
Some time ago we decided that we'd put 2 line in between function definitions, this is mostly to be somewhat consistent, but we're not enforcing it in any way and the test driver hasn't been following this, but since these are new APIs, I added the 2 lines there. But again, it's not something that would be normally worth mentioning during review, but you should know that I made that adjustment. Reviewed-by: Erik Skultety <eskultet@redhat.com>
+static int +testDomainSendKey(virDomainPtr domain, + unsigned int codeset, + unsigned int holdtime, + unsigned int *keycodes, + int nkeycodes, + unsigned int flags) +{ + int ret = -1; + size_t i; + virDomainObjPtr vm = NULL; + + virCheckFlags(0, -1); + + if (!(vm = testDomObjFromDomain(domain))) + goto cleanup; + + if (virDomainObjCheckActive(vm) < 0) + goto cleanup; + + for (i = 0; i < nkeycodes; i++) { + if (virKeycodeValueTranslate(codeset, codeset, keycodes[i]) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid keycode %u of %s codeset"), + keycodes[i], + virKeycodeSetTypeToString(codeset)); + goto cleanup; + } + } + + usleep(holdtime * 1000); + ret = 0; + + cleanup: + virDomainObjEndAPI(&vm); + return ret; +} + static int testConnectGetCPUModelNames(virConnectPtr conn ATTRIBUTE_UNUSED, const char *archName, @@ -7000,6 +7039,7 @@ static virHypervisorDriver testHypervisorDriver = { .nodeGetCPUMap = testNodeGetCPUMap, /* 1.0.0 */ .domainRename = testDomainRename, /* 4.1.0 */ .domainScreenshot = testDomainScreenshot, /* 1.0.5 */ + .domainSendKey = testDomainSendKey, /* 5.5.0 */ .domainGetMetadata = testDomainGetMetadata, /* 1.1.3 */ .domainSetMetadata = testDomainSetMetadata, /* 1.1.3 */ .connectGetCPUModelNames = testConnectGetCPUModelNames, /* 1.1.3 */ -- 2.21.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (6)
-
Daniel P. Berrangé -
Erik Skultety -
Ilias Stamatis -
Ján Tomko -
Pavel Hrdina -
Peter Krempa