rebaseline.py: if expectations dir contains JSON format results, update those instead of image files

rebaseline.py: if expectations dir contains JSON format results, update those instead of image files

Part of Step 3 in https://goto.google.com/ChecksumTransitionDetail

R=senorblanco@chromium.org

Committed: https://code.google.com/p/skia/source/detail?r=9910

[epoger, 2013-07-03 19:06:59]

https://codereview.chromium.org/18348018/diff/1/tools/tests/rebaseline/output...
File
tools/tests/rebaseline/output/using-json1-expectations/output-expected/gm-expectations/base-android-galaxy-nexus/expected-results.json
(right):

https://codereview.chromium.org/18348018/diff/1/tools/tests/rebaseline/output...
tools/tests/rebaseline/output/using-json1-expectations/output-expected/gm-expectations/base-android-galaxy-nexus/expected-results.json:4:
"allowed-digests": null,
In patchset 1, the code doesn't know how to actually update the JSON
expectations.  For now, it just writes them back out unmodified (except for
formatting).

I am uploading the CL as it is now, so that important modifications to the
expected-results.json file (not just whitespace changes) will show up in future
diffs.

[epoger, 2013-07-03 20:07:52]

As of patchset 2, it successfully updates entries within the
expected-results.json file!

I will add a few more tests, and then send it out for review.

https://codereview.chromium.org/18348018/diff/4001/tools/tests/rebaseline/out...
File
tools/tests/rebaseline/output/using-json1-expectations/output-expected/gm-expectations/base-android-galaxy-nexus/expected-results.json
(right):

https://codereview.chromium.org/18348018/diff/4001/tools/tests/rebaseline/out...
tools/tests/rebaseline/output/using-json1-expectations/output-expected/gm-expectations/base-android-galaxy-nexus/expected-results.json:33:
3359963596899141322
Looking at the actual-results.json files, these are exactly the results I would
expect to be rebaselined...

$ cat
tools/tests/rebaseline/input/json1/base-android-galaxy-nexus/Test-Android-GalaxyNexus-SGX540-Arm7-Debug/base-android-galaxy-nexus/actual-results.json
{
   "actual-results" : {
      "failed" : {
         "imageblur_565.png" : [ "bitmap-64bitMD5", 3359963596899141322 ],
         "imageblur_8888.png" : [ "bitmap-64bitMD5", 4217923806027861152 ],
         "shadertext3_8888.png" : [ "bitmap-64bitMD5", 3713708307125704716 ]
       },
      "failure-ignored" : null,
      "no-comparison" : {
         "3x3bitmaprect_565.png" : [ "bitmap-64bitMD5", 16998423976396106083 ],
         "3x3bitmaprect_8888.png" : [ "bitmap-64bitMD5", 2054956815327187963 ],
         "xfermodeimagefilter_pdf.png" : [ "bitmap-64bitMD5",
16502178848783208088 ]
      },
      "succeeded" : {
         "aaclip_gpu.png" : [ "bitmap-64bitMD5", 11899819492385205974 ],
         "aarectmodes_565.png" : [ "bitmap-64bitMD5", 14760033689012826769 ],
         "xfermodes_pdf.png" : [ "bitmap-64bitMD5", 9151974350149210736 ]
      }
   },
[...]

$ cat
tools/tests/rebaseline/input/json1/base-shuttle-win7-intel-float/Test-Win7-ShuttleA-HD2000-x86-Release/base-shuttle-win7-intel-float/actual-results.json
{
   "actual-results" : {
      "failed" : null,
      "failure-ignored" : null,
      "no-comparison" : {
         "3x3bitmaprect_565.png" : [ "bitmap-64bitMD5", 16998423976396106083 ],
         "3x3bitmaprect_8888.png" : [ "bitmap-64bitMD5", 2054956815327187963 ],
         "xfermodeimagefilter_pdf.png" : [ "bitmap-64bitMD5",
16502178848783208088 ]
      },
[...]

[epoger, 2013-07-03 20:18:12]

Ready for review at patchset 3!

Stephen, let me know if you'd like to delegate the review to someone else...

https://codereview.chromium.org/18348018/diff/10001/tools/rebaseline.py
File tools/rebaseline.py (left):

https://codereview.chromium.org/18348018/diff/10001/tools/rebaseline.py#oldco...
tools/rebaseline.py:142: def _DownloadFromGoogleStorage(self, infilename,
outfilename, all_results):
A lot of the old code (which lives on in rebaseline_imagefiles.py) is no longer
needed here.

https://codereview.chromium.org/18348018/diff/10001/tools/tests/rebaseline/ou...
File tools/tests/rebaseline/output/subset/output-expected/command_line (right):

https://codereview.chromium.org/18348018/diff/10001/tools/tests/rebaseline/ou...
tools/tests/rebaseline/output/subset/output-expected/command_line:1: python
tools/rebaseline.py --dry-run --expectations-root fake/expectations/path
--actuals-base-url file:tools/tests/rebaseline/input/json1 --tests test1 test2
--configs 565 8888 --subdirs base-android-galaxy-nexus
base-shuttle-win7-intel-float
I renamed this command-line option to distinguish from the expectations, which
can now also be expressed as a JSON file.

https://codereview.chromium.org/18348018/diff/10001/tools/tests/rebaseline/ou...
File
tools/tests/rebaseline/output/using-json1-expectations/output-expected/gm-expectations/base-android-galaxy-nexus/expected-results.json
(right):

https://codereview.chromium.org/18348018/diff/10001/tools/tests/rebaseline/ou...
tools/tests/rebaseline/output/using-json1-expectations/output-expected/gm-expectations/base-android-galaxy-nexus/expected-results.json:14:
"bitmap-64bitMD5",
If you look at the diffs for patchset 1 vs patchset 2, you'll see the "meat" of
the output changes (as opposed to the whitespace changes based on the JSON
pretty-printing routine)

[Stephen White, 2013-07-03 20:46:18]

The meat of the changes looks good. My comments are mostly just naming nits.

https://codereview.chromium.org/18348018/diff/10001/tools/rebaseline.py
File tools/rebaseline.py (right):

https://codereview.chromium.org/18348018/diff/10001/tools/rebaseline.py#newco...
tools/rebaseline.py:161: sections = [gm_json.JSONKEY_ACTUALRESULTS_FAILED]
Not new to this patch, IWBN to have some comments describing what these section
keys are (or if there are comments elsewhere, perhaps a reference to them). 
Also, JSONKEY_ACTUALRESULTS_NEW would make more sense to me than
JSON_KEY_ACTUALRESULTS_NOCOMPARISON, if these are new results.

https://codereview.chromium.org/18348018/diff/10001/tools/rebaseline.py#newco...
tools/rebaseline.py:188: [gm_json.JSONKEY_EXPECTEDRESULTS_ALLOWEDDIGESTS] = \
Similarly, I have no idea what ALLOWEDDIGESTS means. A comment or reference to
docs would be great.

https://codereview.chromium.org/18348018/diff/10001/tools/rebaseline.py#newco...
tools/rebaseline.py:203: help='base URL from which to read ACTUALS_FILENAME ' +
I'm not sure "ACTUALS_FILENAME" is meaningful to someone using this tool. Is
there something simpler/clearer we could display to the user?

https://codereview.chromium.org/18348018/diff/10001/tools/rebaseline.py#newco...
tools/rebaseline.py:207: help='filename (under ACTUALS_BASE_URL) to read a ' +
Same here for "ACTUALS_BASE_URL".

[epoger, 2013-07-08 14:40:54]

Thanks, Stephen!  Please take a look...

https://codereview.chromium.org/18348018/diff/10001/tools/rebaseline.py
File tools/rebaseline.py (right):

https://codereview.chromium.org/18348018/diff/10001/tools/rebaseline.py#newco...
tools/rebaseline.py:161: sections = [gm_json.JSONKEY_ACTUALRESULTS_FAILED]
On 2013/07/03 20:46:19, Stephen White wrote:
> Not new to this patch, IWBN to have some comments describing what these
section
> keys are (or if there are comments elsewhere, perhaps a reference to them). 
> Also, JSONKEY_ACTUALRESULTS_NEW would make more sense to me than
> JSON_KEY_ACTUALRESULTS_NOCOMPARISON, if these are new results.

Good idea.  I have added comments within gm_json.py.

For now at least, I'm leaving the name as JSON_KEY_ACTUALRESULTS_NOCOMPARISON
(because, for one thing, changing the string value from "no-comparison" to
something else would invalidate any existing GM JSON files).  The comment I
added in gm_json.py should help explain the name.

https://codereview.chromium.org/18348018/diff/10001/tools/rebaseline.py#newco...
tools/rebaseline.py:188: [gm_json.JSONKEY_EXPECTEDRESULTS_ALLOWEDDIGESTS] = \
On 2013/07/03 20:46:19, Stephen White wrote:
> Similarly, I have no idea what ALLOWEDDIGESTS means. A comment or reference to
> docs would be great.

See new comments in gm_json.py

https://codereview.chromium.org/18348018/diff/10001/tools/rebaseline.py#newco...
tools/rebaseline.py:203: help='base URL from which to read ACTUALS_FILENAME ' +
On 2013/07/03 20:46:19, Stephen White wrote:
> I'm not sure "ACTUALS_FILENAME" is meaningful to someone using this tool. Is
> there something simpler/clearer we could display to the user?

ACTUALS_FILENAME is part of the --help output generated by argparse (see below).
 But I have modified this a bit.... hopefully it's clearer.  If not, I welcome
any wording suggestions you might have. :-)


$ tools/rebaseline.py --help
usage: rebaseline.py [-h] [--actuals-base-url ACTUALS_BASE_URL]
                     [--actuals-filename ACTUALS_FILENAME] [--add-new]
                     [--configs CONFIG [CONFIG ...]] [--dry-run]
                     [--expectations-filename EXPECTATIONS_FILENAME]
                     [--expectations-root EXPECTATIONS_ROOT]
                     [--subdirs SUBDIR [SUBDIR ...]] [--tests TEST [TEST ...]]

optional arguments:
  -h, --help            show this help message and exit
  --actuals-base-url ACTUALS_BASE_URL
                        base URL from which to read files containing JSON
                        summaries of actual GM results; defaults to http
                        ://skia-autogen.googlecode.com/svn/gm-actual
  --actuals-filename ACTUALS_FILENAME
                        filename (within platform-specific subdirectories of
                        ACTUALS_BASE_URL) to read a summary of results from;
                        defaults to actual-results.json
  --add-new             in addition to the standard behavior of updating
                        expectations for failing tests, add expectations for
                        tests which don't have expectations yet.
  --configs CONFIG [CONFIG ...]
                        which configurations to rebaseline, e.g. "--configs
                        565 8888"; if unspecified, run a default set of
                        configs. This should ONLY be specified if --tests has
                        also been specified.
  --dry-run             instead of actually downloading files or adding files
                        to checkout, display a list of operations that we
                        would normally perform
  --expectations-filename EXPECTATIONS_FILENAME
                        filename (under EXPECTATIONS_ROOT) to read current
                        expectations from, and to write new expectations into;
                        defaults to expected-results.json
  --expectations-root EXPECTATIONS_ROOT
                        root of expectations directory to update-- should
                        contain one or more base-* subdirectories. Defaults to
                        .
  --subdirs SUBDIR [SUBDIR ...]
                        which platform subdirectories to rebaseline; if
                        unspecified, rebaseline all subdirs, same as "--
                        subdirs base-android-galaxy-nexus base-android-
                        nexus-10 base-android-nexus-4 base-android-nexus-7
                        base-android-nexus-s base-android-xoom base-macmini
                        base-macmini-lion-float base-shuttle-win7-intel-angle
                        base-shuttle-win7-intel-directwrite base-shuttle-win7
                        -intel-float base-shuttle_ubuntu12_ati5770"
  --tests TEST [TEST ...]
                        which tests to rebaseline, e.g. "--tests aaclip
                        bigmatrix"; if unspecified, then all failing tests
                        (according to the actual-results.json file) will be
                        rebaselined.

https://codereview.chromium.org/18348018/diff/10001/tools/rebaseline.py#newco...
tools/rebaseline.py:207: help='filename (under ACTUALS_BASE_URL) to read a ' +
On 2013/07/03 20:46:19, Stephen White wrote:
> Same here for "ACTUALS_BASE_URL".

Looking at the --help output, I think it's good as is... any particular
suggestion?

https://codereview.chromium.org/18348018/diff/18001/gm/gm_expectations.cpp
File gm/gm_expectations.cpp (right):

https://codereview.chromium.org/18348018/diff/18001/gm/gm_expectations.cpp#ne...
gm/gm_expectations.cpp:14: // See gm_json.py for descriptions of each of these
JSON keys.
Added gm_expectations.cpp to this CL, that it tells developers where to find
descriptions of the JSON keys.

[Stephen White, 2013-07-08 17:15:19]

LGTM, although consider the suggestion for "failure-ignored" docs.

https://codereview.chromium.org/18348018/diff/10001/tools/rebaseline.py
File tools/rebaseline.py (right):

https://codereview.chromium.org/18348018/diff/10001/tools/rebaseline.py#newco...
tools/rebaseline.py:161: sections = [gm_json.JSONKEY_ACTUALRESULTS_FAILED]
On 2013/07/08 14:40:54, epoger wrote:
> On 2013/07/03 20:46:19, Stephen White wrote:
> > Not new to this patch, IWBN to have some comments describing what these
> section
> > keys are (or if there are comments elsewhere, perhaps a reference to them). 
> > Also, JSONKEY_ACTUALRESULTS_NEW would make more sense to me than
> > JSON_KEY_ACTUALRESULTS_NOCOMPARISON, if these are new results.
> 
> Good idea.  I have added comments within gm_json.py.
> 
> For now at least, I'm leaving the name as JSON_KEY_ACTUALRESULTS_NOCOMPARISON
> (because, for one thing, changing the string value from "no-comparison" to
> something else would invalidate any existing GM JSON files).  The comment I
> added in gm_json.py should help explain the name.

OK. Just FYI, I think the Blink equivalent of this state is MISSING.

https://codereview.chromium.org/18348018/diff/10001/tools/rebaseline.py#newco...
tools/rebaseline.py:203: help='base URL from which to read ACTUALS_FILENAME ' +
On 2013/07/08 14:40:54, epoger wrote:
> On 2013/07/03 20:46:19, Stephen White wrote:
> > I'm not sure "ACTUALS_FILENAME" is meaningful to someone using this tool. Is
> > there something simpler/clearer we could display to the user?
> 
> ACTUALS_FILENAME is part of the --help output generated by argparse (see
below).
>  But I have modified this a bit.... hopefully it's clearer.  If not, I welcome
> any wording suggestions you might have. :-)

Much better, thanks!
> 
> 
> $ tools/rebaseline.py --help
> usage: rebaseline.py [-h] [--actuals-base-url ACTUALS_BASE_URL]
>                      [--actuals-filename ACTUALS_FILENAME] [--add-new]
>                      [--configs CONFIG [CONFIG ...]] [--dry-run]
>                      [--expectations-filename EXPECTATIONS_FILENAME]
>                      [--expectations-root EXPECTATIONS_ROOT]
>                      [--subdirs SUBDIR [SUBDIR ...]] [--tests TEST [TEST ...]]
> 
> optional arguments:
>   -h, --help            show this help message and exit
>   --actuals-base-url ACTUALS_BASE_URL
>                         base URL from which to read files containing JSON
>                         summaries of actual GM results; defaults to http
>                         ://skia-autogen.googlecode.com/svn/gm-actual
>   --actuals-filename ACTUALS_FILENAME
>                         filename (within platform-specific subdirectories of
>                         ACTUALS_BASE_URL) to read a summary of results from;
>                         defaults to actual-results.json
>   --add-new             in addition to the standard behavior of updating
>                         expectations for failing tests, add expectations for
>                         tests which don't have expectations yet.
>   --configs CONFIG [CONFIG ...]
>                         which configurations to rebaseline, e.g. "--configs
>                         565 8888"; if unspecified, run a default set of
>                         configs. This should ONLY be specified if --tests has
>                         also been specified.
>   --dry-run             instead of actually downloading files or adding files
>                         to checkout, display a list of operations that we
>                         would normally perform
>   --expectations-filename EXPECTATIONS_FILENAME
>                         filename (under EXPECTATIONS_ROOT) to read current
>                         expectations from, and to write new expectations into;
>                         defaults to expected-results.json
>   --expectations-root EXPECTATIONS_ROOT
>                         root of expectations directory to update-- should
>                         contain one or more base-* subdirectories. Defaults to
>                         .
>   --subdirs SUBDIR [SUBDIR ...]
>                         which platform subdirectories to rebaseline; if
>                         unspecified, rebaseline all subdirs, same as "--
>                         subdirs base-android-galaxy-nexus base-android-
>                         nexus-10 base-android-nexus-4 base-android-nexus-7
>                         base-android-nexus-s base-android-xoom base-macmini
>                         base-macmini-lion-float base-shuttle-win7-intel-angle
>                         base-shuttle-win7-intel-directwrite base-shuttle-win7
>                         -intel-float base-shuttle_ubuntu12_ati5770"
>   --tests TEST [TEST ...]
>                         which tests to rebaseline, e.g. "--tests aaclip
>                         bigmatrix"; if unspecified, then all failing tests
>                         (according to the actual-results.json file) will be
>                         rebaselined.

https://codereview.chromium.org/18348018/diff/18001/gm/gm_json.py
File gm/gm_json.py (right):

https://codereview.chromium.org/18348018/diff/18001/gm/gm_json.py#newcode45
gm/gm_json.py:45: # in the FAILED section or the FAILUREIGNORED section?
Just to be braindead-clear, perhaps this should say 'If marked as
"ignore-failure", a test failure will be reported under the FAILUREINGORED
section instead of the FAILED section." (If this will will also cause the bot
not to go red, perhaps that should be documented here too?)

[epoger, 2013-07-08 17:52:03]

Committed patchset #5 manually as r9910 (presubmit successful).

[epoger, 2013-07-08 17:52:29]

https://codereview.chromium.org/18348018/diff/18001/gm/gm_json.py
File gm/gm_json.py (right):

https://codereview.chromium.org/18348018/diff/18001/gm/gm_json.py#newcode45
gm/gm_json.py:45: # in the FAILED section or the FAILUREIGNORED section?
On 2013/07/08 17:15:20, Stephen White wrote:
> Just to be braindead-clear, perhaps this should say 'If marked as
> "ignore-failure", a test failure will be reported under the FAILUREINGORED
> section instead of the FAILED section." (If this will will also cause the bot
> not to go red, perhaps that should be documented here too?)

Good idea. Done.