Add presubmit check for run-bindings-tests

Add presubmit check for run-bindings-tests

run-bindings-tests should be run (and succeed) for any changes to
Source/bindings. This is particularly important to make sure tests are
rebaselined when changes are made to the code generator.
This adds a PRESUBMIT.py script to do just that!

Checked that this worked by changing a test IDL in
  Source/bindings/tests/idls
...and running
  git cl upload
...which then gives a presubmit warning, as desired!

Script based on GetUnitTests in
tools/depot_tools/presubmit_canned_checks.py
...which seems exemplary. I've stripped it down to the minimum. Ref:
http://src.chromium.org/viewvc/chrome/trunk/tools/depot_tools/presubmit_canned_checks.py?view=markup

Proximately, this is in reply to Erik's request:
https://codereview.chromium.org/19047003/#msg5

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=155448

[Nils Barth (inactive), 2013-08-02 05:42:54]

Hiyas,
Here's a presubmit check so we'll never forget to run run-bindings-tests again!

[haraken, 2013-08-02 05:45:20]

Given that run-bindings-tests is heavy, we want to run it only when IDL files or
IDL compiler is touched.

[Nils Barth (inactive), 2013-08-02 05:48:05]

On 2013/08/02 05:45:20, haraken wrote:
> Given that run-bindings-tests is heavy, we want to run it only when IDL files
or
> IDL compiler is touched.

This is run only for changes to Source/bindings, not for changes outside it.
Agreed, we don't want changes to Source/core to run this!

[haraken, 2013-08-02 05:53:11]

ah, thanks. That's great.

The CL looks good to me, but other guys who are familiar with presubmit.py
should take a look.

[Nils Barth (inactive), 2013-08-02 05:58:07]

On 2013/08/02 05:53:11, haraken wrote:
> ah, thanks. That's great.
> 
> The CL looks good to me, but other guys who are familiar with presubmit.py
> should take a look.

I'll ask abarth@ and marja@, as they're to blame for the only PRESUBMIT.py
script currently in Blink.

[Nils Barth (inactive), 2013-08-02 05:58:51]

Adam, Marja, could y'all take a look at this please?
Quick CL to add a presubmit to bindings, but we're new at this
PRESUBMIT.py business.
Thanks!

[do-not-use, 2013-08-02 06:17:18]

The CL looks good to me as well although I am not familiar with presubmit
scripts. I am also very supportive of adding this check. Thanks for working on
this Nils!

[abarth-chromium, 2013-08-02 16:55:54]

I'm not a PRESUBMIT expert.  I mostly cargo-culted the root one from Chromium. 
However, this LGTM.  We can iterate if we mess it up.

[commit-bot: I haz the power, 2013-08-02 16:56:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nbarth@chromium.org/21722003/6001

[commit-bot: I haz the power, 2013-08-02 18:09:02]

Change committed as 155448

[Nils Barth (inactive), 2013-08-05 04:48:22]

On 2013/08/02 16:55:54, abarth wrote:
> I'm not a PRESUBMIT expert.  I mostly cargo-culted the root one from Chromium.

> However, this LGTM.  We can iterate if we mess it up.

Thanks Adam!
I've documented a bit more so these are less of a black art:
https://sites.google.com/a/chromium.org/dev/developers/how-tos/depottools/pre...

[marja, 2013-08-06 16:53:17]

Late lgtm (was ooo)

Random skeptical thought 1: there have been already problems that the presubmit
script is heavy, and this is adding more tests to be ran. :/ Can we do something
about this? Otoh, rebaselining the results all the time (and just the mere act
of remembering to do this) is way worse.

Random skeptical thought 2: what's the benefit of the bindings tests? At least
in my workflow, I just blindly reset the results without looking, if I have
modified the code generator. The tests will catch at least 1) if the generated
code doesn't build -- but in this situation, it's likely that all_webkit won't
build either 2) if the generated code doesn't follow the coding style; in this
case presubmit style check will whine (this has happened to me, but is it a big
enough benefit compared to the downsides (*)?)

(*) Having to rebaseline, time added to presubmit, more conflicts & rebasing of
the CLs needed.

[Nils Barth (inactive), 2013-08-07 02:28:17]

Thanks for the note Marja!

1. We could make run-bindings-tests be a commit-only test, not an upload test.

This would reduce the load, but mean that sometimes we'd only catch it at the
last minute.
Anyway this only happens when making changes to the bindings/ directory, so it's
not too bad.


On 2013/08/06 16:53:17, marja wrote:
> 2: what's the benefit of the bindings tests?

haraken, Christophe: thoughts?

The obvious point being that bindings tests tell us that *something* has
changed,
...and record in the history when specific output changed.
E.g., if you see something wrong in the generated code, you can look at the
history of the expectations to see when that changed happened, instead of
having to figure out the code generator.
In principle the expectation changes should be part of the review,
which is a good reason for making sure these are checked early.

The most important case is when behavior changes but it still compiles.
I'm not sure how common this is though; as you say, usually if there's a problem
it just doesn't build.

That said, if the expectations are unreadable, that's a problem;
having smaller unit tests might help, but that's a lot of work.

Regarding style checker:
Yeah, we'll likely turn off the style check in future, since it complicates the
code generator.
(Imagine if we moved to Chromium style and had 80-character line length limits!)
haraken suggested as much over here:
https://codereview.chromium.org/22514002/#msg3

Of course, we could just run the generated code through clang-format,
but that seems a bit too complicated...

[haraken, 2013-08-07 02:38:18]

>> 2: what's the benefit of the bindings tests?
>
> haraken, Christophe: thoughts?

Bindings tests are for reviewing. It's hard to review code generator changes, so
we want to confirm the changes in the bindings tests. Another benefit is that
you can make sure that you're not making unexpected changes to generated code
when modifying the code generator.

That being said, I agree that we want to improve the architecture of the
bindings tests. It's not a good situation that some developers are rebaselining
the test results blindly. Practically speaking, what is important is not what
changes are made to bindings tests but what changes are made to actual generated
code. Probably it might be a good idea to upstream the actual generated code.

Please start another thread in crbug if you want to keep the discussion.

[marja, 2013-08-07 07:08:06]

Having this check at upload time is good, since that's the moment when the
developer is in the right mental context to run the results reset. Commit-time
is too late (especially when it's caught by the commit bot).

I started crbug.com/269299 for discussion.