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

[John Snow]

On Thu, Mar 17, 2022 at 12:04 PM Hanna Reitz <hreitz@redhat.com> wrote:
>
> 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).

Oh! Today-I-Learned. Yeah, the ability to skip cases is nice indeed.

>
> > 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.
>

Yeah, I like having both. Knowing which call failed exactly is helpful
for fixing the test and/or reproducing it on your own. Seeing the
output can give extremely important clues about the nature of the
failure.

> > 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...?
>

Alright, I'll see what I can do. Thanks for the review, I know churn
like this isn't the most fulfilling thing.

> Hanna
>