7:46 a.m.
Validate @keycodes and sleep for @holdtime before successfully
returning.
Signed-off-by: Ilias Stamatis <stamatis.iliass(a)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
1:23 a.m.
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(a)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
1:40 a.m.
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(a)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
1:52 a.m.
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(a)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
5:31 a.m.
New subject: [libvirt] Scope of the test driver? (Was: Re: [PATCH] test_driver: implement virDomainSendKey)
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.
6:13 a.m.
New subject: [libvirt] Scope of the test driver? (Was: Re: [PATCH] test_driver: implement virDomainSendKey)
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
6:44 a.m.
New subject: [libvirt] Scope of the test driver? (Was: Re: [PATCH] test_driver: implement virDomainSendKey)
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
6:48 a.m.
New subject: [libvirt] Scope of the test driver? (Was: Re: [PATCH] test_driver: implement virDomainSendKey)
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.
7:16 a.m.
New subject: [libvirt] Scope of the test driver? (Was: Re: [PATCH] test_driver: implement virDomainSendKey)
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
8:51 a.m.
New subject: [libvirt] Scope of the test driver? (Was: Re: [PATCH] test_driver: implement virDomainSendKey)
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
5:25 a.m.
New subject: [libvirt] Scope of the test driver? (Was: Re: [PATCH] test_driver: implement virDomainSendKey)
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
5:27 a.m.
New subject: [libvirt] Scope of the test driver? (Was: Re: [PATCH] test_driver: implement virDomainSendKey)
On Fri, Jun 7, 2019 at 12:25 PM Erik Skultety <eskultet(a)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
5:44 a.m.
New subject: [libvirt] Scope of the test driver? (Was: Re: [PATCH] test_driver: implement virDomainSendKey)
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 :|
1:56 a.m.
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
1:59 a.m.
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
5:52 a.m.
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(a)gmail.com>
---
Reviewed-by: Erik Skultety <eskultet(a)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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
1960
days inactive
1966
days old
15 comments
6 participants
participants (6)
-
Daniel P. Berrangé -
Erik Skultety -
Ilias Stamatis -
Ján Tomko -
Pavel Hrdina -
Peter Krempa