qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Hanna Reitz
Subject: Re: [PATCH 12/14] iotests: remove qemu_img_pipe_and_status()
Date: Thu, 17 Mar 2022 17:04:04 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.0

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]