Re: [Qemu-block] [PATCH 7/7] iotests: Rework 113

[Max Reitz]

On 2018-05-02 20:18, John Snow wrote:
> 
> 
> On 05/02/2018 02:13 PM, Max Reitz wrote:
>> On 2018-05-02 20:03, John Snow wrote:
>>>
>>>
>>> On 04/21/2018 12:54 PM, Max Reitz wrote:
>>>> This test case has been broken since 398e6ad014df261d (roughly half a
>>>> year).  qemu-img amend requires its output image to be R/W, so it opens
>>>> it as such; the node is then turned into an read-only node automatically
>>>> which is now accompanied by a warning, however.  This warning has not
>>>> been part of the reference output.
>>>>
>>>> For one thing, this warning shows that we cannot keep the test case as
>>>> it is.  We would need a format that has no create_opts but that does
>>>> have write support -- we do not have such a format, though.
>>>>
>>>> Another thing is that qemu now actually checks whether an image format
>>>> supports amendment instead of whether it has create_opts (since the
>>>> former always implies the latter).  So we can now use any format that
>>>> does not support amendment (even if it supports creation) and thus test
>>>> the same code path.
>>>>
>>>> The reason nobody has noticed the breakage until now of course is the
>>>> fact that nobody runs the iotests for nbd+bochs.  There actually was
>>>> never any reason to set the protocol to "nbd" but because that was
>>>> technically correct; functionally it made no difference.  So that is the
>>>> first thing we are going to change: Make the protocol "file" instead so
>>>> that people might actually notice breakage here.
>>>>
>>>> Secondly, now that bochs no longer works for the amend test case, we
>>>> have to change the format there anyway.  Set let us just bend the truth
>>
>> I suppose s/Set/So/
>>
>>>> a bit, declare this test a raw test.  In fact, that does not even
>>>> concern the bochs test cases, other than the output now reading 'bochs'
>>>> instead of 'IMGFMT'.
>>>>
>>>> So with this test now being a raw test, we can rework the amend test
>>>> case to use raw instead.
>>>>
>>>> Signed-off-by: Max Reitz <address@hidden>
>>>
>>> Well, it passes... Not sure if I'm wild about the format change, it
>>> sounds like a failure of our CI more than something that needed to
>>> change in the test, but... shrug.
>>
>> I think it's not really an issue in the CI.  Testing all possible
>> combinations of protocols and formats seems too much to me.
>>
>> I think it really is an issue in our test suite.  There are many tests
>> that work only with a single combination (numerous file+qcow2 tests, for
>> instance), so having our great test matrix capability is completely
>> useless for them.
>>
>> Maybe tests should be able to offer a preferred format+protocol
>> (+options) combination?  Then, when you run check without any such
>> arguments, it would just run each test in its preferred mode.
>>
>> Max
>>
> 
> "You're not wrong" -- iotests presents an impossible matrix. You're not
> going to fix our infrastructural problems with this one patch, which
> looks like a hack in contrast to how the rest of iotests is presently
> structured.
> 
> I don't oppose it, though. Just voicing my full-throated ambivalence.
> There's definitely a discussion or two to be had about what
> instrumenting iotests looks like and how we can ensure we're getting
> proper coverage. The current solution *is* bad.

I do completely agree with you.  It's a hack.  That's why my
justification in the commit message is rather long.

And indeed I was thinking about what we could do to the iotests to get
around this.  I thought of maybe fixing some iotests to a specific
format+protocol combination and then always run them with that,
regardless of what the user specified -- but that would mean running
them always, which is stupid because it would take too much time for
every combination you want to test.

I didn't think of adding a "default" mode to the check script, though,
which would allow you to run that fixed combination only then.  (Though
you may want to be able to run such tests with different combinations,
too, though -- the "default" mode just ensures the test is run at all).

So my solution was to just hack around this with a questionable
(although not fully wrong) explanation; and to see which tests I
personally didn't run so far (and then added the proper combinations to
my test environment so they would run (except for some, e.g. the ones
requiring root)).  Then I decided that would be enough for me right now...

Max

signature.asc
Description: OpenPGP digital signature