I will start by reviewing the test code, so we can agree on expected
syntax/behavior, before reviewing the implementation:
On Fri, Mar 29, 2013 at 06:14:08PM +0100, Jiří Župka wrote:
> Signed-off-by: Jiří Župka <jzupka@xxxxxxxxxx>
> ---
> virttest/cartesian_config_unittest.py | 79 +++++++++++++++++++++++++++++++++++
> 1 file changed, 79 insertions(+)
What about squashing this into the previous patch? I don't see a reason
to put unit-tests and implementation in separate commits.
>
> diff --git a/virttest/cartesian_config_unittest.py b/virttest/cartesian_config_unittest.py
> index afc1b14..98c1efc 100755
> --- a/virttest/cartesian_config_unittest.py
> +++ b/virttest/cartesian_config_unittest.py
> @@ -86,6 +86,85 @@ class CartesianConfigTest(unittest.TestCase):
> )
>
>
> + def testNameVariant(self):
> + self._checkStringDump("""
> + variants name=tests: # All tests in configuration
I like how you tried to make the syntax extensible, but:
1) I dislike the fact that all variants-block "metadata" will have to be
stuck in a single line if we extend this.
2) I find the model required to understand what "name=tests" means
confusing. With the syntax above, "name" (the left-hand side of the "="
sign) is a kind of variable/option name, but the right-hand side of the
"=" sign ("tests") is _also_ a variable/option name.
I mean: on all programming languages I know, variables are declared like
this:
var i;
or:
int i;
not like this:
var name=i;
or:
var type=int name=i;
That said, I would love to have extensibility to allow other
variants-block metadata in the future, but I think the variants-block
_name_ is special and doesn't need to be prepended with "name=". IMO,
the "name=" prefix makes the semantics more confusing, not clearer.
> + - wait:
> + run = "wait"
> + variants:
> + - long:
> + time = short_time
> + - short: long
> + time = logn_time
> + - test2:
> + run = "test1"
> +
> + variants name=virt_system:
> + - @linux:
> + - windows:
> +
> + variants name=host_os:
> + - linux:
> + image = linux
> + - windows:
> + image = windows
> +
> + only host_os>linux
That this ">" syntax mean? Is it something new?
Is the ">" operator whitespace-sensitive? Would "host_os > linux" work?
> + """,
> + [
> + {'dep': [],
> + 'host_os': 'linux',
> + 'image': 'linux',
> + 'name': 'host_os>linux.virt_system>linux.tests>wait.long',
> + 'run': 'wait',
> + 'shortname': 'host_os>linux.tests>wait.long',
> + 'tests': 'wait',
> + 'time': 'short_time',
> + 'virt_system': 'linux'},
> + {'dep': ['host_os>linux.virt_system>linux.tests>wait.long'],
> + 'host_os': 'linux',
> + 'image': 'linux',
> + 'name': 'host_os>linux.virt_system>linux.tests>wait.short',
> + 'run': 'wait',
> + 'shortname': 'host_os>linux.tests>wait.short',
> + 'tests': 'wait',
> + 'time': 'logn_time',
> + 'virt_system': 'linux'},
> + {'dep': [],
> + 'host_os': 'linux',
> + 'image': 'linux',
> + 'name': 'host_os>linux.virt_system>linux.tests>test2',
> + 'run': 'test1',
> + 'shortname': 'host_os>linux.tests>test2',
> + 'tests': 'test2',
> + 'virt_system': 'linux'},
> + {'dep': [],
> + 'host_os': 'linux',
> + 'image': 'linux',
> + 'name': 'host_os>linux.virt_system>windows.tests>wait.long',
> + 'run': 'wait',
> + 'shortname': 'host_os>linux.virt_system>windows.tests>wait.long',
> + 'tests': 'wait',
> + 'time': 'short_time',
> + 'virt_system': 'windows'},
> + {'dep': ['host_os>linux.virt_system>windows.tests>wait.long'],
> + 'host_os': 'linux',
> + 'image': 'linux',
> + 'name': 'host_os>linux.virt_system>windows.tests>wait.short',
> + 'run': 'wait',
> + 'shortname': 'host_os>linux.virt_system>windows.tests>wait.short',
> + 'tests': 'wait',
> + 'time': 'logn_time',
> + 'virt_system': 'windows'},
> + {'dep': [],
> + 'host_os': 'linux',
> + 'image': 'linux',
> + 'name': 'host_os>linux.virt_system>windows.tests>test2',
> + 'run': 'test1',
> + 'shortname': 'host_os>linux.virt_system>windows.tests>test2',
> + 'tests': 'test2',
> + 'virt_system': 'windows'},
> + ])
> def testHugeTest1(self):
> self._checkConfigDump('testcfg.huge/test1.cfg',
> 'testcfg.huge/test1.cfg.repr.gz')
> --
> 1.8.1.4
>
--
Eduardo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
