-

Added support for substrate parsing in TestCfg

https://code.launchpad.net/~marcoceppi/charm-tools/test-cfg/+merge/200921

(do not edit description out of merge proposal)

[Marco Ceppi for realsies, 2014-01-08 22:51:55]

Please take a look.

[Marco Ceppi for realsies, 2014-01-08 22:56:44]

Please take a look.

[benjamin.saller, 2014-01-09 07:52:40]

I like what you started here and put a few comments inline.

I took a stab at extending your work in another branch. 

https://codereview.appspot.com/49670043/

Look at this when you have time. I think its more testable than what you have
there (and more tested infact). If you like the model we can look at cleaning it
up a little more and merging. Let me know what you think.

https://codereview.appspot.com/49160046/diff/20001/charmtools/test.py
File charmtools/test.py (right):

https://codereview.appspot.com/49160046/diff/20001/charmtools/test.py#newcode411
charmtools/test.py:411: setattr(self, key, val)
While this is a common pattern, its generally better to whitelist vars that can
be set in this way. Its possible now for the options to break the tests by
setting a name they sholdn't have on this class.

https://codereview.appspot.com/49160046/diff/20001/charmtools/test.py#newcode413
charmtools/test.py:413: self.substrates = {}
I think while we could validate the substrates its important to note that what
we produce from this parse is a set of rules, not a list of substrates. 

As we add substrates we want to automatically include those unless the charm
explicitly said it only tests on one substrate (which could happen for EC2
specific charms for example) or that it explicitly doesn't run on one (or more)
substrates

[Marco Ceppi for realsies, 2014-01-09 15:07:47]

I'll take a look at your proposal too, thanks for the review!

https://codereview.appspot.com/49160046/diff/20001/charmtools/test.py
File charmtools/test.py (right):

https://codereview.appspot.com/49160046/diff/20001/charmtools/test.py#newcode413
charmtools/test.py:413: self.substrates = {}
I agree, think of self.substrates as more self.substrate_rules where it as if
it's inclusive or not and the values for inclusive or not. It never has a
comprehensive list of _all_ substrates at any one time, it's more a parsing of
the substrate option provided in the test_config.yaml

On 2014/01/09 07:52:40, benjamin.saller wrote:
> I think while we could validate the substrates its important to note that what
> we produce from this parse is a set of rules, not a list of substrates. 
> 
> As we add substrates we want to automatically include those unless the charm
> explicitly said it only tests on one substrate (which could happen for EC2
> specific charms for example) or that it explicitly doesn't run on one (or
more)
> substrates