On Thu, Mar 17, 2022 at 12:04 PM Hanna Reitz <hre...@redhat.com> wrote: > > On 17.03.22 16:58, John Snow wrote: > > On Thu, Mar 17, 2022 at 11:28 AM Hanna Reitz <hre...@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 >
