Re: [Qemu-devel] [PATCH 8/8] OptsVisitor: introduce unit tests, with test cases for range flattening
On 08/19/13 21:55, Laszlo Ersek wrote: > On 08/19/13 21:26, Luiz Capitulino wrote: >> On Mon, 22 Jul 2013 23:07:36 +0200 >> Laszlo Ersek wrote: >> >>> >>> Signed-off-by: Laszlo Ersek >> >> This patch now conflicts, can you respin please? > > Can you retry with "git am -3"? > > "git rebase -i" didn't ask me to do anything manually, so I'm guessing > if you let "git am" to do a 3-way merge, it should work. > > (Of course I don't have anything against posting a v2, this is just an > attempt to keep the traffic low.) ... yeah it looks like: - a trivial context change in "tests/Makefile" (due to commit 3464700f), - "qapi-schema-test.json" has been renamed to "tests/qapi-schema/qapi-schema-test.json" (commit 4f193e34): diff --git a/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json similarity index 100% rename from qapi-schema-test.json rename to tests/qapi-schema/qapi-schema-test.json These should be no problem for "git am -3". Thanks, Laszlo
Re: [Qemu-devel] [PATCH 8/8] OptsVisitor: introduce unit tests, with test cases for range flattening
On 08/19/13 21:26, Luiz Capitulino wrote: > On Mon, 22 Jul 2013 23:07:36 +0200 > Laszlo Ersek wrote: > >> >> Signed-off-by: Laszlo Ersek > > This patch now conflicts, can you respin please? Can you retry with "git am -3"? "git rebase -i" didn't ask me to do anything manually, so I'm guessing if you let "git am" to do a 3-way merge, it should work. (Of course I don't have anything against posting a v2, this is just an attempt to keep the traffic low.) Thanks! Laszlo
Re: [Qemu-devel] [PATCH 8/8] OptsVisitor: introduce unit tests, with test cases for range flattening
On Mon, 22 Jul 2013 23:07:36 +0200
Laszlo Ersek wrote:
>
> Signed-off-by: Laszlo Ersek
This patch now conflicts, can you respin please?
> ---
> tests/Makefile|6 +-
> qapi-schema-test.json | 15 +++
> tests/test-opts-visitor.c | 275
> +
> .gitignore|1 +
> 4 files changed, 296 insertions(+), 1 deletions(-)
> create mode 100644 tests/test-opts-visitor.c
>
> diff --git a/tests/Makefile b/tests/Makefile
> index 425a9a8..8bb290e 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -23,6 +23,8 @@ check-unit-y += tests/test-string-input-visitor$(EXESUF)
> gcov-files-test-string-input-visitor-y = qapi/string-input-visitor.c
> check-unit-y += tests/test-string-output-visitor$(EXESUF)
> gcov-files-test-string-output-visitor-y = qapi/string-output-visitor.c
> +check-unit-y += tests/test-opts-visitor$(EXESUF)
> +gcov-files-test-opts-visitor-y = qapi/opts-visitor.c
> check-unit-y += tests/test-coroutine$(EXESUF)
> gcov-files-test-coroutine-y = coroutine-$(CONFIG_COROUTINE_BACKEND).c
> check-unit-y += tests/test-visitor-serialization$(EXESUF)
> @@ -81,7 +83,8 @@ test-obj-y = tests/check-qint.o tests/check-qstring.o
> tests/check-qdict.o \
> tests/test-string-input-visitor.o tests/test-qmp-output-visitor.o \
> tests/test-qmp-input-visitor.o tests/test-qmp-input-strict.o \
> tests/test-qmp-commands.o tests/test-visitor-serialization.o \
> - tests/test-x86-cpuid.o tests/test-mul64.o tests/test-int128.o
> + tests/test-x86-cpuid.o tests/test-mul64.o tests/test-int128.o \
> + tests/test-opts-visitor.o
>
> test-qapi-obj-y = tests/test-qapi-visit.o tests/test-qapi-types.o
>
> @@ -123,6 +126,7 @@ tests/test-qmp-input-visitor$(EXESUF):
> tests/test-qmp-input-visitor.o $(test-qap
> tests/test-qmp-input-strict$(EXESUF): tests/test-qmp-input-strict.o
> $(test-qapi-obj-y) libqemuutil.a libqemustub.a
> tests/test-qmp-commands$(EXESUF): tests/test-qmp-commands.o
> tests/test-qmp-marshal.o $(test-qapi-obj-y) qapi-types.o qapi-visit.o
> libqemuutil.a libqemustub.a
> tests/test-visitor-serialization$(EXESUF):
> tests/test-visitor-serialization.o $(test-qapi-obj-y) libqemuutil.a
> libqemustub.a
> +tests/test-opts-visitor$(EXESUF): tests/test-opts-visitor.o
> $(test-qapi-obj-y) libqemuutil.a libqemustub.a
>
> tests/test-mul64$(EXESUF): tests/test-mul64.o libqemuutil.a
>
> diff --git a/qapi-schema-test.json b/qapi-schema-test.json
> index 4434fa3..fe5af75 100644
> --- a/qapi-schema-test.json
> +++ b/qapi-schema-test.json
> @@ -51,3 +51,18 @@
> { 'command': 'user_def_cmd', 'data': {} }
> { 'command': 'user_def_cmd1', 'data': {'ud1a': 'UserDefOne'} }
> { 'command': 'user_def_cmd2', 'data': {'ud1a': 'UserDefOne', 'ud1b':
> 'UserDefOne'}, 'returns': 'UserDefTwo' }
> +
> +# For testing integer range flattening in opts-visitor. The following schema
> +# corresponds to the option format:
> +#
> +# -userdef i64=3-6,i64=-5--1,u64=2,u16=1,u16=7-12
> +#
> +# For simplicity, this example doesn't use [type=]discriminator nor optargs
> +# specific to discriminator values.
> +{ 'type': 'UserDefOptions',
> + 'data': {
> +'*i64' : [ 'int'],
> +'*u64' : [ 'uint64' ],
> +'*u16' : [ 'uint16' ],
> +'*i64x': 'int' ,
> +'*u64x': 'uint64' } }
> diff --git a/tests/test-opts-visitor.c b/tests/test-opts-visitor.c
> new file mode 100644
> index 000..9f902b5
> --- /dev/null
> +++ b/tests/test-opts-visitor.c
> @@ -0,0 +1,275 @@
> +/*
> + * Options Visitor unit-tests.
> + *
> + * Copyright (C) 2013 Red Hat, Inc.
> + *
> + * Authors:
> + * Laszlo Ersek (based on test-string-output-visitor)
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include
> +
> +#include "qemu/config-file.h" /* qemu_add_opts() */
> +#include "qemu/option.h" /* qemu_opts_parse() */
> +#include "qapi/opts-visitor.h"/* opts_visitor_new() */
> +#include "test-qapi-visit.h" /* visit_type_UserDefOptions() */
> +#include "qapi/dealloc-visitor.h" /* qapi_dealloc_visitor_new() */
> +
> +static QemuOptsList userdef_opts = {
> +.name = "userdef",
> +.head = QTAILQ_HEAD_INITIALIZER(userdef_opts.head),
> +.desc = { { 0 } } /* validated with OptsVisitor */
> +};
> +
> +/* fixture (= glib test case context) and test case manipulation */
> +
> +typedef struct OptsVisitorFixture {
> +UserDefOptions *userdef;
> +Error *err;
> +} OptsVisitorFixture;
> +
> +
> +static void
> +setup_fixture(OptsVisitorFixture *f, gconstpointer test_data)
> +{
> +const char *opts_string = test_data;
> +QemuOpts *opts;
> +OptsVisitor *ov;
> +
> +opts = qemu_opts_parse(qemu_find_opts("userdef"), opts_string, 0);
> +g_assert(opts != NULL);
> +
> +ov = opts_visitor_new(opts);
> +visit_type_UserDefOptions(opts_get_visitor(ov), &f->userdef, NULL,
> +
Re: [Qemu-devel] [PATCH 8/8] OptsVisitor: introduce unit tests, with test cases for range flattening
On 07/23/13 00:26, Eric Blake wrote: > On 07/22/2013 04:24 PM, Laszlo Ersek wrote: >>> Pretty thorough, although I thought of a couple other ideas to test: >>> i64=5z-6 should fail; i64=5-6-7 should fail >> >> I can add them if you insist, but I wrote (and single-stepped all of) >> the test cases so that all branches added by patches 3, 5 and 6 would be >> covered. (Some of the final tests in this function are actually >> redundant, but I liked how they looked :)) >> >> For example, "i64=5z-6" is no different from "i64=5z", in patch 3 both >> the first added (*endptr == '\0') condition and the (*endptr == '-') >> fail the same way for both input strings: we never look past the "z". >> >> Likewise, "i64=5-6-7" is the same case as "i64=5-6z": both characters >> after the "6" (ie. "-" and "z") violate the second added (*endptr == >> '\0') condition in patch 3 the same way. >> >> Do you accept this argument? :) > > Yes, I can agree you have 100% code coverage as currently coded. Adding > what currently forms redundant cases may avoid future patch-writers from > breaking 100% coverage while actually triggering different paths between > the cases; but at the same time, we can assume such a future > patch-writer would be adding some new feature to the parser, and could > expand the testsuite accordingly as part of their efforts. Agreed! > So no, I > won't insist on a respin :) Thank you very much :) /me bows and scrapes Laszlo
Re: [Qemu-devel] [PATCH 8/8] OptsVisitor: introduce unit tests, with test cases for range flattening
On 07/23/13 00:04, Eric Blake wrote:
> On 07/22/2013 03:07 PM, Laszlo Ersek wrote:
>> Signed-off-by: Laszlo Ersek
>> ---
>> tests/Makefile|6 +-
>> qapi-schema-test.json | 15 +++
>> tests/test-opts-visitor.c | 275
>> +
>> .gitignore|1 +
>> 4 files changed, 296 insertions(+), 1 deletions(-)
>> create mode 100644 tests/test-opts-visitor.c
>
>> +add_test("/visitor/opts/i64/val1/errno",&expect_fail,
>> + "i64=0x8000");
>> +add_test("/visitor/opts/i64/val1/empty",&expect_fail, "i64=");
>> +add_test("/visitor/opts/i64/val1/trailing", &expect_fail, "i64=5z");
>> +add_test("/visitor/opts/i64/nonlist", &expect_fail, "i64x=5-6");
>> +add_test("/visitor/opts/i64/val2/errno",&expect_fail,
>> + "i64=0x7fff-0x8000");
>> +add_test("/visitor/opts/i64/val2/empty",&expect_fail, "i64=5-");
>> +add_test("/visitor/opts/i64/val2/trailing", &expect_fail, "i64=5-6z");
>> +add_test("/visitor/opts/i64/range/empty", &expect_fail, "i64=6-5");
>> +add_test("/visitor/opts/i64/range/minval", &expect_i64_min,
>> + "i64=-0x8000--0x8000");
>> +add_test("/visitor/opts/i64/range/maxval", &expect_i64_max,
>> + "i64=0x7fff-0x7fff");
>
> Pretty thorough, although I thought of a couple other ideas to test:
> i64=5z-6 should fail; i64=5-6-7 should fail
I can add them if you insist, but I wrote (and single-stepped all of)
the test cases so that all branches added by patches 3, 5 and 6 would be
covered. (Some of the final tests in this function are actually
redundant, but I liked how they looked :))
For example, "i64=5z-6" is no different from "i64=5z", in patch 3 both
the first added (*endptr == '\0') condition and the (*endptr == '-')
fail the same way for both input strings: we never look past the "z".
Likewise, "i64=5-6-7" is the same case as "i64=5-6z": both characters
after the "6" (ie. "-" and "z") violate the second added (*endptr ==
'\0') condition in patch 3 the same way.
Do you accept this argument? :)
Thanks!
Laszlo
Re: [Qemu-devel] [PATCH 8/8] OptsVisitor: introduce unit tests, with test cases for range flattening
On 07/22/2013 04:24 PM, Laszlo Ersek wrote: >> Pretty thorough, although I thought of a couple other ideas to test: >> i64=5z-6 should fail; i64=5-6-7 should fail > > I can add them if you insist, but I wrote (and single-stepped all of) > the test cases so that all branches added by patches 3, 5 and 6 would be > covered. (Some of the final tests in this function are actually > redundant, but I liked how they looked :)) > > For example, "i64=5z-6" is no different from "i64=5z", in patch 3 both > the first added (*endptr == '\0') condition and the (*endptr == '-') > fail the same way for both input strings: we never look past the "z". > > Likewise, "i64=5-6-7" is the same case as "i64=5-6z": both characters > after the "6" (ie. "-" and "z") violate the second added (*endptr == > '\0') condition in patch 3 the same way. > > Do you accept this argument? :) Yes, I can agree you have 100% code coverage as currently coded. Adding what currently forms redundant cases may avoid future patch-writers from breaking 100% coverage while actually triggering different paths between the cases; but at the same time, we can assume such a future patch-writer would be adding some new feature to the parser, and could expand the testsuite accordingly as part of their efforts. So no, I won't insist on a respin :) -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 8/8] OptsVisitor: introduce unit tests, with test cases for range flattening
On 07/22/2013 03:07 PM, Laszlo Ersek wrote:
> Signed-off-by: Laszlo Ersek
> ---
> tests/Makefile|6 +-
> qapi-schema-test.json | 15 +++
> tests/test-opts-visitor.c | 275
> +
> .gitignore|1 +
> 4 files changed, 296 insertions(+), 1 deletions(-)
> create mode 100644 tests/test-opts-visitor.c
> +add_test("/visitor/opts/i64/val1/errno",&expect_fail,
> + "i64=0x8000");
> +add_test("/visitor/opts/i64/val1/empty",&expect_fail, "i64=");
> +add_test("/visitor/opts/i64/val1/trailing", &expect_fail, "i64=5z");
> +add_test("/visitor/opts/i64/nonlist", &expect_fail, "i64x=5-6");
> +add_test("/visitor/opts/i64/val2/errno",&expect_fail,
> + "i64=0x7fff-0x8000");
> +add_test("/visitor/opts/i64/val2/empty",&expect_fail, "i64=5-");
> +add_test("/visitor/opts/i64/val2/trailing", &expect_fail, "i64=5-6z");
> +add_test("/visitor/opts/i64/range/empty", &expect_fail, "i64=6-5");
> +add_test("/visitor/opts/i64/range/minval", &expect_i64_min,
> + "i64=-0x8000--0x8000");
> +add_test("/visitor/opts/i64/range/maxval", &expect_i64_max,
> + "i64=0x7fff-0x7fff");
Pretty thorough, although I thought of a couple other ideas to test:
i64=5z-6 should fail; i64=5-6-7 should fail
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
[Qemu-devel] [PATCH 8/8] OptsVisitor: introduce unit tests, with test cases for range flattening
Signed-off-by: Laszlo Ersek
---
tests/Makefile|6 +-
qapi-schema-test.json | 15 +++
tests/test-opts-visitor.c | 275 +
.gitignore|1 +
4 files changed, 296 insertions(+), 1 deletions(-)
create mode 100644 tests/test-opts-visitor.c
diff --git a/tests/Makefile b/tests/Makefile
index 425a9a8..8bb290e 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -23,6 +23,8 @@ check-unit-y += tests/test-string-input-visitor$(EXESUF)
gcov-files-test-string-input-visitor-y = qapi/string-input-visitor.c
check-unit-y += tests/test-string-output-visitor$(EXESUF)
gcov-files-test-string-output-visitor-y = qapi/string-output-visitor.c
+check-unit-y += tests/test-opts-visitor$(EXESUF)
+gcov-files-test-opts-visitor-y = qapi/opts-visitor.c
check-unit-y += tests/test-coroutine$(EXESUF)
gcov-files-test-coroutine-y = coroutine-$(CONFIG_COROUTINE_BACKEND).c
check-unit-y += tests/test-visitor-serialization$(EXESUF)
@@ -81,7 +83,8 @@ test-obj-y = tests/check-qint.o tests/check-qstring.o
tests/check-qdict.o \
tests/test-string-input-visitor.o tests/test-qmp-output-visitor.o \
tests/test-qmp-input-visitor.o tests/test-qmp-input-strict.o \
tests/test-qmp-commands.o tests/test-visitor-serialization.o \
- tests/test-x86-cpuid.o tests/test-mul64.o tests/test-int128.o
+ tests/test-x86-cpuid.o tests/test-mul64.o tests/test-int128.o \
+ tests/test-opts-visitor.o
test-qapi-obj-y = tests/test-qapi-visit.o tests/test-qapi-types.o
@@ -123,6 +126,7 @@ tests/test-qmp-input-visitor$(EXESUF):
tests/test-qmp-input-visitor.o $(test-qap
tests/test-qmp-input-strict$(EXESUF): tests/test-qmp-input-strict.o
$(test-qapi-obj-y) libqemuutil.a libqemustub.a
tests/test-qmp-commands$(EXESUF): tests/test-qmp-commands.o
tests/test-qmp-marshal.o $(test-qapi-obj-y) qapi-types.o qapi-visit.o
libqemuutil.a libqemustub.a
tests/test-visitor-serialization$(EXESUF): tests/test-visitor-serialization.o
$(test-qapi-obj-y) libqemuutil.a libqemustub.a
+tests/test-opts-visitor$(EXESUF): tests/test-opts-visitor.o $(test-qapi-obj-y)
libqemuutil.a libqemustub.a
tests/test-mul64$(EXESUF): tests/test-mul64.o libqemuutil.a
diff --git a/qapi-schema-test.json b/qapi-schema-test.json
index 4434fa3..fe5af75 100644
--- a/qapi-schema-test.json
+++ b/qapi-schema-test.json
@@ -51,3 +51,18 @@
{ 'command': 'user_def_cmd', 'data': {} }
{ 'command': 'user_def_cmd1', 'data': {'ud1a': 'UserDefOne'} }
{ 'command': 'user_def_cmd2', 'data': {'ud1a': 'UserDefOne', 'ud1b':
'UserDefOne'}, 'returns': 'UserDefTwo' }
+
+# For testing integer range flattening in opts-visitor. The following schema
+# corresponds to the option format:
+#
+# -userdef i64=3-6,i64=-5--1,u64=2,u16=1,u16=7-12
+#
+# For simplicity, this example doesn't use [type=]discriminator nor optargs
+# specific to discriminator values.
+{ 'type': 'UserDefOptions',
+ 'data': {
+'*i64' : [ 'int'],
+'*u64' : [ 'uint64' ],
+'*u16' : [ 'uint16' ],
+'*i64x': 'int' ,
+'*u64x': 'uint64' } }
diff --git a/tests/test-opts-visitor.c b/tests/test-opts-visitor.c
new file mode 100644
index 000..9f902b5
--- /dev/null
+++ b/tests/test-opts-visitor.c
@@ -0,0 +1,275 @@
+/*
+ * Options Visitor unit-tests.
+ *
+ * Copyright (C) 2013 Red Hat, Inc.
+ *
+ * Authors:
+ * Laszlo Ersek (based on test-string-output-visitor)
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include
+
+#include "qemu/config-file.h" /* qemu_add_opts() */
+#include "qemu/option.h" /* qemu_opts_parse() */
+#include "qapi/opts-visitor.h"/* opts_visitor_new() */
+#include "test-qapi-visit.h" /* visit_type_UserDefOptions() */
+#include "qapi/dealloc-visitor.h" /* qapi_dealloc_visitor_new() */
+
+static QemuOptsList userdef_opts = {
+.name = "userdef",
+.head = QTAILQ_HEAD_INITIALIZER(userdef_opts.head),
+.desc = { { 0 } } /* validated with OptsVisitor */
+};
+
+/* fixture (= glib test case context) and test case manipulation */
+
+typedef struct OptsVisitorFixture {
+UserDefOptions *userdef;
+Error *err;
+} OptsVisitorFixture;
+
+
+static void
+setup_fixture(OptsVisitorFixture *f, gconstpointer test_data)
+{
+const char *opts_string = test_data;
+QemuOpts *opts;
+OptsVisitor *ov;
+
+opts = qemu_opts_parse(qemu_find_opts("userdef"), opts_string, 0);
+g_assert(opts != NULL);
+
+ov = opts_visitor_new(opts);
+visit_type_UserDefOptions(opts_get_visitor(ov), &f->userdef, NULL,
+ &f->err);
+opts_visitor_cleanup(ov);
+qemu_opts_del(opts);
+}
+
+
+static void
+teardown_fixture(OptsVisitorFixture *f, gconstpointer test_data)
+{
+if (f->userdef != NULL) {
+QapiDeallocVisitor *dv;
+
+dv = qapi_dealloc_visitor_new();
+visit_type_UserDefOptions(qapi_dealloc_get_visitor(dv), &f->userde
