| Message ID | 1364577250-2637-6-git-send-email-jzupka@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@redhat.com> > --- > 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 >
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 + - 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 + """, + [ + {'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')
Signed-off-by: Ji?í Župka <jzupka@redhat.com> --- virttest/cartesian_config_unittest.py | 79 +++++++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+)