1113576 - update compare.py to include current platforms, more useful input

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

compare.py is a great tool to quickly compare a try revision to that of a master branch.

We are missing:
* some tests
* e10s platforms

I would also like to add:
* better support for >1 data point on try
* removing printing of datazilla related info

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: compare.py updates (1.0)

Comment 2

[Avi Halachmi (:avih)]

Comment on attachment 8539204 [details] [diff] [review]
compare.py updates (1.0)

Review of attachment 8539204 [details] [diff] [review]:
-----------------------------------------------------------------

Since compare.py is currently not used in any official capacity, I'm rubberstamping r+ without properly reviewing it, trusting Joel to make it more useful for the main user of this file, which is himself.

Joel, if you want a more thorough review, I think :wlach could help more. Your call.

Comment 3

[William Lachance (:wlach)]

Comment on attachment 8539204 [details] [diff] [review]
compare.py updates (1.0)

Review of attachment 8539204 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just a few comments. Please address if possible. :)

::: talos/compare.py
@@ +76,5 @@
> +platform_map['OSX10.8 (e10s)'] = 53
> +platforms = ['Linux', 'Linux64', 'Win7', 'WinXP', 'Win8', 'OSX64', 'OSX10.8']
> +platforms_e10s = ['Linux (e10s)', 'Linux64 (e10s)', 'Win7 (e10s)', 'WinXP (e10s)', 'Win8 (e10s)', 'OSX64 (e10s)', 'OSX10.8 (e10s)']
> +
> +#TODO: we might want to do this smarter since we won't have data for all non-e10s and e10s platforms at once

What do you mean by this? Could you explain in more detail?

@@ +279,4 @@
>  
>  
> +def compareResults(revision, branch, masterbranch, skipdays, platforms, tests, pgo=False, printurl=False, dzdata=None, pgodzdata=None, doPrint=False):
> +    startdate = int(time.mktime((datetime.datetime.now() - datetime.timedelta(days=(skipdays+14))).timetuple()))

Could you at least make the "14" a hardcoded constant on the top of the file and explain what it means? Perhaps better would be to make it a command line argument with a default of 14 (and an explanation of what it does)

@@ +291,2 @@
>          for t in tests:
> +            dzval = ""

Nit: I think this should be None

@@ +294,5 @@
>                  if p in dzdata:
>                      if test_map[t]['tbplname'] in dzdata[p]:
>                          dzval = dzdata[p][test_map[t]['tbplname']]
>  
> +            pgodzval = ""

Nit: I think this should be None

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: compare.py updates (1.1)

:wlach, I have updated the patch with your nits.  Regarding the e10s platforms, they needed to be defined intially.  Right now talos e10s on try isn't running, in fact it is really only running on mozilla-central.  So the usage of it now yields a lot of noise in the compare.  

As we have active plans to increase and expand our e10s coverage this will be more meaningful.  It will always be a balance to strike.  As we use it more, it will make more sense to what usage is best by default and what is better defined behind a cli switch.

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

https://hg.mozilla.org/build/talos/rev/3e79e6bf42b6