Telemetry Pages support AsDict()

Telemetry Pages support AsDict()

We will be using the AsDict() call to support JSON output of Telemetry results.
This CL adds the ability to output the important information associated with a
page to the Telemetry Page class.

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

[qyearsley, 2014-07-10 22:53:15]

On 2014/07/10 22:28:21, eakuefner wrote:

LGTM. The variable d could also be called page_dict or something but it's OK
since that variable is only in scope for the body of one small method.

[eakuefner, 2014-07-11 01:34:31]

The CQ bit was checked by eakuefner@chromium.org

[commit-bot: I haz the power, 2014-07-11 01:38:20]

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

[tonyg, 2014-07-11 02:01:37]

https://codereview.chromium.org/388653002/diff/1/tools/telemetry/telemetry/pa...
File tools/telemetry/telemetry/page/page.py (right):

https://codereview.chromium.org/388653002/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/page.py:60: def AsDict(self):
This seems really straightforward to unittest.

https://codereview.chromium.org/388653002/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/page.py:66: if self._name != '':
The style guide wants this to be 'if self._name:'.

http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?utm_source=Pyt...

[eakuefner, 2014-07-11 02:02:10]

The CQ bit was unchecked by eakuefner@chromium.org

[eakuefner, 2014-07-11 02:40:10]

The CQ bit was checked by eakuefner@chromium.org

[commit-bot: I haz the power, 2014-07-11 02:41:13]

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

[tonyg, 2014-07-11 03:08:31]

https://codereview.chromium.org/388653002/diff/10001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/page_unittest.py (right):

https://codereview.chromium.org/388653002/diff/10001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_unittest.py:127: # This should not throw an
exception
This comment isn't necessary

https://codereview.chromium.org/388653002/diff/10001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_unittest.py:128: self.assertEqual(d['name'],
'Google')
What about id? Should we just assertEqual on the whole dict?

[nduca, 2014-07-11 04:01:56]

The CQ bit was unchecked by nduca@chromium.org

[nduca, 2014-07-11 04:02:13]

please wait for lgtm from tonyg and i please

[nduca, 2014-07-11 04:03:04]

https://codereview.chromium.org/388653002/diff/10001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/page_unittest.py (right):

https://codereview.chromium.org/388653002/diff/10001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_unittest.py:125: p =
page.Page('http://google.com', name='Google')
you need two tests... one with name, one without

[tonyg, 2014-07-11 15:02:18]

On 2014/07/11 04:02:13, nduca wrote:
> please wait for lgtm from tonyg and i please

A lg2m from either is okay in this case. Just note that the presubmit failure
was telling you that qyearsley isn't an owner of this dir.

[eakuefner, 2014-07-11 16:12:12]

https://codereview.chromium.org/388653002/diff/10001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/page_unittest.py (right):

https://codereview.chromium.org/388653002/diff/10001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_unittest.py:125: p =
page.Page('http://google.com', name='Google')
On 2014/07/11 04:03:04, nduca wrote:
> you need two tests... one with name, one without

Done.

https://codereview.chromium.org/388653002/diff/10001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_unittest.py:127: # This should not throw an
exception
On 2014/07/11 03:08:31, tonyg wrote:
> This comment isn't necessary

Done.

https://codereview.chromium.org/388653002/diff/10001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_unittest.py:128: self.assertEqual(d['name'],
'Google')
On 2014/07/11 03:08:31, tonyg wrote:
> What about id? Should we just assertEqual on the whole dict?

Added a separate test to reason about ids in AsDicted Pages. The issue is that
the scheme for assigning ids to pages doesn't give us the ability to make any
claim here about the page's specific id might be.

[tonyg, 2014-07-11 17:18:24]

https://codereview.chromium.org/388653002/diff/30001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/page_unittest.py (right):

https://codereview.chromium.org/388653002/diff/30001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_unittest.py:124: def
testAsDictDoesNotInsertName(self):
Sorry, to belabor, but I think the goal of this unittest is to verify that the
entire dict is as expected. If someone adds a field, they should notice the test
and update it too.

It also shouldn't have to test other behavior in this module (like unique IDs,
which is already tested).

So to be explicit, here's what I'd recommend:

def testNamelessPageAsDict(self):
  nameless_dict = page.Page('http://example.com/').AsDict()
  self.assertIn('id', nameless_dict)
  del nameless_dict['id']
  self.assertEquals({
        'url': 'http://example.com/',
      }, nameless_dict)

def testNamedPageAsDict(self):
  named_dict = page.Page('http://example.com/', 'Example').AsDict()
  self.assertIn('id', named_dict)
  del named_dict['id']
  self.assertEquals({
        'url': 'http://example.com/',
        'name': 'Example'
      }, named_dict)

WDYT?

[eakuefner, 2014-07-11 17:32:41]

On 2014/07/11 17:18:24, tonyg wrote:
> WDYT?

nduca@ has given me feedback on a different CL (
https://codereview.chromium.org/378633003/diff/1/tools/telemetry/telemetry/re...
) that suggests that testing for equality of the dict is overtesting, and that
instead we should test on the fields of the dict. I'm happy to proceed in either
direction but as it stands it seems like these two viewpoints are contradictory
unless I'm missing something.

[tonyg, 2014-07-11 17:58:51]

On 2014/07/11 17:32:41, eakuefner wrote:
> On 2014/07/11 17:18:24, tonyg wrote:
> > WDYT?
> 
> nduca@ has given me feedback on a different CL (
>
https://codereview.chromium.org/378633003/diff/1/tools/telemetry/telemetry/re...
> ) that suggests that testing for equality of the dict is overtesting, and that
> instead we should test on the fields of the dict. I'm happy to proceed in
either
> direction but as it stands it seems like these two viewpoints are
contradictory
> unless I'm missing something.

I agree w/ him in the case you linked to, but think this is different. That's
more of a regression test where the method being tested depends on lots of other
methods throughout the code. When they get updated, we wouldn't want to have to
update that test every time.

The test in this CL is testing a pure, leaf function. So I think it's
appropriate to test the whole dict here.

I'll give an lgtm in advance of whichever way you decide to go. I'm not trying
to hold this CL up and normally wouldn't be so picky about a small issue. But
since you're just getting started on Telemetry, I'm trying to give guidance for
the purposes of future CLs.

[eakuefner-google (do not use), 2014-07-11 18:06:20]

On 2014/07/11 17:58:51, tonyg wrote:
> I'll give an lgtm in advance of whichever way you decide to go. I'm not trying
> to hold this CL up and normally wouldn't be so picky about a small issue. But
> since you're just getting started on Telemetry, I'm trying to give guidance
for
> the purposes of future CLs.

Thanks, I appreciate the feedback because I do agree that there's a learning
curve, and I see now how the cases are different from one another. I agree that
in this case it's simple enough to just test the whole dict, so that's what I'll
do here.

[nduca, 2014-07-11 18:56:50]

lgtm

[eakuefner-google (do not use), 2014-07-11 19:53:04]

The CQ bit was checked by eakuefner@google.com

[commit-bot: I haz the power, 2014-07-11 19:54:01]

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

[commit-bot: I haz the power, 2014-07-11 21:15:26]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  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-11 21:45:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-11 21:45:58]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[eakuefner, 2014-07-11 23:33:56]

The CQ bit was checked by eakuefner@chromium.org

[commit-bot: I haz the power, 2014-07-11 23:34:29]

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

[commit-bot: I haz the power, 2014-07-12 01:57:28]

Change committed as 282776