Add src-side launcher for telemetry_unittests

Add src-side launcher for telemetry_unittests

This will need a recipe-side change to pass the paths known to recipe
(from the path recipe module) and properties (from the properties
recipe module).

Each of these key-value pairs is passed as two command line arguments,
so that parsing them is easy and rock solid (avoids escaping issues),
and is also easy to convert to dict python-side.

BUG=422235
R=iannucci@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/5316422143176a4a6457ce173ec9597a2e0eda69

[Paweł Hajdan Jr., 2014-10-16 14:53:22]

phajdan.jr@chromium.org changed reviewers:
+ iannucci@chromium.org

[Paweł Hajdan Jr., 2014-10-16 14:53:22]

Tested locally, and it's also nice we can test using trybots for regressions
(note that I'll need a recipe-side change to start using the telemetry script,
but we can also test checkdeps didn't break).

[iannucci, 2014-10-16 21:08:30]

On 2014/10/16 14:53:22, Paweł Hajdan Jr. wrote:
> Tested locally, and it's also nice we can test using trybots for regressions
> (note that I'll need a recipe-side change to start using the telemetry script,
> but we can also test checkdeps didn't break).

1) command line lengths are a real problem on windows. I would really recommend
passing them as json files instead

2) this may create hidden dependencies for us, since we don't know which scripts
rely on which properties. I would much rather have the src-side scripts have a
way to print their property dependencies out, and then only have our stuff send
the requested properties. This would make it easy for us to see who needs which
properties. WDYT?

[Paweł Hajdan Jr., 2014-10-17 11:11:20]

On 2014/10/16 21:08:30, iannucci wrote:
> 1) command line lengths are a real problem on windows. I would really
recommend
> passing them as json files instead

Oh, OK, I added fromfile_prefix_chars support from argparse so we have a
standard and transparent way of handling this.

> 2) this may create hidden dependencies for us, since we don't know which
scripts
> rely on which properties. I would much rather have the src-side scripts have a
> way to print their property dependencies out, and then only have our stuff
send
> the requested properties. This would make it easy for us to see who needs
which
> properties. WDYT?

Of course I was thinking about this. It's more complicated that it may seem
though.

a) testing whether a recipe-side change doesn't break things only testable
src-side has historically been non-trivial; would the ability to do a build with
experimental/patched recipe version help here? Then we could use that to see
what would break if we removed a property without breaking production.

b) we already have that problem to some degree (passing --factory-properties to
runtest.py); given that we can test src-side changes right now and can't test
build-side changes in the same way (i.e. on a real trybot), arguably this makes
things better; I'd imagine next step might be remove runtest.py / move some
logic src-side, while providing a cleaner API build-side that is easier to
reason about

c) for a recipe to get any info from the script or even a file, it'd need to add
an artificial step; this can very easily clutter the build results page which
Chrome developers will object to, and when we convert all steps to this format
the number of steps would approximately double

d) note that we're likely to run into above problem to be able to query
compile_targets, but then we could have just one script to call and query all
the scripts in src/testing/scripts (that aggregator script could also live in
src or it could be a recipe module resource).

e) it should be possible to grep for property names in the scripts; they're all
relatively small and simple, and we could possible use some conventions or APIs
to help greppability

f) finally, what are example scenarios where what you're suggesting would make a
difference? In my opinion the biggest gains are in testability, being able to
see if the entire job works on a trybot, easily, and I believe this change
provides it. Of course I'm open to further refinements later, the main question
is why not start with something simple, unless there is a regression?

[iannucci, 2014-10-20 17:56:54]

On 2014/10/17 11:11:20, Paweł Hajdan Jr. wrote:
> On 2014/10/16 21:08:30, iannucci wrote:
> > 1) command line lengths are a real problem on windows. I would really
> recommend
> > passing them as json files instead
> 
> Oh, OK, I added fromfile_prefix_chars support from argparse so we have a
> standard and transparent way of handling this.
> 

Ugh, gross... fromfiles are a hack and are definitely out of place on
linux/mac... why prefer them over a real data format?

> > 2) this may create hidden dependencies for us, since we don't know which
> scripts
> > rely on which properties. I would much rather have the src-side scripts have
a
> > way to print their property dependencies out, and then only have our stuff
> send
> > the requested properties. This would make it easy for us to see who needs
> which
> > properties. WDYT?
> 
> Of course I was thinking about this. It's more complicated that it may seem
> though.
> 
> a) testing whether a recipe-side change doesn't break things only testable
> src-side has historically been non-trivial; would the ability to do a build
with
> experimental/patched recipe version help here? Then we could use that to see
> what would break if we removed a property without breaking production.

Yes, it would solve this problem.

> 
> b) we already have that problem to some degree (passing --factory-properties
to
> runtest.py); given that we can test src-side changes right now and can't test
> build-side changes in the same way (i.e. on a real trybot), arguably this
makes
> things better; I'd imagine next step might be remove runtest.py / move some
> logic src-side, while providing a cleaner API build-side that is easier to
> reason about
> 

We definitely do! That's why we don't want the problem in more places :)

It also means that we're much more strongly tied to buildbot, which I do not
like, at all. People will use broken features like buildnumber (I just dissuaded
someone from doing that 4 days ago).

> c) for a recipe to get any info from the script or even a file, it'd need to
add
> an artificial step; this can very easily clutter the build results page which
> Chrome developers will object to, and when we convert all steps to this format
> the number of steps would approximately double

This is a UI problem, not a functionality problem. I would much rather implement
hidden steps on the UI than introduce dependencies on all of *-properties.

> 
> d) note that we're likely to run into above problem to be able to query
> compile_targets, but then we could have just one script to call and query all
> the scripts in src/testing/scripts (that aggregator script could also live in
> src or it could be a recipe module resource).
> 

This also seems reasonable to me, and would be a good solution to solving c as
well.

> e) it should be possible to grep for property names in the scripts; they're
all
> relatively small and simple, and we could possible use some conventions or
APIs
> to help greppability

The issues is the API... it makes it look like all of the properties are
available. They're not all available, and some of them /definitely/ need to be
exempted. "can I refactor this?" should not imply "just grep for all strings in
all source code! And hope no one is doing string concatenation!"

> 
> f) finally, what are example scenarios where what you're suggesting would make
a
> difference? In my opinion the biggest gains are in testability, being able to
> see if the entire job works on a trybot, easily, and I believe this change
> provides it. Of course I'm open to further refinements later, the main
question
> is why not start with something simple, unless there is a regression?

The regression is that we're going from exposing precise arguments to a pile of
global values/landmines. People will depend on them, and it will make it more
difficult to clean them up. "It's a mess, so let's put more crap on the pile" is
not a good long term solution.

[iannucci, 2014-10-20 19:11:40]

OK, discussed offline. @fromfile is still a problem because:
  * only supported natively by argparse (ties to python, would require extra
work to support in !python).
  * is a windows-native concept (really only exists because windows has CLI
length issues)
  * still has C

[iannucci, 2014-10-20 19:14:01]

Oops, sent early.

OK, discussed offline. @fromfile is still a problem because:
  * only supported natively by argparse (ties to python, would require extra
work to support in !python).
  * is a windows-native concept (really only exists because windows has CLI
length issues)
  * still has CLI quoting complexity
  * is not typed (property values are actually typed in the JSON-style type
schema).

JSON is universally parsable, and would not be hard to support in argparse
either. You can do:

def parse_json(fname):
  with open(fname) as f:
    return json.load(f)

....

p.add_argument('--properties', type=parse_json)

[Paweł Hajdan Jr., 2014-10-21 14:40:20]

PTAL

Please see https://codereview.chromium.org/666023004 for recipe-side change.

I added a small script get_compile_targets.py so that we can get all of them in
one step.

[iannucci, 2014-10-22 07:23:15]

lgtm

https://codereview.chromium.org/649683005/diff/80001/testing/scripts/checkdep...
File testing/scripts/checkdeps.py (right):

https://codereview.chromium.org/649683005/diff/80001/testing/scripts/checkdep...
testing/scripts/checkdeps.py:38: def mode_compile_targets(args):
I would drop the mode_ prefix from these. maybe 'main_' would be a better
prefix?

https://codereview.chromium.org/649683005/diff/80001/testing/scripts/checkdep...
testing/scripts/checkdeps.py:44: sys.exit(common.run_script(sys.argv[1:],
mode_run, mode_compile_targets))
what about passing {
  'run': mode_run,
  'compile_targets': mode_compile_targets
}

or a kwargs version? I'm trying to avoid lots of functions as positional
arguments.

https://codereview.chromium.org/649683005/diff/80001/testing/scripts/common.py
File testing/scripts/common.py (right):

https://codereview.chromium.org/649683005/diff/80001/testing/scripts/common.p...
testing/scripts/common.py:42: run_parser.add_argument('--output', required=True)
type=argparse.FileType('w') ?

https://codereview.chromium.org/649683005/diff/80001/testing/scripts/common.p...
testing/scripts/common.py:66: def parse_common_test_results(json_results):
is this /really/ common? it seems like it's got a lot of webkit specific stuff

https://codereview.chromium.org/649683005/diff/80001/testing/scripts/common.p...
testing/scripts/common.py:67: def convert_trie_to_flat_paths(trie, prefix=None):
lol, I think this function is in like 4 locations now

https://codereview.chromium.org/649683005/diff/80001/testing/scripts/get_comp...
File testing/scripts/get_compile_targets.py (right):

https://codereview.chromium.org/649683005/diff/80001/testing/scripts/get_comp...
testing/scripts/get_compile_targets.py:36: passthrough_args +
why are passthrough_args before the mode parameter? that would bury the mode
parameter pretty far down the command line (trying to avoid another runtest.py
CLI fiasco :D).

[Paweł Hajdan Jr., 2014-10-22 09:43:27]

Thank you for the review. I believe I addressed the comments, and ongoing
feedback and improvements to this infrastructure are welcome.

In case I missed something, please ping me and I'll fix it.

https://codereview.chromium.org/649683005/diff/80001/testing/scripts/checkdep...
File testing/scripts/checkdeps.py (right):

https://codereview.chromium.org/649683005/diff/80001/testing/scripts/checkdep...
testing/scripts/checkdeps.py:38: def mode_compile_targets(args):
On 2014/10/22 07:23:14, iannucci wrote:
> I would drop the mode_ prefix from these. maybe 'main_' would be a better
> prefix?

Done.

https://codereview.chromium.org/649683005/diff/80001/testing/scripts/checkdep...
testing/scripts/checkdeps.py:44: sys.exit(common.run_script(sys.argv[1:],
mode_run, mode_compile_targets))
On 2014/10/22 07:23:14, iannucci wrote:
> what about passing {
>   'run': mode_run,
>   'compile_targets': mode_compile_targets
> }
> 
> or a kwargs version? I'm trying to avoid lots of functions as positional
> arguments.

Done.

https://codereview.chromium.org/649683005/diff/80001/testing/scripts/common.py
File testing/scripts/common.py (right):

https://codereview.chromium.org/649683005/diff/80001/testing/scripts/common.p...
testing/scripts/common.py:42: run_parser.add_argument('--output', required=True)
On 2014/10/22 07:23:15, iannucci wrote:
> type=argparse.FileType('w') ?

Done.

https://codereview.chromium.org/649683005/diff/80001/testing/scripts/common.p...
testing/scripts/common.py:66: def parse_common_test_results(json_results):
On 2014/10/22 07:23:14, iannucci wrote:
> is this /really/ common? it seems like it's got a lot of webkit specific stuff

As part of Ojan's CY work all test harnesses are standardizing on this format
(which indeed is originating from webkit). Please see PythonBasedTest in
steps.py, now several types of steps use this format, including python mojo
tests and telemetry.

https://codereview.chromium.org/649683005/diff/80001/testing/scripts/common.p...
testing/scripts/common.py:67: def convert_trie_to_flat_paths(trie, prefix=None):
On 2014/10/22 07:23:14, iannucci wrote:
> lol, I think this function is in like 4 locations now

Acknowledged.

https://codereview.chromium.org/649683005/diff/80001/testing/scripts/get_comp...
File testing/scripts/get_compile_targets.py (right):

https://codereview.chromium.org/649683005/diff/80001/testing/scripts/get_comp...
testing/scripts/get_compile_targets.py:36: passthrough_args +
On 2014/10/22 07:23:15, iannucci wrote:
> why are passthrough_args before the mode parameter? that would bury the mode
> parameter pretty far down the command line (trying to avoid another runtest.py
> CLI fiasco :D).

Suggestions are welcome - I think the reason is these arguments are defined in
the main parser as opposed to each of the subparsers, since they're common to
all subparsers.

[commit-bot: I haz the power, 2014-10-22 10:39:25]

Patchset 7 (id:??) landed as
https://crrev.com/5316422143176a4a6457ce173ec9597a2e0eda69
Cr-Commit-Position: refs/heads/master@{#300659}

[Paweł Hajdan Jr., 2014-10-22 10:39:29]

Committed patchset #7 (id:120001) manually as
5316422143176a4a6457ce173ec9597a2e0eda69.

[jam, 2014-10-22 16:29:15]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/655843004/ by jam@chromium.org.

The reason for reverting is: windows trybots broken

please be around after landing changes and verify that bots are working.