Telemetry: Add option to create a dirty profile as part of a test run

Telemetry: Add option to create a dirty profile as part of a test run

Add a dirty_small profile type, when used this creates a "small" dirty profile which is used in the subsequent test run.

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

[nduca, 2013-04-26 20:09:36]

https://codereview.chromium.org/14359012/diff/4001/tools/perf/perf_tools/smal...
File tools/perf/perf_tools/small_profile.py (right):

https://codereview.chromium.org/14359012/diff/4001/tools/perf/perf_tools/smal...
tools/perf/perf_tools/small_profile.py:1: # Copyright (c) 2013 The Chromium
Authors. All rights reserved.
why is this one here and not with the dirty one?

https://codereview.chromium.org/14359012/diff/4001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/core/browser.py (right):

https://codereview.chromium.org/14359012/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/browser.py:81: def profile_directory(self):
we shouldn't let people see where the profile dir is

https://codereview.chromium.org/14359012/diff/4001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/core/browser_options.py (left):

https://codereview.chromium.org/14359012/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/browser_options.py:25: 
what happens when this patch and you try --profile-type=dirty on clank? do we
explode? how?

https://codereview.chromium.org/14359012/diff/4001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/core/browser_options.py (right):

https://codereview.chromium.org/14359012/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/browser_options.py:197: self.profile_dir =
profile_types.GetProfileDir(self.profile_type)
i'd like to not allow profile_dir to be set by users of BrowserOptions. Can you
push this to be more of a constructor-time parameter to BrowserBackend?

can't all this go away completely, basically?

https://codereview.chromium.org/14359012/diff/4001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/core/dirty_profile_creator.py (right):

https://codereview.chromium.org/14359012/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/dirty_profile_creator.py:9: # Object that manages
filling in dirty profiles.
maybe put this module in a subdir, profile_creators?

[jeremy, 2013-05-01 11:52:00]

Ready for another look, thanks!

https://codereview.chromium.org/14359012/diff/4001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/core/browser_options.py (left):

https://codereview.chromium.org/14359012/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/browser_options.py:25: 
I've added code to explicitly thrown an exception on Clank and CrOS when
specifying a dirty profile.

[nduca, 2013-05-02 05:43:34]

https://codereview.chromium.org/14359012/diff/16001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/browser_options.py (right):

https://codereview.chromium.org/14359012/diff/16001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/browser_options.py:25: 
Do we let people specify the profile dir? If we do, we should prevent it.

And we shold also prevent them doing --extra-browser-args="--user-data-dir".
https://code.google.com/p/chromium/issues/detail?id=237438 is going to create
infrastructure for this and you may want to follow along.

https://codereview.chromium.org/14359012/diff/16001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/chrome/desktop_browser_finder.py (right):

https://codereview.chromium.org/14359012/diff/16001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/chrome/desktop_browser_finder.py:66: if
(self.options.profile_type.startswith('dirty')):
what about having 

creator = ProfileTypes.GetProfileCreator(self.options.profile_type)
if creator.NeedsToCreateProfile():
   create browser internal, do the stuff.
else:
  b= blahblah

Then have the profile types get autodiscovered in profile_types using
discover.py.

https://codereview.chromium.org/14359012/diff/16001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/chrome/desktop_browser_finder.py:78: (b, backend)
= self._CreateBrowserInternal(True)
when passing bool arguments, pass the name too
 CreateBrowserInternal(delete_profile_dir_after_run=True) so its obvious what
you're trying to do.

also, shouldn't that be false here?

https://codereview.chromium.org/14359012/diff/16001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/profile_creators/dirty_profile_creator.py
(right):

https://codereview.chromium.org/14359012/diff/16001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_creators/dirty_profile_creator.py:16:
profile_creator = small_profile.SmallProfile(b)
i guess i'm confused about what dirty_profile_creator is addding on top of
small_profile.py

https://codereview.chromium.org/14359012/diff/16001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/profile_creators/small_profile.py (right):

https://codereview.chromium.org/14359012/diff/16001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_creators/small_profile.py:5: import os
why isn't this small_profile_creator?

https://codereview.chromium.org/14359012/diff/16001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_creators/small_profile.py:10: class
SmallProfile(object):
you should have a profile_creator.py that defines a base class from which these
inherit. You can then discover all the subclasses automatically using discover.
And it becomes cleaner to understand how they work.

https://codereview.chromium.org/14359012/diff/16001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_creators/small_profile.py:17: '..', '..',
'..', '..',
clean_profile_creator should exist too. It should be a noop but it should exist.

https://codereview.chromium.org/14359012/diff/16001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/profile_types.py (right):

https://codereview.chromium.org/14359012/diff/16001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_types.py:7: BASE_PROFILE_TYPES =
['clean', 'default', 'dirty_small']
i dont understand the point of "default". I'd rather it didn't exist.

[jeremy, 2013-05-02 15:00:51]

Addressed Nat's comments, ready for another look.

Tony: Please see question inline about using WPR in the telemetry tests

https://codereview.chromium.org/14359012/diff/16001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/browser_options.py (right):

https://codereview.chromium.org/14359012/diff/16001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/browser_options.py:25: 
Doesn't look like we do, only via profile-type. tonyg@ would know for sure.

https://codereview.chromium.org/14359012/diff/16001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/chrome/desktop_browser_finder.py (right):

https://codereview.chromium.org/14359012/diff/16001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/chrome/desktop_browser_finder.py:66: if
(self.options.profile_type.startswith('dirty')):
On 2013/05/02 05:43:34, nduca wrote:
> what about having 
> 
> creator = ProfileTypes.GetProfileCreator(self.options.profile_type)
> if creator.NeedsToCreateProfile():
>    create browser internal, do the stuff.
> else:
>   b= blahblah
> 
> Then have the profile types get autodiscovered in profile_types using
> discover.py.

Done.

https://codereview.chromium.org/14359012/diff/16001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/chrome/desktop_browser_finder.py:78: (b, backend)
= self._CreateBrowserInternal(True)
Done.
True is what we want here.  The dirty profile is created in it's own directory,
the test then takes the profile and copies it into a new temporary directory
which is deleted.

The reason that we don't delete the profile is because a followup CL will create
the dirty profile in a known location based on the build ID so we cache the
dirty profiles as we discussed in person.

https://codereview.chromium.org/14359012/diff/16001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/profile_creators/dirty_profile_creator.py
(right):

https://codereview.chromium.org/14359012/diff/16001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_creators/dirty_profile_creator.py:16:
profile_creator = small_profile.SmallProfile(b)
It provides the mapping between profile_type and which profiel creator we need
(the idea is that we may have several of them).

The use of discover.py removes the need for that.

https://codereview.chromium.org/14359012/diff/16001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/profile_creators/small_profile.py (right):

https://codereview.chromium.org/14359012/diff/16001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_creators/small_profile.py:5: import os
On 2013/05/02 05:43:34, nduca wrote:
> why isn't this small_profile_creator?

Done.

https://codereview.chromium.org/14359012/diff/16001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_creators/small_profile.py:10: class
SmallProfile(object):
On 2013/05/02 05:43:34, nduca wrote:
> you should have a profile_creator.py that defines a base class from which
these
> inherit. You can then discover all the subclasses automatically using
discover.
> And it becomes cleaner to understand how they work.

Done.

https://codereview.chromium.org/14359012/diff/16001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_creators/small_profile.py:17: '..', '..',
'..', '..',
There is a larger question here about how we want to set up the various profile
types in Telemetry.  I don't fully understand the considerations and I'd rather
not tackle that in this CL.

For dirty profiles, the only case I currently need is a small profile.  If we
need any further profile types they should be easy to add.

https://codereview.chromium.org/14359012/diff/16001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/profile_types.py (right):

https://codereview.chromium.org/14359012/diff/16001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_types.py:7: BASE_PROFILE_TYPES =
['clean', 'default', 'dirty_small']
Neither do I, again I think this issue is orthogonal to this CL.  I just
documented the variable used since it wasn't immediately obvious to me what was
going on.

https://codereview.chromium.org/14359012/diff/25001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/browser_unittest.py (right):

https://codereview.chromium.org/14359012/diff/25001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/browser_unittest.py:90: def
testDirtyProfileCreation(self):
This test currently fails because it loads content from the live web (?), how do
I get WPR running for the test? Do you have any other suggestions?

I'm intentionally not mocking out browser internals since I want one test for
all platforms.

https://codereview.chromium.org/14359012/diff/25001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/profile_types.py (right):

https://codereview.chromium.org/14359012/diff/25001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_types.py:22: #     
profile_creator.ProfileCreator)
This doesn't work, I suspect the problem is with the first 2 arguments, if you
could vend me a clue on why this might not be working I'd be grateful otherwise
I'll look into getting this working on Sunday.

[dtu, 2013-05-02 18:43:05]

https://codereview.chromium.org/14359012/diff/16001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/profile_creators/small_profile.py (right):

https://codereview.chromium.org/14359012/diff/16001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_creators/small_profile.py:7: from
telemetry.page import page_set
telemetry.core cannot depend on telemetry.page.

https://codereview.chromium.org/14359012/diff/16001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_creators/small_profile.py:18: 'perf',
'page_sets', 'top_25.json')
tools/telemetry cannot depend on tools/perf.

[jeremy, 2013-05-02 20:47:15]

https://codereview.chromium.org/14359012/diff/16001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/profile_creators/small_profile.py (right):

https://codereview.chromium.org/14359012/diff/16001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_creators/small_profile.py:7: from
telemetry.page import page_set
I understand this file is placed in the wrong place in the tree, where do you
think the right place is to put it so that these dependencies will work right?

tools/perf/profile_creators ?

[nduca, 2013-05-03 17:58:56]

its in the right piece, just dont use page_sets and do it by hand, i guess...

[nduca, 2013-05-03 17:59:40]

or we could break the core<->page rule... i mean, i dont really think its
important that they dont refer to each other...

[dtu, 2013-05-03 19:45:58]

On 2013/05/03 17:59:40, nduca wrote:
> or we could break the core<->page rule... i mean, i dont really think its
> important that they dont refer to each other...

For core/page, yeah, I think it's fine. For telemetry/perf I'm more hesitant.

[nduca, 2013-05-03 20:03:32]

Yeah, definitely telemetry->perf is verbotten.

So then this looks relatively solid as is. We just need to look through the
details of the patch now, I guesss...


right off the bat, profile_types and profile_creators seems strange. why the
different names?

[dtu, 2013-05-03 21:19:21]

On 2013/05/03 20:03:32, nduca wrote:
> Yeah, definitely telemetry->perf is verbotten.
> 
> So then this looks relatively solid as is. We just need to look through the
> details of the patch now, I guesss...
> 
> 
> right off the bat, profile_types and profile_creators seems strange. why the
> different names?

Wait, the only reason it imports page_set is to use the top_25 page set. So it
still relies on a file in perf.

[nduca, 2013-05-03 21:21:12]

Thats not good. We'll have to figure something out for that.

[jeremy, 2013-05-06 10:43:25]

On 2013/05/03 20:03:32, nduca wrote:
> So then this looks relatively solid as is. We just need to look through the
> details of the patch now, I guess...

Thanks! Would appreciate a thorough review so we can hopefully minimise the
number of iterations this patch needs to go through and get it landed quickly to
everyone's satisfaction.

> right off the bat, profile_types and profile_creators seems strange. why the
> different names?

I'm not strongly attached :) Any alternate design and naming you think would be
better is fine by me, just let me know the names and give some guidance on what
you'd like me to change.

[dtu, 2013-05-07 00:48:12]

https://chromiumcodereview.appspot.com/14359012/diff/25001/tools/telemetry/te...
File tools/telemetry/telemetry/core/browser_unittest.py (right):

https://chromiumcodereview.appspot.com/14359012/diff/25001/tools/telemetry/te...
tools/telemetry/telemetry/core/browser_unittest.py:90: def
testDirtyProfileCreation(self):
On 2013/05/02 15:00:52, jeremy wrote:
> This test currently fails because it loads content from the live web (?), how
do
> I get WPR running for the test? Do you have any other suggestions?
> 
> I'm intentionally not mocking out browser internals since I want one test for
> all platforms.

What if you always use a PageTest for the ProfileCreator? It comes with WPR
included :)

https://chromiumcodereview.appspot.com/14359012/diff/25001/tools/telemetry/te...
File tools/telemetry/telemetry/core/chrome/desktop_browser_finder.py (right):

https://chromiumcodereview.appspot.com/14359012/diff/25001/tools/telemetry/te...
tools/telemetry/telemetry/core/chrome/desktop_browser_finder.py:75: b.Close()
Use a with or try/finally block to ensure we don't leave browsers running if
this gets interrupted.

https://chromiumcodereview.appspot.com/14359012/diff/25001/tools/telemetry/te...
File tools/telemetry/telemetry/core/profile_creators/profile_creator.py (right):

https://chromiumcodereview.appspot.com/14359012/diff/25001/tools/telemetry/te...
tools/telemetry/telemetry/core/profile_creators/profile_creator.py:12: pass
raise NotImplementedError()

https://chromiumcodereview.appspot.com/14359012/diff/25001/tools/telemetry/te...
File tools/telemetry/telemetry/core/profile_creators/small_profile_creator.py
(right):

https://chromiumcodereview.appspot.com/14359012/diff/25001/tools/telemetry/te...
tools/telemetry/telemetry/core/profile_creators/small_profile_creator.py:13:
super(SmallProfileCreator, self).__init__(browser)
This constructor is redundant with the parent's constructor. Remove.

https://chromiumcodereview.appspot.com/14359012/diff/25001/tools/telemetry/te...
File tools/telemetry/telemetry/core/profile_types.py (right):

https://chromiumcodereview.appspot.com/14359012/diff/25001/tools/telemetry/te...
tools/telemetry/telemetry/core/profile_types.py:4: 
Comments in this file, since it's a public API for telemetry.core.

https://chromiumcodereview.appspot.com/14359012/diff/25001/tools/telemetry/te...
tools/telemetry/telemetry/core/profile_types.py:16: def
DiscoverCreateableProfiles():
Precede with _ for protected methods.

https://chromiumcodereview.appspot.com/14359012/diff/25001/tools/telemetry/te...
tools/telemetry/telemetry/core/profile_types.py:22: #     
profile_creator.ProfileCreator)
On 2013/05/02 15:00:52, jeremy wrote:
> This doesn't work, I suspect the problem is with the first 2 arguments, if you
> could vend me a clue on why this might not be working I'd be grateful
otherwise
> I'll look into getting this working on Sunday.

top_level_dir (the 2nd arg) should be tools/telemetry.

https://chromiumcodereview.appspot.com/14359012/diff/25001/tools/telemetry/te...
tools/telemetry/telemetry/core/profile_types.py:29: global PROFILE_CREATORS
Not global, the variable is module-level.

https://chromiumcodereview.appspot.com/14359012/diff/25001/tools/telemetry/te...
tools/telemetry/telemetry/core/profile_types.py:32: return BASE_PROFILE_TYPES +
PROFILE_TYPE_MAPPING.keys() +\
Google style guide discourages the \ line continuation. Wrap the thing in
parentheses instead.

https://chromiumcodereview.appspot.com/14359012/diff/25001/tools/telemetry/te...
tools/telemetry/telemetry/core/profile_types.py:36: if profile_type in
BASE_PROFILE_TYPES or GetProfileCreator(profile_type):
profile_type in PROFILE_CREATORS

https://chromiumcodereview.appspot.com/14359012/diff/25001/tools/telemetry/te...
File tools/telemetry/telemetry/core/profile_types_unittest.py (right):

https://chromiumcodereview.appspot.com/14359012/diff/25001/tools/telemetry/te...
tools/telemetry/telemetry/core/profile_types_unittest.py:10: from
telemetry.core.profile_creators import profile_creator
Import at the top of the file.

https://chromiumcodereview.appspot.com/14359012/diff/25001/tools/telemetry/te...
tools/telemetry/telemetry/core/profile_types_unittest.py:13:
self.assertNotIn(profile_creator.ProfileCreator, creators.keys())
Unfortunately, Chromium is still Python 2.6 and these asserts are 2.7. Use
assertTrue and assertFalse.

[jeremy, 2013-05-08 09:13:04]

Thanks Dave! Ready for another look...

https://codereview.chromium.org/14359012/diff/25001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/chrome/desktop_browser_finder.py (right):

https://codereview.chromium.org/14359012/diff/25001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/chrome/desktop_browser_finder.py:75: b.Close()
On 2013/05/07 00:48:13, Dave Tu wrote:
> Use a with or try/finally block to ensure we don't leave browsers running if
> this gets interrupted.

Done.

https://codereview.chromium.org/14359012/diff/25001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/profile_creators/profile_creator.py (right):

https://codereview.chromium.org/14359012/diff/25001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_creators/profile_creator.py:12: pass
On 2013/05/07 00:48:13, Dave Tu wrote:
> raise NotImplementedError()

Done.

https://codereview.chromium.org/14359012/diff/25001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/profile_creators/small_profile_creator.py
(right):

https://codereview.chromium.org/14359012/diff/25001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_creators/small_profile_creator.py:13:
super(SmallProfileCreator, self).__init__(browser)
On 2013/05/07 00:48:13, Dave Tu wrote:
> This constructor is redundant with the parent's constructor. Remove.

Done.

https://codereview.chromium.org/14359012/diff/25001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/profile_types.py (right):

https://codereview.chromium.org/14359012/diff/25001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_types.py:4: 
On 2013/05/07 00:48:13, Dave Tu wrote:
> Comments in this file, since it's a public API for telemetry.core.

Done.

https://codereview.chromium.org/14359012/diff/25001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_types.py:16: def
DiscoverCreateableProfiles():
On 2013/05/07 00:48:13, Dave Tu wrote:
> Precede with _ for protected methods.

Done.

https://codereview.chromium.org/14359012/diff/25001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_types.py:22: #     
profile_creator.ProfileCreator)
On 2013/05/07 00:48:13, Dave Tu wrote:
> On 2013/05/02 15:00:52, jeremy wrote:
> > This doesn't work, I suspect the problem is with the first 2 arguments, if
you
> > could vend me a clue on why this might not be working I'd be grateful
> otherwise
> > I'll look into getting this working on Sunday.
> 
> top_level_dir (the 2nd arg) should be tools/telemetry.

Done.

https://codereview.chromium.org/14359012/diff/25001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_types.py:29: global PROFILE_CREATORS
On 2013/05/07 00:48:13, Dave Tu wrote:
> Not global, the variable is module-level.

Done.

https://codereview.chromium.org/14359012/diff/25001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_types.py:32: return BASE_PROFILE_TYPES +
PROFILE_TYPE_MAPPING.keys() +\
On 2013/05/07 00:48:13, Dave Tu wrote:
> Google style guide discourages the \ line continuation. Wrap the thing in
> parentheses instead.

Done.

https://codereview.chromium.org/14359012/diff/25001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_types.py:36: if profile_type in
BASE_PROFILE_TYPES or GetProfileCreator(profile_type):
On 2013/05/07 00:48:13, Dave Tu wrote:
> profile_type in PROFILE_CREATORS

Done.

https://codereview.chromium.org/14359012/diff/25001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/profile_types_unittest.py (right):

https://codereview.chromium.org/14359012/diff/25001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_types_unittest.py:10: from
telemetry.core.profile_creators import profile_creator
On 2013/05/07 00:48:13, Dave Tu wrote:
> Import at the top of the file.

Done.

https://codereview.chromium.org/14359012/diff/25001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_types_unittest.py:13:
self.assertNotIn(profile_creator.ProfileCreator, creators.keys())
On 2013/05/07 00:48:13, Dave Tu wrote:
> Unfortunately, Chromium is still Python 2.6 and these asserts are 2.7. Use
> assertTrue and assertFalse.

Done.

[tonyg, 2013-05-10 00:41:33]

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/browser_unittest.py (right):

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/browser_unittest.py:91: is_running_on_desktop =
p.startswith('linux') or p in ['darwin', 'win32']
I don't think this works. When telemetry runs on cros or android, it is actually
running on the host linux machine. I think the only way to test
is_running_on_desktop is to test options.browser.startswith('android') or
options.browser.startswith('cros')

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/browser_unittest.py:93: return
Output a logging.warning here that the test is skipped.

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/chrome/android_browser_finder.py (right):

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/chrome/android_browser_finder.py:56: raise
Exception("Dirty profiles not currently supported on Android")
s/Dirty profiles/Profile creation/

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/chrome/cros_browser_finder.py (right):

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/chrome/cros_browser_finder.py:33: raise
Exception("Dirty profiles not currently supported on Chrome OS")
s/Dirty profiles/Profile creation/

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/chrome/desktop_browser_backend.py (right):

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/chrome/desktop_browser_backend.py:105:
logging.critical('Dirty profiles cannot be used with content shell')
s/Dirty profiles/Profile creation/

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/profile_creators/profile_creator.py (right):

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_creators/profile_creator.py:5: # Base
class for an object that constructs a Chrome profile.
"""docstring"""

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_creators/profile_creator.py:10: # Fill in
the profile in question.
"""docstring"""

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/profile_creators/small_profile_creator.py
(right):

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_creators/small_profile_creator.py:10: #
Runs a browser through a series of operations to fill in a small test profile.
"""docstring"""

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_creators/small_profile_creator.py:15:
'perf', 'page_sets', 'top_25.json')
Just thinking aloud: one way to avoid this dependency would be to pass the page
set in on the command line.
--profile-type=generated
--profile-seed-page-set=tools/perf/page_sets/top_25.json

See what Nat and Dave think before changing any code around though.

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_creators/small_profile_creator.py:19:
tab.Navigate(page.url)
Is web page replay started at this point? I don't think we want to hit the live
sites during profile creation.

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/profile_types.py (right):

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_types.py:20: # 'small_profile'.
Please make all these function level comments into """docstrings""". See
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Comments

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_types.py:23: from telemetry.test import
discover
Imports at the top of the file

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_types.py:23: from telemetry.test import
discover
I'm not sure we want to depend on test from here. Nat, Dave, any thoughts? Maybe
discover doesn't belong in test. It would be useful to use for profilers as
well.

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_types.py:36: if (test_name is
'profile_creator'):
if test_name == 'profile_creator':

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_types.py:37: pass
Do you mean continue instead of pass?

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_types.py:38: test_name =
test_name[:-len('_creator')]
This assumes test_name ends with _creator. Should we add:
assert test_name.endswith('_creator')

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_types.py:46: if len(PROFILE_CREATORS) ==
0:
if not PROFILE_CREATORS:

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_types.py:55: if profile_type in
BASE_PROFILE_TYPES or \
Use parens instead of \

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_types.py:56: profile_type in
PROFILE_CREATORS.keys():
I think the .keys() is unnecessary.

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/profile_types_unittest.py (right):

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_types_unittest.py:1: # Copyright (c) 2012
The Chromium Authors. All rights reserved.
2013

[tonyg, 2013-05-10 15:47:42]

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/profile_creators/small_profile_creator.py
(right):

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_creators/small_profile_creator.py:15:
'perf', 'page_sets', 'top_25.json')
On 2013/05/10 00:41:33, tonyg wrote:
> Just thinking aloud: one way to avoid this dependency would be to pass the
page
> set in on the command line.
> --profile-type=generated
> --profile-seed-page-set=tools/perf/page_sets/top_25.json
> 
> See what Nat and Dave think before changing any code around though.

After thinking about this some more, I wonder whether we should just move the
profile_creators directory to tools/perf/profile_creators. Then they get
discovered and passed on the command line just like page_sets and
page_measurements.

[jeremy, 2013-05-13 08:23:53]

All fixed, ready for another look.

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/browser_unittest.py (right):

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/browser_unittest.py:91: is_running_on_desktop =
p.startswith('linux') or p in ['darwin', 'win32']
On 2013/05/10 00:41:33, tonyg wrote:
> I don't think this works. When telemetry runs on cros or android, it is
actually
> running on the host linux machine. I think the only way to test
> is_running_on_desktop is to test options.browser.startswith('android') or
> options.browser.startswith('cros')

Done.

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/browser_unittest.py:93: return
On 2013/05/10 00:41:33, tonyg wrote:
> Output a logging.warning here that the test is skipped.

Done.

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/chrome/android_browser_finder.py (right):

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/chrome/android_browser_finder.py:56: raise
Exception("Dirty profiles not currently supported on Android")
On 2013/05/10 00:41:33, tonyg wrote:
> s/Dirty profiles/Profile creation/

Done.

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/chrome/desktop_browser_backend.py (right):

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/chrome/desktop_browser_backend.py:105:
logging.critical('Dirty profiles cannot be used with content shell')
On 2013/05/10 00:41:33, tonyg wrote:
> s/Dirty profiles/Profile creation/

Done.

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/profile_creators/profile_creator.py (right):

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_creators/profile_creator.py:5: # Base
class for an object that constructs a Chrome profile.
On 2013/05/10 00:41:33, tonyg wrote:
> """docstring"""

Done.

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_creators/profile_creator.py:5: # Base
class for an object that constructs a Chrome profile.
On 2013/05/10 00:41:33, tonyg wrote:
> """docstring"""

Done.

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_creators/profile_creator.py:10: # Fill in
the profile in question.
On 2013/05/10 00:41:33, tonyg wrote:
> """docstring"""

Done.

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/profile_creators/small_profile_creator.py
(right):

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_creators/small_profile_creator.py:10: #
Runs a browser through a series of operations to fill in a small test profile.
On 2013/05/10 00:41:33, tonyg wrote:
> """docstring"""

Done.

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/profile_types.py (right):

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_types.py:20: # 'small_profile'.
On 2013/05/10 00:41:33, tonyg wrote:
> Please make all these function level comments into """docstrings""". See
> http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Comments

Done.

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_types.py:23: from telemetry.test import
discover
On 2013/05/10 00:41:33, tonyg wrote:
> Imports at the top of the file

Done.

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_types.py:36: if (test_name is
'profile_creator'):
On 2013/05/10 00:41:33, tonyg wrote:
> if test_name == 'profile_creator':

Done.

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_types.py:37: pass
On 2013/05/10 00:41:33, tonyg wrote:
> Do you mean continue instead of pass?

Done.

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_types.py:38: test_name =
test_name[:-len('_creator')]
On 2013/05/10 00:41:33, tonyg wrote:
> This assumes test_name ends with _creator. Should we add:
> assert test_name.endswith('_creator')

Done.

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_types.py:46: if len(PROFILE_CREATORS) ==
0:
On 2013/05/10 00:41:33, tonyg wrote:
> if not PROFILE_CREATORS:

Done.

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_types.py:55: if profile_type in
BASE_PROFILE_TYPES or \
On 2013/05/10 00:41:33, tonyg wrote:
> Use parens instead of \

Done.

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_types.py:56: profile_type in
PROFILE_CREATORS.keys():
On 2013/05/10 00:41:33, tonyg wrote:
> I think the .keys() is unnecessary.

Done.

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/profile_types_unittest.py (right):

https://codereview.chromium.org/14359012/diff/50003/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_types_unittest.py:1: # Copyright (c) 2012
The Chromium Authors. All rights reserved.
On 2013/05/10 00:41:33, tonyg wrote:
> 2013

Done.

[jeremy, 2013-05-13 08:46:56]

https://codereview.chromium.org/14359012/diff/64001/tools/perf/profile_creato...
File tools/perf/profile_creators/__init__.py (right):

https://codereview.chromium.org/14359012/diff/64001/tools/perf/profile_creato...
tools/perf/profile_creators/__init__.py:8: def Init():
Is this really needed here?
Copied over from perf_tools/

[tonyg, 2013-05-13 18:21:59]

https://codereview.chromium.org/14359012/diff/64001/tools/perf/profile_creato...
File tools/perf/profile_creators/__init__.py (right):

https://codereview.chromium.org/14359012/diff/64001/tools/perf/profile_creato...
tools/perf/profile_creators/__init__.py:8: def Init():
On 2013/05/13 08:46:56, jeremy wrote:
> Is this really needed here?
> Copied over from perf_tools/

Not sure. If it runs without it, I'd remove it.

https://codereview.chromium.org/14359012/diff/64001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/profile_types.py (right):

https://codereview.chromium.org/14359012/diff/64001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_types.py:21: Returns a dictionary of all
the profile creators we can use to create
Bring this up a line along with """ (same thing below).

https://codereview.chromium.org/14359012/diff/64001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_types.py:28: os.path.dirname(__file__),
'..', '..', '..',  'perf', 'profile_creators')
This dep is still bad. We need to have tools/perf/run_multipage_benchmarks pass
the profile_creators dir to PageTestRunner.run() the same way test_dir works
today.

https://codereview.chromium.org/14359012/diff/64001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/test/run_tests.py (right):

https://codereview.chromium.org/14359012/diff/64001/tools/telemetry/telemetry...
tools/telemetry/telemetry/test/run_tests.py:91:
os.path.join(os.path.dirname(__file__), '..', '..', '..', 'perf'))
What is this for?

[jeremy, 2013-05-19 14:53:25]

Ready for another look, hope this iteration fixes all issues with depencies.

kbr: owner review for content/test/gpu/run_gpu_tests please

https://codereview.chromium.org/14359012/diff/64001/tools/perf/profile_creato...
File tools/perf/profile_creators/__init__.py (right):

https://codereview.chromium.org/14359012/diff/64001/tools/perf/profile_creato...
tools/perf/profile_creators/__init__.py:8: def Init():
On 2013/05/13 18:21:59, tonyg wrote:
> On 2013/05/13 08:46:56, jeremy wrote:
> > Is this really needed here?
> > Copied over from perf_tools/
> 
> Not sure. If it runs without it, I'd remove it.

Done.

https://codereview.chromium.org/14359012/diff/64001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/profile_types.py (right):

https://codereview.chromium.org/14359012/diff/64001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_types.py:21: Returns a dictionary of all
the profile creators we can use to create
On 2013/05/13 18:21:59, tonyg wrote:
> Bring this up a line along with """ (same thing below).

Done.

https://codereview.chromium.org/14359012/diff/64001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/test/run_tests.py (right):

https://codereview.chromium.org/14359012/diff/64001/tools/telemetry/telemetry...
tools/telemetry/telemetry/test/run_tests.py:91:
os.path.join(os.path.dirname(__file__), '..', '..', '..', 'perf'))
In order for discover.py to be able to load the classes in
'tools/perf/profile_creators', 'tools/perf' needs to be in the path.

It is in the path when telemetry is executed from tools/perf/run_measurement
because the executed scripts directory is added to the path when the script is
executed but unit tests are run from tools/telemetry/run_tests so
'tools/telemetry' is in the path which doesn't really help us.

I've added a support class in test data rather than using the class in
tools/perf/profile_creators for the test.

https://codereview.chromium.org/14359012/diff/81002/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/profile_types_unittest.py (right):

https://codereview.chromium.org/14359012/diff/81002/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/profile_types_unittest.py:8: class
ProfileTypesTest(unittest.TestCase):
I've removed the ProfileCreator tests from here in the meantime, the reason
being that we need a directory that can be scanned for ProfileCreator
subclasses. 

I could place a dummy package in telemetry.test but then I'd need to add
'tools/telemetry/telemetry/tests' to the path and I'm not sure how controversial
that change is.

[Ken Russell (switch to Gerrit), 2013-05-20 17:57:01]

content/test/gpu LGTM

[tonyg, 2013-05-21 02:06:29]

lgtm

Looks really clean now. Thanks for sticking with it through the iterations.

[nduca, 2013-05-21 02:10:26]

(Jeremy: you did it! High five!)

[commit-bot: I haz the power, 2013-05-21 08:38:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremy@chromium.org/14359012/89001

[commit-bot: I haz the power, 2013-05-21 12:28:23]

Change committed as 201293

[jeremy, 2013-05-21 12:29:17]

Thanks Guys!  Still need to get this working with WPR and get the results cached
on the bots before the work is really done.