[libvirt] Scope of the test driver? (Was: Re: [PATCH] test_driver: implement virDomainSendKey)

[Pavel Hrdina]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190603/756fdff2/attachment-0001.sig>