On 21/03/2017 17:01, Markus Armbruster wrote: > Eric Blake <ebl...@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
