Re: [PATCH v5 1/5] iotests: remove 'linux' from default supported platforms

[Thomas Huth <thuth@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5649 bytes --]

On 04/10/2019 14.44, Max Reitz wrote:
> On 04.10.19 12:19, Kevin Wolf wrote:
>> Am 02.10.2019 um 19:47 hat Max Reitz geschrieben:
>>> On 02.10.19 18:44, Kevin Wolf wrote:
>>>> Am 02.10.2019 um 13:57 hat Max Reitz geschrieben:
>>>>> It usually worked fine for me because it’s rather rare that non-block
>>>>> patches broke the iotests.
>>>>
>>>> I disagree. It happened all the time that someone else broke the iotests
>>>> in master and we needed to fix it up.
>>>
>>> OK.
>>>
>>> (In my experience, it’s still mostly block patches, only that they tend
>>> to go through non-block trees.)
>>
>> Fair enough, it's usually code that touches block code. I assumed "block
>> patches" to mean patches that go through one of the block trees and for
>> which iotests are run before sending a pull request.
>>
>> In the end, I don't care what code these patches touched. I do an
>> innocent git pull, and when I finally see that it's master that breaks
>> iotests and not my patches on top of it, I'm annoyed.
> 
> Hm.  Part of my point was that this still happens all the time.
> 
> Which is why I’d prefer more tests to run in CI than a handful of not
> very useful ones in make check.

Ok, so let's try to add some more useful test to the "auto" group. Kevin
mentioned 030, 040 and 041, and I think it should be ok to enable them
(IIRC the only issue was that they run a little bit longer, but if they
are very useful, we should include them anyway).

Do you have any other tests in mind which could be very useful?

[...]
>> So if you merge that revert, when iotests break in master, I take it I
>> can rely on you to fix it up before it impacts my working branch?
> 
> This is not a game, I’m talking purely from my standpoint:
> (I talked wrongly, but more on that below)
> 
> Whenever make check fails, it’s urgent.  Without iotests running in make
> check, we had some time to sort the situation out.  That’s no longer the
> case.
>
> That means we need to take care of everything immediately.  And I purely
> want help with that.

While that sounds noble at a first glance, I think you've maybe got too
high expectations at yourself here? I mean, all other maintainers of
other subsystems with tests have the same "problem" if the tests for
their subsystem run in "make check". The "normal" behavior that I've
seen so far (and which I think is the right way to deal with it):

1) If a pull request gets rejected due to a "make check" failure, simply
drop the offending patches from the pull request, send a v2 pull req
without them, and tell the author of the offending patches to fix the
problem (unless the fix is completely trivial and could immediately be
applied to the v2 pull req). You really don't have to fix each and every
issue on your own as a maintainer and can redirect this to the patch
authors again.

2) If a test fails occasionally during "make check" (due to race
conditions or whatever), it gets disabled from "make check" if it can't
be fixed easily (e.g. in the qtests it gets moved to the "SPEED=slow"
category, or in iotests it would get removed from the "auto" group).

>> Yes, making iotests stable on CI probably involves some pain, especially
>> initially. However, if we don't fix them upstream, they'll end up on our
>> desk again downstream because people run tests neither on your nor on my
>> laptop.
>>
>> I think we might actually save time by fixing them once and for all,
>> even if they are problems in the test cases and not in QEMU, and making
>> new test cases portable from the start, instead of getting individual
>> reports one at a time and having to look at the same test cases again
>> and again.
> 
> You should really know I’m all for that and that I’ve done my share of
> work there.
> 
> But from my perspective we’ve said and tried that for six years and we
> aren’t there still.  So to me “We should just fix it” of course sounds
> correct, but I also don’t believe it’ll happen any time soon.  Mostly
> because we generally don’t know what to fix before it breaks, but also
> because that’s exactly what we’ve tried to do for at least six years.

Well, I guess we won't ever get there if we don't try. And the hurdles
will rather get higher over the years since more and more tests are
added ...

> OTOH, keeping the iotests in make check means we have to act on failure
> reports immediately.  For example, someone™ needs to investigate the 130
> failure Peter has reported.  I’ve just replied “I don’t know”, CC’d
> Thomas, and waited whether anyone else would do anything.  Nobody did,
> and that can’t work.  (I also wrote that I wouldn’t act on it, precisely
> because I didn’t see the point.  I assumed that if anyone disagreed with
> that statement, they would have said something.)
> 
> So acting on such reports means pain, too.  I consider it greater than
> the previous kind of pain, because I prefer “This sucks, I’ll have to
> fix it this week or so” to “Oh crap, I’ll have to investigate now,
> reproduce it, write an email as soon as possible, and fix it”.

I think that "I have to investigate now ..." mindset is not the right
way to handle these kind of issues. If a test is shaky and can not be
fixed easily, we should simply disable it from the "auto" group again.
So if you like, I can send a patch to remove 130 from the "auto" group
(personally, I'd rather wait to see if it fails anytime soon again, or
if this was maybe rather a one-time issue due to a non-clean test system
...)

 Thomas


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]