Re: [PATCH 12/14] iotests: remove qemu_img_pipe_and_status()

From: Hanna Reitz <hreitz@redhat.com>
To: John Snow <jsnow@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>, Eric Blake <eblake@redhat.com>,
	qemu-devel <qemu-devel@nongnu.org>,
	Qemu-block <qemu-block@nongnu.org>
Subject: Re: [PATCH 12/14] iotests: remove qemu_img_pipe_and_status()
Date: Thu, 17 Mar 2022 17:04:04 +0100	[thread overview]
Message-ID: <1683432d-771e-2ffa-accd-916aaf3801dd@redhat.com> (raw)
In-Reply-To: <CAFn=p-ZpoF+QVZv0Quq8NmYVSvXOtVMxPmynDFSr7hG54aV-CA@mail.gmail.com>

On 17.03.22 16:58, John Snow wrote:
> On Thu, Mar 17, 2022 at 11:28 AM Hanna Reitz <hreitz@redhat.com> wrote:
>> On 09.03.22 04:54, John Snow wrote:
>>> With the exceptional 'create' calls removed in the prior commit, change
>>> qemu_img_log() and img_info_log() to call qemu_img() directly
>>> instead.
>>>
>>> In keeping with the spirit of diff-based tests, allow these calls to
>>> qemu_img() to return an unchecked non-zero status code -- because any
>>> error we'd see from the output is going into the log anyway.
>> :(
>>
>> I’d prefer having an exception that points exactly to where in the test
>> the offending qemu-img call was.  But then again, I dislike such
>> log-based tests anyway, and this is precisely one reason for it...
>>
>> I think Kevin disliked my approach of just `assert qemu_img() == 0`
>> mainly because you don’t get the stderr output with it.  But you’ve
>> solved that problem now, so I don’t think there’s a reason why we
>> wouldn’t want a raised exception.
>>
>> Hanna
>>
> I thought you and Kevin actually preferred diff-based tests, maybe I
> misunderstood. I know that there was a strong dislike of the unittest
> based tests,

Oh gosh not from me.  I really like them, because they allow skipping 
test cases so easily (and because reviewing patches for such tests tend 
to be easier, because the code is explicit about what results it expects).

> and that the new script-style was more preferred. I
> thought inherent to that was an actual preference for diff-based
> itself, but maybe not?
>
> I'd say negative tests are easier with the diff-based as one benefit.
> I'm a little partial to that kind of test. (I noticed that bitmap
> tests were the most habitual user of negative tests involving
> qemu-img, haha.) Otherwise, I guess I don't really care what the test
> mechanism is provided that the error output is informative. Happy to
> defer to consensus between you and Kevin.

I don’t think we have a consensus. :)

But AFAIU the main disagreement was based on what each of us found more 
important when something fails.  Kevin found it more important to see 
what the failing process wrote to stderr, I found it more important to 
see what call failed exactly.  Since your exception model gives us both, 
I believe we can both be happy with it.

> Anyway, this patch (and the ones that follow it, I haven't read your
> feedback on 13-14 yet) doesn't close the door on making everything
> Except-by-default, it would just be further work that needs to happen
> after the fact. How do you want to move forward?
>
> - Replace calls to qemu_img_log() with qemu_img()
> - Make qemu_img_log() raise by default, but log output on success cases
> - Something else?

Second one sounds good to me for this series, because I’d expect it’d 
mean the least amount of changes...?

Hanna