Re: [Qemu-devel] [PATCH 1/2] tests: Expose regression in QemuOpts visitor

[Laurent Vivier <lvivier@redhat.com>]

On 21/03/2017 17:01, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 03/21/2017 08:33 AM, Laurent Vivier wrote:
>>> On 21/03/2017 14:21, Eric Blake wrote:
>>>> On 03/21/2017 04:01 AM, Laurent Vivier wrote:
>>>>> On 21/03/2017 04:17, Eric Blake wrote:
>>>>>> Commit 15c2f669e broke the ability of the QemuOpts visitor to
>>>>>> flag extra input parameters, but the regression went unnoticed
>>>>>> because of missing testsuite coverage.  Add a test to cover this.
>>>>>
>>>>> I don't know where I'm wrong, but when I run this test without the fix
>>>>> it never fails.
>>>>
>>>> Intentional:
>>>>
>>>>
>>>>>> +    v = opts_visitor_new(opts);
>>>>>> +    /* FIXME: bogus should be diagnosed */
>>>>>> +    visit_type_UserDefOptions(v, NULL, &userdef, &error_abort);
>>>>
>>>> The test is written with a FIXME here, then updated in the next patch to
>>>> remove the fixme and adjust the condition to what we really want, so
>>>> that 'make check-unit' is not broken in the meantime.
>>>
>>> OK.
>>>
>>> Why don't you reverse the patch order to have a commit to apply the fix
>>> and a commit to apply the test (fully)?
>>>
>>> Like this, it easy to not apply the fix and only the test to check the
>>> test really detects the problem and the fix really fix it (it's what
>>> I've tried to do)... and the "make check" is never broken.
>>
>> Applying just the one-liner fix to qapi/opts-visitor.c in isolation
>> already causes a 'make check' failure; a careful read of 2/2 shows that
>> I was adjusting the expected output of two separate tests, added over
>> two separate commits, but both with a BUG/FIXME tag.  I'm not opposed to
>> reworking the series to apply the testsuite coverage after the bug fix,
>> if that is deemed necessary, but would like an opinion from Markus which
>> way he would prefer (as this is the code he maintains) before causing
>> myself artificial churn.
> 
> I really, really like to start with the problem statement (test case),
> not the solution.  I also like see the solution's effect in the update
> to the test case.
> 
> Since "make check" must not fail, and our (rickety) testing framework
> doesn't let us express "this is expected to fail", the problem statement
> can't be a failing test case, but has to be a test case expecting the
> broken behavior.
> 
> If that's not good enough to convince you that it detects the problem, I
> recommend to git-checkout tests/ after the fix into the tree before the
> fix.
> 

OK. I'm convinced :)

Thanks,
Laurent