|
|
DescriptionEnable tests to have host-specific parameters. Remove a bunch of todos where we disabled tests because they would not work everywhere. Now we selectively enable and set constraints on BVTs.
Patch Set 1 #Patch Set 2 : Adding host attributes to enable parameterizing tests based on host #Patch Set 3 : log all host attributes for debugging #
Total comments: 14
Patch Set 4 : respond to review #Patch Set 5 : fix comments #
Total comments: 4
Patch Set 6 : clean up #
Total comments: 6
Patch Set 7 : minor #
Messages
Total messages: 16 (0 generated)
lgtm
Please take a look at comments from Sameer. http://codereview.chromium.org/1712007 I think he want it kept in BVT. On Tue, Apr 27, 2010 at 12:27 PM, <seano@chromium.org> wrote: > lgtm > > > http://codereview.chromium.org/1780007/show > -- Eric Li 李咏竹 Google Kirkland
Agreed - I want it kept in the BVT as well, longer term. The problem is that right now, the reference boards we're going to be installing in the farm will hang if you run this test without a hardware modification. So for now I think we want to remove it and then add it back, as in the TODO. On Tue, Apr 27, 2010 at 8:08 PM, Eric Li(李咏竹) <ericli@chromium.org> wrote: > Please take a look at comments from Sameer. > http://codereview.chromium.org/1712007 > I think he want it kept in BVT. > > On Tue, Apr 27, 2010 at 12:27 PM, <seano@chromium.org> wrote: >> >> lgtm >> >> http://codereview.chromium.org/1780007/show > > > > -- > Eric Li > 李咏竹 > Google Kirkland > > >
Ken, If you had reached agreement with Sameer, LTGM. Sameer, Could you also LGTM this? On Wed, Apr 28, 2010 at 9:29 AM, Ken Mixter <kmixter@chromium.org> wrote: > Agreed - I want it kept in the BVT as well, longer term. The problem > is that right now, the reference boards we're going to be installing > in the farm will hang if you run this test without a hardware > modification. So for now I think we want to remove it and then add it > back, as in the TODO. > > On Tue, Apr 27, 2010 at 8:08 PM, Eric Li(李咏竹) <ericli@chromium.org> wrote: > > Please take a look at comments from Sameer. > > http://codereview.chromium.org/1712007 > > I think he want it kept in BVT. > > > > On Tue, Apr 27, 2010 at 12:27 PM, <seano@chromium.org> wrote: > >> > >> lgtm > >> > >> http://codereview.chromium.org/1780007/show > > > > > > > > -- > > Eric Li > > 李咏竹 > > Google Kirkland > > > > > > > -- Eric Li 李咏竹 Google Kirkland
On 2010/04/28 16:37:58, ericli wrote: > Ken, > > If you had reached agreement with Sameer, LTGM. > > Sameer, > > Could you also LGTM this? Had a chat with Ken and suggested using autotest labels to exclude systems where this test should not be run on. > > On Wed, Apr 28, 2010 at 9:29 AM, Ken Mixter <mailto:kmixter@chromium.org> wrote: > > > Agreed - I want it kept in the BVT as well, longer term. The problem > > is that right now, the reference boards we're going to be installing > > in the farm will hang if you run this test without a hardware > > modification. So for now I think we want to remove it and then add it > > back, as in the TODO. > > > > On Tue, Apr 27, 2010 at 8:08 PM, Eric Li(李咏竹) <mailto:ericli@chromium.org> wrote: > > > Please take a look at comments from Sameer. > > > http://codereview.chromium.org/1712007 > > > I think he want it kept in BVT. > > > > > > On Tue, Apr 27, 2010 at 12:27 PM, <mailto:seano@chromium.org> wrote: > > >> > > >> lgtm > > >> > > >> http://codereview.chromium.org/1780007/show > > > > > > > > > > > > -- > > > Eric Li > > > 李咏竹 > > > Google Kirkland > > > > > > > > > > > > > > > -- > Eric Li > 李咏竹 > Google Kirkland >
Complete redid this. PTAL.
Please help me to create all host attributes labels in the server. I will take care of assigning labels to hosts. Other suggestion: rename site_host_attributes_config.py to something like private_host_attributes_config.py or similar. Since the site_* name convention has some different suggestion here. I am also suggesting to rename site_* into cros_* in general, but that is not in the scope of this CL. http://codereview.chromium.org/1780007/diff/10001/11002 File server/site_host_attributes.py (right): http://codereview.chromium.org/1780007/diff/10001/11002#newcode39 server/site_host_attributes.py:39: hardcoded_host_attributes = {} Could you try something like this? hardcoded_hard_attr = utils.import_site_class(__file__, "autotest_lib.server.site_host_attributes_config", "hardcoded_hard_attr", {}) This logic is everywhere inside autotest code, kind of standard logic. http://codereview.chromium.org/1780007/diff/10001/11002#newcode83 server/site_host_attributes.py:83: self._attributes[splitnames[0]] = value so if an attribute has no comma ',' in it, it will end up as self._attributes['foo']=[] This is not obvious, wonder if you could document it. http://codereview.chromium.org/1780007/diff/10001/11002#newcode100 server/site_host_attributes.py:100: return self._attributes[attribute] return self._attributes.get(attribute) You dont need the try catch block here. Same effect.
It's nice to see that support for this is coming in place... A few comments, mostly regarding documentation. http://codereview.chromium.org/1780007/diff/10001/11002 File server/site_host_attributes.py (right): http://codereview.chromium.org/1780007/diff/10001/11002#newcode16 server/site_host_attributes.py:16: # host_attributes.get('drive_kind') => 'ssd,1' Where does the ",1" come from? http://codereview.chromium.org/1780007/diff/10001/11002#newcode83 server/site_host_attributes.py:83: self._attributes[splitnames[0]] = value Do you need special processing for has_* attributes here so that the "has" method below works OK? In general, this has_ thing is a bit confusing. Maybe always strip the has_ from the attribute name, if possible. Then calling the "has" method would simply mean check the attribute is set to true. http://codereview.chromium.org/1780007/diff/10001/11002#newcode86 server/site_host_attributes.py:86: def set_defaults(self, default_attributes): This method needs better documentation -- it seems that it simply updates the attributes with "default_attributes" for attributes that are not already present. Who's expected to invoke this method? http://codereview.chromium.org/1780007/diff/10001/11002#newcode107 server/site_host_attributes.py:107: Retrieve an attribute that starts with "has_" and ignore Change doc string (or the code) -- it seems that the code below takes into account that attribute value.
PTAL http://codereview.chromium.org/1780007/diff/10001/11002 File server/site_host_attributes.py (right): http://codereview.chromium.org/1780007/diff/10001/11002#newcode16 server/site_host_attributes.py:16: # host_attributes.get('drive_kind') => 'ssd,1' On 2010/04/29 19:39:06, petkov wrote: > Where does the ",1" come from? > Yeah these comments were too sparse and inconsistent. Should be better now. http://codereview.chromium.org/1780007/diff/10001/11002#newcode39 server/site_host_attributes.py:39: hardcoded_host_attributes = {} On 2010/04/29 03:25:09, ericli wrote: > Could you try something like this? > > hardcoded_hard_attr = utils.import_site_class(__file__, > "autotest_lib.server.site_host_attributes_config", "hardcoded_hard_attr", {}) > > This logic is everywhere inside autotest code, kind of standard logic. > Done. http://codereview.chromium.org/1780007/diff/10001/11002#newcode83 server/site_host_attributes.py:83: self._attributes[splitnames[0]] = value On 2010/04/29 19:39:06, petkov wrote: > Do you need special processing for has_* attributes here so that the "has" > method below works OK? > > In general, this has_ thing is a bit confusing. Maybe always strip the has_ from > the attribute name, if possible. Then calling the "has" method would simply mean > check the attribute is set to true. Rather than document and explain away the confusion, I've tried to simplify things. There are no implicit 'has_' prefixes, and the has function was renamed to "bool" returns a boolean. http://codereview.chromium.org/1780007/diff/10001/11002#newcode83 server/site_host_attributes.py:83: self._attributes[splitnames[0]] = value On 2010/04/29 03:25:09, ericli wrote: > so if an attribute has no comma ',' in it, it will end up as > self._attributes['foo']=[] > This is not obvious, wonder if you could document it. > It will map to ','.join([]) which is ''. I've documented that. http://codereview.chromium.org/1780007/diff/10001/11002#newcode86 server/site_host_attributes.py:86: def set_defaults(self, default_attributes): On 2010/04/29 19:39:06, petkov wrote: > This method needs better documentation -- it seems that it simply updates the > attributes with "default_attributes" for attributes that are not already > present. Who's expected to invoke this method? > No one, it was dead code and is now removed. http://codereview.chromium.org/1780007/diff/10001/11002#newcode100 server/site_host_attributes.py:100: return self._attributes[attribute] On 2010/04/29 03:25:09, ericli wrote: > return self._attributes.get(attribute) > > You dont need the try catch block here. Same effect. Thanks. Done. http://codereview.chromium.org/1780007/diff/10001/11002#newcode107 server/site_host_attributes.py:107: Retrieve an attribute that starts with "has_" and ignore On 2010/04/29 19:39:06, petkov wrote: > Change doc string (or the code) -- it seems that the code below takes into > account that attribute value. > Done.
Simpler and better... One concern about the "bool" naming and a nit about sorting, LGTM otherwise. http://codereview.chromium.org/1780007/diff/18001/3003 File server/site_host_attributes.py (right): http://codereview.chromium.org/1780007/diff/18001/3003#newcode68 server/site_host_attributes.py:68: 'has_resume_bug,False' Keep this sorted? http://codereview.chromium.org/1780007/diff/18001/3003#newcode89 server/site_host_attributes.py:89: for key in self._attributes: You could do for key, val in self._attributes.items(): Then you don't need the extra lookup in the logging below. http://codereview.chromium.org/1780007/diff/18001/3003#newcode90 server/site_host_attributes.py:90: logging.info('Host attribute: %s => %s' % You could use , rather than % http://codereview.chromium.org/1780007/diff/18001/3003#newcode108 server/site_host_attributes.py:108: def bool(self, attribute): bool() is a built-in function and has a slightly different meaning -- it returns True/False for a wider range of input strings. Maybe rename to flag()?
PTAL I'd like to submit this today if possible. Eric - we can add labels, but without labels this will only have the effect of running a few tests that should not be run on eeePC 1008's.
PTAL I'd like to submit this today if possible. Eric - we can add labels, but without labels this will only have the effect of running a few tests that should not be run.
LGTM. On 2010/04/30 21:04:59, kmixter1 wrote: > PTAL > > I'd like to submit this today if possible. Eric - we can add labels, but > without labels this will only have the effect of running a few tests that should > not be run.
A couple of suggestions, LGTM otherwise. http://codereview.chromium.org/1780007/diff/20001/21002 File server/site_host_attributes.py (right): http://codereview.chromium.org/1780007/diff/20001/21002#newcode15 server/site_host_attributes.py:15: # 'illegal_string,1,2,3' I'd remove this one from the example -- or add a comment that this is not valid. http://codereview.chromium.org/1780007/diff/20001/21002#newcode87 server/site_host_attributes.py:87: logging.info('Host attribute: %s => %s' % I'd do logging.info('Host attribute: %s => %s', key, value) http://codereview.chromium.org/1780007/diff/20001/21002#newcode104 server/site_host_attributes.py:104: self._attributes[splitnames[0]] = value You could get rid of _attributes and just use setattr(self, splitnames[0], value) Then you don't need __getattr__
pushing. http://codereview.chromium.org/1780007/diff/20001/21002 File server/site_host_attributes.py (right): http://codereview.chromium.org/1780007/diff/20001/21002#newcode15 server/site_host_attributes.py:15: # 'illegal_string,1,2,3' On 2010/04/30 21:42:36, petkov wrote: > I'd remove this one from the example -- or add a comment that this is not valid. > Done. http://codereview.chromium.org/1780007/diff/20001/21002#newcode87 server/site_host_attributes.py:87: logging.info('Host attribute: %s => %s' % On 2010/04/30 21:42:36, petkov wrote: > I'd do > > logging.info('Host attribute: %s => %s', key, value) > Done. http://codereview.chromium.org/1780007/diff/20001/21002#newcode104 server/site_host_attributes.py:104: self._attributes[splitnames[0]] = value On 2010/04/30 21:42:36, petkov wrote: > You could get rid of _attributes and just use > > setattr(self, splitnames[0], value) > > Then you don't need __getattr__ > Done. |