| Message ID | 20170321031705.22291-2-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Eric Blake (2017-03-20 22:17:04) > 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. > > Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com> > --- > tests/test-opts-visitor.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/tests/test-opts-visitor.c b/tests/test-opts-visitor.c > index 2238f8e..a47c344 100644 > --- a/tests/test-opts-visitor.c > +++ b/tests/test-opts-visitor.c > @@ -247,6 +247,24 @@ test_opts_range_beyond(void) > qemu_opts_del(opts); > } > > +static void > +test_opts_dict_unvisited(void) > +{ > + QemuOpts *opts; > + Visitor *v; > + UserDefOptions *userdef; > + > + opts = qemu_opts_parse(qemu_find_opts("userdef"), "i64x=0,bogus=1", false, > + &error_abort); > + > + v = opts_visitor_new(opts); > + /* FIXME: bogus should be diagnosed */ > + visit_type_UserDefOptions(v, NULL, &userdef, &error_abort); > + visit_free(v); > + qemu_opts_del(opts); > + qapi_free_UserDefOptions(userdef); > +} > + > int > main(int argc, char **argv) > { > @@ -343,6 +361,8 @@ main(int argc, char **argv) > g_test_add_func("/visitor/opts/range/beyond", > test_opts_range_beyond); > > + g_test_add_func("/visitor/opts/dict/unvisited", test_opts_dict_unvisited); > + > g_test_run(); > return 0; > } > -- > 2.9.3 >
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. Laurent > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > tests/test-opts-visitor.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/tests/test-opts-visitor.c b/tests/test-opts-visitor.c > index 2238f8e..a47c344 100644 > --- a/tests/test-opts-visitor.c > +++ b/tests/test-opts-visitor.c > @@ -247,6 +247,24 @@ test_opts_range_beyond(void) > qemu_opts_del(opts); > } > > +static void > +test_opts_dict_unvisited(void) > +{ > + QemuOpts *opts; > + Visitor *v; > + UserDefOptions *userdef; > + > + opts = qemu_opts_parse(qemu_find_opts("userdef"), "i64x=0,bogus=1", false, > + &error_abort); > + > + v = opts_visitor_new(opts); > + /* FIXME: bogus should be diagnosed */ > + visit_type_UserDefOptions(v, NULL, &userdef, &error_abort); > + visit_free(v); > + qemu_opts_del(opts); > + qapi_free_UserDefOptions(userdef); > +} > + > int > main(int argc, char **argv) > { > @@ -343,6 +361,8 @@ main(int argc, char **argv) > g_test_add_func("/visitor/opts/range/beyond", > test_opts_range_beyond); > > + g_test_add_func("/visitor/opts/dict/unvisited", test_opts_dict_unvisited); > + > g_test_run(); > return 0; > } >
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. A similar approach was taken by Markus in commit 9cb8ef3 (add a test that passes but shows undesirable behavior with a BUG: note), which also gets fixed up by my 2/2. Maybe I should use BUG: instead of FIXME; but it all goes away in the next patch.
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. Thanks, Laurent
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.
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.
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
diff --git a/tests/test-opts-visitor.c b/tests/test-opts-visitor.c index 2238f8e..a47c344 100644 --- a/tests/test-opts-visitor.c +++ b/tests/test-opts-visitor.c @@ -247,6 +247,24 @@ test_opts_range_beyond(void) qemu_opts_del(opts); } +static void +test_opts_dict_unvisited(void) +{ + QemuOpts *opts; + Visitor *v; + UserDefOptions *userdef; + + opts = qemu_opts_parse(qemu_find_opts("userdef"), "i64x=0,bogus=1", false, + &error_abort); + + v = opts_visitor_new(opts); + /* FIXME: bogus should be diagnosed */ + visit_type_UserDefOptions(v, NULL, &userdef, &error_abort); + visit_free(v); + qemu_opts_del(opts); + qapi_free_UserDefOptions(userdef); +} + int main(int argc, char **argv) { @@ -343,6 +361,8 @@ main(int argc, char **argv) g_test_add_func("/visitor/opts/range/beyond", test_opts_range_beyond); + g_test_add_func("/visitor/opts/dict/unvisited", test_opts_dict_unvisited); + g_test_run(); return 0; }
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. Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/test-opts-visitor.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)