----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/1740/ -----------------------------------------------------------
Review request for OpenLMI Developers.
Repository: openlmi-providers
Description -------
Add indication testing framework
Set of classes and methods to help with event stream oritnted tests.
Diffs -----
src/account/test/common.py 95abd381089506c3f9b4d6632f2303da322fa585 src/python/lmi/test/ind.py PRE-CREATION src/python/lmi/test/util.py 699507c2340e9a50ff22ce08b67fe3e0a3f9eef8
Diff: http://reviewboard-openlmi.rhcloud.com/r/1740/diff/
Testing -------
Thanks,
Alois Mahdal
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/1740/#review2639 -----------------------------------------------------------
Ship it!
src/python/lmi/test/ind.py http://reviewboard-openlmi.rhcloud.com/r/1740/#comment1455
Seems like run() doesn't accept anything.
src/python/lmi/test/ind.py http://reviewboard-openlmi.rhcloud.com/r/1740/#comment1457
subsctiption -> subscription
src/python/lmi/test/util.py http://reviewboard-openlmi.rhcloud.com/r/1740/#comment1456
Consider making it a subclass of abc.ABCMeta and decorating _run() method as abstract (the one that needs to be overriden).
- Michal Minar
On May 14, 2014, 4:26 p.m., Alois Mahdal wrote:
This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/1740/
(Updated May 14, 2014, 4:26 p.m.)
Review request for OpenLMI Developers.
Repository: openlmi-providers
Description
Add indication testing framework
Set of classes and methods to help with event stream oritnted tests.
Diffs
src/account/test/common.py 95abd381089506c3f9b4d6632f2303da322fa585 src/python/lmi/test/ind.py PRE-CREATION src/python/lmi/test/util.py 699507c2340e9a50ff22ce08b67fe3e0a3f9eef8
Diff: http://reviewboard-openlmi.rhcloud.com/r/1740/diff/
Testing
Thanks,
Alois Mahdal
On May 19, 2014, 9:39 a.m., Michal Minar wrote:
Please disregard the 'Ship it!' flag until all the issues are resolved/revisited/discarded.
- Michal
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/1740/#review2639 -----------------------------------------------------------
On May 14, 2014, 4:26 p.m., Alois Mahdal wrote:
This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/1740/
(Updated May 14, 2014, 4:26 p.m.)
Review request for OpenLMI Developers.
Repository: openlmi-providers
Description
Add indication testing framework
Set of classes and methods to help with event stream oritnted tests.
Diffs
src/account/test/common.py 95abd381089506c3f9b4d6632f2303da322fa585 src/python/lmi/test/ind.py PRE-CREATION src/python/lmi/test/util.py 699507c2340e9a50ff22ce08b67fe3e0a3f9eef8
Diff: http://reviewboard-openlmi.rhcloud.com/r/1740/diff/
Testing
Thanks,
Alois Mahdal
On May 19, 2014, 9:39 a.m., Michal Minar wrote:
src/python/lmi/test/ind.py, lines 144-149 http://reviewboard-openlmi.rhcloud.com/r/1740/diff/1/?file=9884#file9884line144
Seems like run() doesn't accept anything.
TestDriver.run does:
def run(self, args): self.args.update(args) result = self._run() self._cleanup() return result
IndicationTestDriver._run (or any *TestDriver._run) does not.
There's intent to keep the driver interface simple and identical over implementations: Instantiate, call run(case) and, unless you have good reason and you are very careful, throw away.
- Alois
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/1740/#review2639 -----------------------------------------------------------
On May 14, 2014, 4:26 p.m., Alois Mahdal wrote:
This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/1740/
(Updated May 14, 2014, 4:26 p.m.)
Review request for OpenLMI Developers.
Repository: openlmi-providers
Description
Add indication testing framework
Set of classes and methods to help with event stream oritnted tests.
Diffs
src/account/test/common.py 95abd381089506c3f9b4d6632f2303da322fa585 src/python/lmi/test/ind.py PRE-CREATION src/python/lmi/test/util.py 699507c2340e9a50ff22ce08b67fe3e0a3f9eef8
Diff: http://reviewboard-openlmi.rhcloud.com/r/1740/diff/
Testing
Thanks,
Alois Mahdal
On May 19, 2014, 9:39 a.m., Michal Minar wrote:
src/python/lmi/test/ind.py, lines 144-149 http://reviewboard-openlmi.rhcloud.com/r/1740/diff/1/?file=9884#file9884line144
Seems like run() doesn't accept anything.
Alois Mahdal wrote: TestDriver.run does:
def run(self, args): self.args.update(args) result = self._run() self._cleanup() return result IndicationTestDriver._run (or any *TestDriver._run) does not. There's intent to keep the driver interface simple and identical over implementations: Instantiate, call run(case) and, unless you have good reason and you are very careful, throw away.
Ah, you're right. I thought it refers to local _run() method. It's unfortunate naming - having run() and _run() implemented in the same class. Could you please move the comment to the base class instead? Its run() method lacks any docstring. I think it's more appropriate place for this description.
- Michal
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/1740/#review2639 -----------------------------------------------------------
On May 14, 2014, 4:26 p.m., Alois Mahdal wrote:
This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/1740/
(Updated May 14, 2014, 4:26 p.m.)
Review request for OpenLMI Developers.
Repository: openlmi-providers
Description
Add indication testing framework
Set of classes and methods to help with event stream oritnted tests.
Diffs
src/account/test/common.py 95abd381089506c3f9b4d6632f2303da322fa585 src/python/lmi/test/ind.py PRE-CREATION src/python/lmi/test/util.py 699507c2340e9a50ff22ce08b67fe3e0a3f9eef8
Diff: http://reviewboard-openlmi.rhcloud.com/r/1740/diff/
Testing
Thanks,
Alois Mahdal
On May 19, 2014, 9:39 a.m., Michal Minar wrote:
src/python/lmi/test/ind.py, lines 144-149 http://reviewboard-openlmi.rhcloud.com/r/1740/diff/1/?file=9884#file9884line144
Seems like run() doesn't accept anything.
Alois Mahdal wrote: TestDriver.run does:
def run(self, args): self.args.update(args) result = self._run() self._cleanup() return result IndicationTestDriver._run (or any *TestDriver._run) does not. There's intent to keep the driver interface simple and identical over implementations: Instantiate, call run(case) and, unless you have good reason and you are very careful, throw away.
Michal Minar wrote: Ah, you're right. I thought it refers to local _run() method. It's unfortunate naming - having run() and _run() implemented in the same class. Could you please move the comment to the base class instead? Its run() method lacks any docstring. I think it's more appropriate place for this description.
OK, thanks, I'll describe the interface better in the base class.
OTOH, the actual semantics of the args parameter need to be defined per implementation, i.e. only IndicationTestDriver will need, and know what to do with args['handlers'].
But yes, it is probably little bit confusing; I'll have to think of better way how to describe this.
- Alois
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/1740/#review2639 -----------------------------------------------------------
On May 14, 2014, 4:26 p.m., Alois Mahdal wrote:
This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/1740/
(Updated May 14, 2014, 4:26 p.m.)
Review request for OpenLMI Developers.
Repository: openlmi-providers
Description
Add indication testing framework
Set of classes and methods to help with event stream oritnted tests.
Diffs
src/account/test/common.py 95abd381089506c3f9b4d6632f2303da322fa585 src/python/lmi/test/ind.py PRE-CREATION src/python/lmi/test/util.py 699507c2340e9a50ff22ce08b67fe3e0a3f9eef8
Diff: http://reviewboard-openlmi.rhcloud.com/r/1740/diff/
Testing
Thanks,
Alois Mahdal
On May 19, 2014, 9:39 a.m., Michal Minar wrote:
src/python/lmi/test/util.py, line 473 http://reviewboard-openlmi.rhcloud.com/r/1740/diff/1/?file=9885#file9885line473
Consider making it a subclass of abc.ABCMeta and decorating _run() method as abstract (the one that needs to be overriden).
Done.
Note that since commits have been split, fixed version will appear in a new review request; this request will be closed.
On May 19, 2014, 9:39 a.m., Michal Minar wrote:
src/python/lmi/test/ind.py, lines 144-149 http://reviewboard-openlmi.rhcloud.com/r/1740/diff/1/?file=9884#file9884line144
Seems like run() doesn't accept anything.
Alois Mahdal wrote: TestDriver.run does:
def run(self, args): self.args.update(args) result = self._run() self._cleanup() return result IndicationTestDriver._run (or any *TestDriver._run) does not. There's intent to keep the driver interface simple and identical over implementations: Instantiate, call run(case) and, unless you have good reason and you are very careful, throw away.
Michal Minar wrote: Ah, you're right. I thought it refers to local _run() method. It's unfortunate naming - having run() and _run() implemented in the same class. Could you please move the comment to the base class instead? Its run() method lacks any docstring. I think it's more appropriate place for this description.
Alois Mahdal wrote: OK, thanks, I'll describe the interface better in the base class.
OTOH, the actual semantics of the args parameter need to be defined per implementation, i.e. only IndicationTestDriver will need, and know what to do with args['handlers']. But yes, it is probably little bit confusing; I'll have to think of better way how to describe this.
I have renamed `_run` to `_do_run` (inspired by `do_GET` from Python's standard SimpleHTTPServer, although preserving the underscore).
Also `args` to `argset`, since the former can be easily confused with convention as in `*args`, and the later can be easily pluralised (e.g. `for argset in argsets`).
Note that since commits have been split, fixed version will appear in a new review request; this request will be closed.
- Alois
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/1740/#review2639 -----------------------------------------------------------
On May 14, 2014, 4:26 p.m., Alois Mahdal wrote:
This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/1740/
(Updated May 14, 2014, 4:26 p.m.)
Review request for OpenLMI Developers.
Repository: openlmi-providers
Description
Add indication testing framework
Set of classes and methods to help with event stream oritnted tests.
Diffs
src/account/test/common.py 95abd381089506c3f9b4d6632f2303da322fa585 src/python/lmi/test/ind.py PRE-CREATION src/python/lmi/test/util.py 699507c2340e9a50ff22ce08b67fe3e0a3f9eef8
Diff: http://reviewboard-openlmi.rhcloud.com/r/1740/diff/
Testing
Thanks,
Alois Mahdal
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/1740/#review2640 -----------------------------------------------------------
And one more issue: please split additions to lmi.test and account/tests into two individual commits.
src/python/lmi/test/ind.py http://reviewboard-openlmi.rhcloud.com/r/1740/#comment1458
Please, add some docstring.
Add some comments on attributes/methods that need to be overriden by subclasses (options, default_args).
I would turn *options* property into a virtual method (get_driver_options()) and provide some sane defaults in default implementation. The same for default_args.
- Michal Minar
On May 14, 2014, 4:26 p.m., Alois Mahdal wrote:
This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/1740/
(Updated May 14, 2014, 4:26 p.m.)
Review request for OpenLMI Developers.
Repository: openlmi-providers
Description
Add indication testing framework
Set of classes and methods to help with event stream oritnted tests.
Diffs
src/account/test/common.py 95abd381089506c3f9b4d6632f2303da322fa585 src/python/lmi/test/ind.py PRE-CREATION src/python/lmi/test/util.py 699507c2340e9a50ff22ce08b67fe3e0a3f9eef8
Diff: http://reviewboard-openlmi.rhcloud.com/r/1740/diff/
Testing
Thanks,
Alois Mahdal
On May 19, 2014, 10:03 a.m., Michal Minar wrote:
src/python/lmi/test/ind.py, line 31 http://reviewboard-openlmi.rhcloud.com/r/1740/diff/1/?file=9884#file9884line31
Please, add some docstring. Add some comments on attributes/methods that need to be overriden by subclasses (options, default_args). I would turn *options* property into a virtual method (get_driver_options()) and provide some sane defaults in default implementation. The same for default_args.
I've revamped all the docstrings, and added them where they were missing. Hopefully I did not miss anything.
Also default options are now loaded in the test case; and used when driver is sinstantiated. default_args are not recommended to use other than in very specific cases (this is now explained in dcstrings), so there is no need to load them from environment.
Note that since commits have been split, fixed version will appear in a new review request; this request will be closed.
- Alois
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/1740/#review2640 -----------------------------------------------------------
On May 14, 2014, 4:26 p.m., Alois Mahdal wrote:
This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/1740/
(Updated May 14, 2014, 4:26 p.m.)
Review request for OpenLMI Developers.
Repository: openlmi-providers
Description
Add indication testing framework
Set of classes and methods to help with event stream oritnted tests.
Diffs
src/account/test/common.py 95abd381089506c3f9b4d6632f2303da322fa585 src/python/lmi/test/ind.py PRE-CREATION src/python/lmi/test/util.py 699507c2340e9a50ff22ce08b67fe3e0a3f9eef8
Diff: http://reviewboard-openlmi.rhcloud.com/r/1740/diff/
Testing
Thanks,
Alois Mahdal
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-openlmi.rhcloud.com/r/1740/ -----------------------------------------------------------
(Updated May 21, 2014, 3:23 p.m.)
Status ------
This change has been discarded.
Review request for OpenLMI Developers.
Repository: openlmi-providers
Description -------
Add indication testing framework
Set of classes and methods to help with event stream oritnted tests.
Diffs -----
src/account/test/common.py 95abd381089506c3f9b4d6632f2303da322fa585 src/python/lmi/test/ind.py PRE-CREATION src/python/lmi/test/util.py 699507c2340e9a50ff22ce08b67fe3e0a3f9eef8
Diff: http://reviewboard-openlmi.rhcloud.com/r/1740/diff/
Testing -------
Thanks,
Alois Mahdal
openlmi-reviews@lists.fedorahosted.org