Refactor Telemetry value system to support AsDict()

Refactor Telemetry value system to support AsDict()

This patch adds an AsDict call from Telemetry Values and implements it for
the various Value subtypes. The AsDict call will be used to produce the
intermediate representation of page test results for JSON output.

Patch set 1 pulls in the value system changes from 378633003

BUG=375541
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283349

[eakuefner, 2014-07-09 23:11:16]

This patch provides AsDict() for the Telemetry value system, including
implementations and tests for scalars, strings, lists of either, and histograms.
It depends on the pages-have-ids patch that is currently landing.

I had an idea for GetJSONTypeName() that would reduce the amount of duplication
and overhead but potentially be more fragile or harder to understand, so I did
not implement it but wanted to ask for feedback:

What if we made GetJSONTypeName a @classmethod of Value and had it return
cls.__module__? This would be equivalent in practice to writing it out
individually for each value, though it does rely on reflection and file naming.

[tonyg, 2014-07-10 15:15:04]

Some comments and questions to get started on.

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/__init__.py (right):

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:164: pg_id = self.page.id if
self.page else -1
s/pg_id/page_id/

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:167: 'name': self.name,
Is it intentional that we no longer send the URL in all cases?

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:168: 'page': pg_id,
Or maybe we should consider omitting this field if !self.page?

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:171: 'important': self.important
Is
https://docs.google.com/a/google.com/document/d/1HZv6v5hFPpey5N2DsjQ0KEIvR8B-...
still up to date? If not, I think it should be before we more further into
implementation.

For instance, we had planned to drop the term 'important' because that term
doesn't really mean anything and instead switch to 'alert': 'regressions'.

We had also planned to have something indicating whether bigger or smaller is
better. Right now we base that decision off of units specified in
tools/perf/unit-info.json. And for things like 'count' or 'score' or 'percent',
it isn't the case that all tests would share the same definition of whether
bigger or smaller is better. We wanted to push that back into Telemetry at the
point the Measurement adds the results.

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/as_dict_unittest.py (right):

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/as_dict_unittest.py:24: scalar.ScalarValue(None,
'x', 'unit', False, 3).AsDict()
Shouldn't we assertEqual on the return value?

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/histogram.py (right):

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/histogram.py:22: def ToJSONString(self):
This method should go away in favor of the code below doing something like
json.dumps(histogram.AsDict()).

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/histogram.py:68: def ToJSONString(self):
ditto

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/histogram.py:82: self.ToJSONString())
This method should be able to pass around dicts instead of json strings now.

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/histogram_unittest.py (right):

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/histogram_unittest.py:53:
self.assertEquals(type(d['values'][0]), dict)
Seems like we should assert on the whole json instead of the type.

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/list_of_scalar_values_unittest.py (right):

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/list_of_scalar_values_unittest.py:86:
self.assertEquals(v0t, list)
again, think we should test more here.

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/list_of_string_values.py (right):

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/list_of_string_values.py:55: return
'list_of_scalar_values'
list_of_string_values

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/string.py (right):

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/string.py:51: def AsDict(self):
All of these ended up having the same implementation. I wonder whether that
suggests something should be pushed up to a base class.

[eakuefner, 2014-07-10 16:06:09]

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/__init__.py (right):

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:167: 'name': self.name,
On 2014/07/10 15:15:03, tonyg wrote:
> Is it intentional that we no longer send the URL in all cases?

Yes: a page (and eventually a page run, but that's another CL) will be uniquely
identifiable by its ID and a JSON results object will ship with a map of IDs to
pages. Thus pages will also implement AsDict(), though I wasn't sure whether to
make that part of this CL.

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:168: 'page': pg_id,
On 2014/07/10 15:15:03, tonyg wrote:
> Or maybe we should consider omitting this field if !self.page?

Yes also, that's a great idea.

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:171: 'important': self.important
On 2014/07/10 15:15:03, tonyg wrote:
> Is
>
https://docs.google.com/a/google.com/document/d/1HZv6v5hFPpey5N2DsjQ0KEIvR8B-...
> still up to date? If not, I think it should be before we more further into
> implementation.
> 
> For instance, we had planned to drop the term 'important' because that term
> doesn't really mean anything and instead switch to 'alert': 'regressions'.
> 
> We had also planned to have something indicating whether bigger or smaller is
> better. Right now we base that decision off of units specified in
> tools/perf/unit-info.json. And for things like 'count' or 'score' or
'percent',
> it isn't the case that all tests would share the same definition of whether
> bigger or smaller is better. We wanted to push that back into Telemetry at the
> point the Measurement adds the results.

It's not quite up to date since I think the shape of the project now is such
that our end goal is still similar but the way we are going to get there has
changed. I'll update it before pushing more changes to this CL to clarify what
the view is.

In terms of reworking the notion of "importance" for values, my understanding is
that that will involve a fairly broad effort that's probably overall outside of
the scope of this CL. The schema here matches the data that we are currently
able to extract from values. I think it might be okay to get rid of the
important field so as not to give the impression that it matters or should be
expected in output from this backend.

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/string.py (right):

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/string.py:51: def AsDict(self):
On 2014/07/10 15:15:04, tonyg wrote:
> All of these ended up having the same implementation. I wonder whether that
> suggests something should be pushed up to a base class.

I agree that this is a good idea. We will probably need a method in each
subclass that tells AsDict what the "SerializableValue" or something like this
is. The issue here is that for example ScalarValues have a "value" field, where
ListOfScalarValues have a "values" field, and lacking some more granular
abstraction over "singular" or "plural" values or what have you, it wouldn't be
possible for Value to simply determine which field to look for and convert to
dict.

As mentioned in my comment yesterday there's also a solution for moving
GetJSONTypeName up, but I can't evaluate by myself whether that's a good idea.

[nduca, 2014-07-10 17:58:48]

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/__init__.py (right):

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:153: def GetJSONTypeName(self):
you have this as a classmethod in the subclasses.

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:157: def AsDict(self):
where's your unit test coverage of this part of things? i think there's a
value_unittest.py

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:168: 'page': pg_id,
the field name should be page_id

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/as_dict_unittest.py (right):

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/as_dict_unittest.py:10: class
TestBase(unittest.TestCase):
please move unit tests for as dict to each of the individual per-class unittest
files. it is non normative to have a standalone test file like this.

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/histogram.py (right):

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/histogram.py:22: def ToJSONString(self):
be really careful in doing this, to the point that i'd almost suggest it as a
followup patch. this is used  by buildbot output and its test code has
assumptions all over the place about the json string being "stable". Maybe use
dumps(..,sort_keys=True) and it'll work? Anyway, i think this is possibly a rats
nest.

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/histogram_unittest.py (right):

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/histogram_unittest.py:53:
self.assertEquals(type(d['values'][0]), dict)
yeah this is undertested. the buckets returned should be tested more thoroughly.

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/scalar_unittest.py (right):

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/scalar_unittest.py:66:
self.assertEquals(v.AsDict()['values'], v.value)
a temp would be good here
d = v.AsDict()

also, don't do v.value --- do 42. you're not testing the v.value property,
you're testing that its 42.

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/string.py (right):

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/string.py:51: def AsDict(self):
i think this duplication is okay. i'd suggest leaving it as is... its
duplication, yes, but its really easy to read.

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/string_unittest.py (right):

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/string_unittest.py:66:
self.assertEquals(v.AsDict()['values'], v.value)
same comment as before --- 'foo', not v.value. and temp var

[nduca, 2014-07-10 17:58:55]

lets do a separate patch to rename the important field throughout the
architecture to the name suggested by tonyg, and add it at that time to the
asdict.

lets also file a bug for introducing bigger_is_better//improvement direction
field.

> Thus pages will also implement AsDict(), though I wasn't sure whether to
> make that part of this CL.

separate cl but please get on that :)

> > All of these ended up having the same implementation. I wonder whether that
> > suggests something should be pushed up to a base class.
I think a bit of duplication 
> 
> I agree that this is a good idea. We will probably need a method in each
> subclass that tells AsDict what the "SerializableValue" or something like this
> is. The issue here is that for example ScalarValues have a "value" field,
where
> ListOfScalarValues have a "values" field, and lacking some more granular
> abstraction over "singular" or "plural" values or what have you, it wouldn't
be
> possible for Value to simply determine which field to look for and convert to
> dict.
> 
> As mentioned in my comment yesterday there's also a solution for moving
> GetJSONTypeName up, but I can't evaluate by myself whether that's a good idea.

[eakuefner, 2014-07-10 18:36:23]

On 2014/07/10 17:58:55, nduca wrote:
> lets do a separate patch to rename the important field throughout the
> architecture to the name suggested by tonyg, and add it at that time to the
> asdict.
> 
> lets also file a bug for introducing bigger_is_better//improvement direction
> field.
Spoke to tonyg@ in person and will file these bugs.


> > Thus pages will also implement AsDict(), though I wasn't sure whether to
> > make that part of this CL.
> separate cl but please get on that :)
Ack. I plan to get all this auxiliary stuff hammered out by the end of the week.
 
> > > All of these ended up having the same implementation. I wonder whether
that
> > > suggests something should be pushed up to a base class.
> I think a bit of duplication 
The rest of your thought didn't transmit. I assume it's consistent with what you
wrote inline, though, which to me basically said that a little duplication
enhances readability in this case.

[eakuefner, 2014-07-10 18:37:24]

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/__init__.py (right):

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:157: def AsDict(self):
On 2014/07/10 17:58:47, nduca wrote:
> where's your unit test coverage of this part of things? i think there's a
> value_unittest.py

There is, but it's called value_unittest_.py, so I think it's disabled for
reasons of which I'm not aware, which is why I originally created the one-off
test file for this code.

Should I just rename it, dump my tests there, and see what happens?

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/histogram.py (right):

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/histogram.py:22: def ToJSONString(self):
On 2014/07/10 17:58:48, nduca wrote:
> be really careful in doing this, to the point that i'd almost suggest it as a
> followup patch. this is used  by buildbot output and its test code has
> assumptions all over the place about the json string being "stable". Maybe use
> dumps(..,sort_keys=True) and it'll work? Anyway, i think this is possibly a
rats
> nest.

This was my feeling exactly -- there are comments in the code about having to do
histogram-to-JSON manually since there are stability issues, so I was going to
leave it alone for now.

[nduca, 2014-07-11 20:41:10]

> There is, but it's called value_unittest_.py, so I think it's disabled for
> reasons of which I'm not aware, which is why I originally created the one-off
> test file for this code.
Then enable the test and disable individual tests. Or make a second test case
subclass in the same module. Dont just go moving things.

[eakuefner, 2014-07-12 00:32:12]

Two notes up front: 

1. There is some overlap in the value subclass tests because we test a whole
dict for each value subtype. I think that even though this ends up testing
whether 'name', 'important', and 'unit' carry through multiple times, it's worth
the benefits of being able to compare a whole concrete input to a whole concrete
output and being able to test that type and value are preserved for each of the
types.

2. I'm going to leave renaming/replacing the important field to another CL if
that's alright.

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/__init__.py (right):

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:153: def GetJSONTypeName(self):
On 2014/07/10 17:58:47, nduca wrote:
> you have this as a classmethod in the subclasses.

Done.

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:157: def AsDict(self):
On 2014/07/10 17:58:47, nduca wrote:
> where's your unit test coverage of this part of things? i think there's a
> value_unittest.py

Done.

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:164: pg_id = self.page.id if
self.page else -1
On 2014/07/10 15:15:03, tonyg wrote:
> s/pg_id/page_id/

Done.

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:168: 'page': pg_id,
On 2014/07/10 17:58:47, nduca wrote:
> the field name should be page_id

Done.

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/as_dict_unittest.py (right):

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/as_dict_unittest.py:10: class
TestBase(unittest.TestCase):
On 2014/07/10 17:58:48, nduca wrote:
> please move unit tests for as dict to each of the individual per-class
unittest
> files. it is non normative to have a standalone test file like this.

Done.

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/as_dict_unittest.py:24: scalar.ScalarValue(None,
'x', 'unit', False, 3).AsDict()
On 2014/07/10 15:15:03, tonyg wrote:
> Shouldn't we assertEqual on the return value?

Done.

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/histogram_unittest.py (right):

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/histogram_unittest.py:53:
self.assertEquals(type(d['values'][0]), dict)
On 2014/07/10 17:58:48, nduca wrote:
> yeah this is undertested. the buckets returned should be tested more
thoroughly.

Done.

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/list_of_scalar_values_unittest.py (right):

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/list_of_scalar_values_unittest.py:86:
self.assertEquals(v0t, list)
On 2014/07/10 15:15:04, tonyg wrote:
> again, think we should test more here.

Done.

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/list_of_string_values.py (right):

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/list_of_string_values.py:55: return
'list_of_scalar_values'
On 2014/07/10 15:15:04, tonyg wrote:
> list_of_string_values

Done.

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/scalar_unittest.py (right):

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/scalar_unittest.py:66:
self.assertEquals(v.AsDict()['values'], v.value)
On 2014/07/10 17:58:48, nduca wrote:
> a temp would be good here
> d = v.AsDict()
> 
> also, don't do v.value --- do 42. you're not testing the v.value property,
> you're testing that its 42.

Done.

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/string_unittest.py (right):

https://codereview.chromium.org/381723002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/string_unittest.py:66:
self.assertEquals(v.AsDict()['values'], v.value)
On 2014/07/10 17:58:48, nduca wrote:
> same comment as before --- 'foo', not v.value. and temp var

Done.

[nduca, 2014-07-14 21:49:05]

looking almost good more feedback

https://codereview.chromium.org/381723002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/__init__.py (right):

https://codereview.chromium.org/381723002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:166: 'important': self.important
remove this line

https://codereview.chromium.org/381723002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/histogram.py (right):

https://codereview.chromium.org/381723002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/histogram.py:93: d['values'] = [b.AsDict() for b
in self.buckets]
d['buckets']

https://codereview.chromium.org/381723002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/histogram_unittest.py (right):

https://codereview.chromium.org/381723002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/histogram_unittest.py:42: self.assertEquals(
you need a test of the bucket.asvalue()

https://codereview.chromium.org/381723002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/histogram_unittest.py:54: 
to eliminate overtesting, 

on Value:

def AsDict(self):
  ...

def _AsDictImpl(self):
  real as dict goes here

def AsDictWithoutBaseClassEntries():
  full_dict = self.AsDict()
  basic_dict_keys = set(self._AsDictImpl().keys())
  
  return dict([(k, v) for (k, v) in full_dict.AsDict().IterValues() if k not in
basic_dict_keys])

https://codereview.chromium.org/381723002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/histogram_unittest.py:55: self.assertEquals(d, {
self.assertEquals(['buckets'], d.keys())
self.assertTrue(instanceof(d['buckets'], list))
self.assertEquals(len(d['buckets'], 1)

https://codereview.chromium.org/381723002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/scalar.py (right):

https://codereview.chromium.org/381723002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/scalar.py:55: d['values'] = self.value
please say value...

[eakuefner, 2014-07-14 23:55:19]

https://codereview.chromium.org/381723002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/__init__.py (right):

https://codereview.chromium.org/381723002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:166: 'important': self.important
On 2014/07/14 21:49:05, nduca wrote:
> remove this line

Done.

https://codereview.chromium.org/381723002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/histogram.py (right):

https://codereview.chromium.org/381723002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/histogram.py:93: d['values'] = [b.AsDict() for b
in self.buckets]
On 2014/07/14 21:49:05, nduca wrote:
> d['buckets']

Done.

https://codereview.chromium.org/381723002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/histogram_unittest.py (right):

https://codereview.chromium.org/381723002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/histogram_unittest.py:42: self.assertEquals(
On 2014/07/14 21:49:05, nduca wrote:
> you need a test of the bucket.asvalue()

Done.

https://codereview.chromium.org/381723002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/histogram_unittest.py:54: 
On 2014/07/14 21:49:05, nduca wrote:
> to eliminate overtesting, 
> 
> on Value:
> 
> def AsDict(self):
>   ...
> 
> def _AsDictImpl(self):
>   real as dict goes here
> 
> def AsDictWithoutBaseClassEntries():
>   full_dict = self.AsDict()
>   basic_dict_keys = set(self._AsDictImpl().keys())
>   
>   return dict([(k, v) for (k, v) in full_dict.AsDict().IterValues() if k not
in
> basic_dict_keys])

Done.

https://codereview.chromium.org/381723002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/histogram_unittest.py:55: self.assertEquals(d, {
On 2014/07/14 21:49:05, nduca wrote:
> self.assertEquals(['buckets'], d.keys())
> self.assertTrue(instanceof(d['buckets'], list))
> self.assertEquals(len(d['buckets'], 1)

Done.

https://codereview.chromium.org/381723002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/scalar.py (right):

https://codereview.chromium.org/381723002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/scalar.py:55: d['values'] = self.value
On 2014/07/14 21:49:05, nduca wrote:
> please say value...

Done.

[nduca, 2014-07-15 01:07:09]

lgtm with nits

https://codereview.chromium.org/381723002/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/__init__.py (right):

https://codereview.chromium.org/381723002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:178: basic_dict_keys =
set(self._AsDictImpl().keys())
basic -> base

https://codereview.chromium.org/381723002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:180: return dict([(k, v) for (k, v)
in full_dict.iteritems()
put a comment above this that explains what this is doing

https://codereview.chromium.org/381723002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:181: if k not in basic_dict_keys])
first letter of this line should be below the k in (k, v)

e.g.
dict([(k, v) ...
       if k not in ...])

https://codereview.chromium.org/381723002/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/value_unittest.py (right):

https://codereview.chromium.org/381723002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/value_unittest.py:50: class
ValueForAsDictTest(value.Value):
better to make ValueForTest() in this file that stubs out everything, then have
ValueForAsDictTest and ValueForMergingTest subclass that

https://codereview.chromium.org/381723002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/value_unittest.py:58: @classmethod
1 space betwen methods

https://codereview.chromium.org/381723002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/value_unittest.py:103: def
testAsDictIsAbstractInValue(self):
you dont need this test

[eakuefner, 2014-07-15 04:15:16]

https://codereview.chromium.org/381723002/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/__init__.py (right):

https://codereview.chromium.org/381723002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:178: basic_dict_keys =
set(self._AsDictImpl().keys())
On 2014/07/15 01:07:09, nduca wrote:
> basic -> base

Done.

https://codereview.chromium.org/381723002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:180: return dict([(k, v) for (k, v)
in full_dict.iteritems()
On 2014/07/15 01:07:08, nduca wrote:
> put a comment above this that explains what this is doing

Done.

https://codereview.chromium.org/381723002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:181: if k not in basic_dict_keys])
On 2014/07/15 01:07:08, nduca wrote:
> first letter of this line should be below the k in (k, v)
> 
> e.g.
> dict([(k, v) ...
>        if k not in ...])

Done.

https://codereview.chromium.org/381723002/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/value_unittest.py (right):

https://codereview.chromium.org/381723002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/value_unittest.py:50: class
ValueForAsDictTest(value.Value):
On 2014/07/15 01:07:09, nduca wrote:
> better to make ValueForTest() in this file that stubs out everything, then
have
> ValueForAsDictTest and ValueForMergingTest subclass that

Done.

https://codereview.chromium.org/381723002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/value_unittest.py:58: @classmethod
On 2014/07/15 01:07:09, nduca wrote:
> 1 space betwen methods

Done.

https://codereview.chromium.org/381723002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/value_unittest.py:103: def
testAsDictIsAbstractInValue(self):
On 2014/07/15 01:07:09, nduca wrote:
> you dont need this test

Done.

[eakuefner, 2014-07-15 04:15:30]

The CQ bit was checked by eakuefner@chromium.org

[commit-bot: I haz the power, 2014-07-15 04:16:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eakuefner@chromium.org/381723002/100001

[commit-bot: I haz the power, 2014-07-15 20:38:17]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-07-16 05:07:25]

Change committed as 283349