Add first bindings unit test and enable testing.

Add first bindings unit test and enable testing.

Also:
* Quick refactor of jinja2 context creation into a builder.
* Switched Attribute/Method to dicts.
* Added a bit more meat more to header template.
* Added nicer parameter handling to run-bindings-tests
* Added stub helper for unit tests.

BUG=654129
R=bashi,yukishiino,haraken

Committed: https://crrev.com/0c7a8ec99ddb52ef1e0db1423b7a774f85506bc2
Cr-Commit-Position: refs/heads/master@{#437989}

[dglazkov, 2016-12-12 03:54:03]

Add harness code after typ roll.

[dglazkov, 2016-12-12 03:54:04]

The CQ bit was checked by dglazkov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 03:54:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dglazkov, 2016-12-12 03:55:37]

Description was changed from

==========
Add first bindings unit test and enable testing.

Also:
* Quick refactor of jinja2 context creation into a builder.
* Switched Attribute/Method to dicts.
* Added a bit more meat more to header template.

BUG=654129
R=bashi,yukishiino,haraken
==========

to

==========
Add first bindings unit test and enable testing.

Also:
* Quick refactor of jinja2 context creation into a builder.
* Switched Attribute/Method to dicts.
* Added a bit more meat more to header template.
* Added nicer parameter handling to run-bindings-tests
* Added stub helper for unit tests.

BUG=654129
R=bashi,yukishiino,haraken
==========

[dglazkov, 2016-12-12 03:55:48]

PTAL.

[haraken, 2016-12-12 04:20:08]

LGTM

https://codereview.chromium.org/2568693002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/tests/results/core/WebTestInterface3.cpp
(right):

https://codereview.chromium.org/2568693002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/tests/results/core/WebTestInterface3.cpp:14:
#include "WebIterator.h"

These header files look strange though; e.g., Webvoid.h should not exist.

[peria, 2016-12-12 04:43:09]

peria@chromium.org changed reviewers:
+ peria@chromium.org

[peria, 2016-12-12 04:43:10]

https://codereview.chromium.org/2568693002/diff/20001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/run-bindings-tests (right):

https://codereview.chromium.org/2568693002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/run-bindings-tests:43: argument_parser =
typ.ArgumentParser()
optional:
I prefer to put a routine to parse options into a separate function.

https://codereview.chromium.org/2568693002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/run-bindings-tests:57: # First, run bindings
unit tests.
Why don't you use typ.main() as you did in
https://codereview.chromium.org/2554503004/?

I think it good to check if one or more tests exist,
but it is a bit too aggressive to run tests directly.

[dglazkov, 2016-12-12 05:01:41]

On 2016/12/12 at 04:20:08, haraken wrote:
> LGTM
> 
>
https://codereview.chromium.org/2568693002/diff/20001/third_party/WebKit/Sour...
> File
third_party/WebKit/Source/bindings/tests/results/core/WebTestInterface3.cpp
(right):
> 
>
https://codereview.chromium.org/2568693002/diff/20001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/bindings/tests/results/core/WebTestInterface3.cpp:14:
#include "WebIterator.h"
> 
> These header files look strange though; e.g., Webvoid.h should not exist.

Yeah, but not for long. I am coming for them :)

[bashi, 2016-12-12 05:04:38]

bashi@chromium.org changed reviewers:
+ bashi@chromium.org

[bashi, 2016-12-12 05:04:38]

This CL lgtm (but I have one question).

https://codereview.chromium.org/2568693002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/code_generator_web_module.py
(right):

https://codereview.chromium.org/2568693002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/code_generator_web_module.py:81:
return_type = idl_operation.idl_type.preprocessed_type.base_type
Not directly related to this CL but do we need preprocessed_type? Maybe we need
a function like "return_type_for_type(idl_type)" instead? (As you did for
includes_for_type())

|preprocessed_type| is installed in v8_types.py, which means that it includes V8
specific logic. I'm not sure what's a right value of 'return_type' for web
modules.

https://codereview.chromium.org/2568693002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/scripts/code_generator_web_module_test.py
(right):

https://codereview.chromium.org/2568693002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/code_generator_web_module_test.py:15:
"""A collection of stub makers and helper utils to make testing code
Using stubs for definitions sounds a good idea but I'm a bit worrying about
we'll end up creating almost the same objects of IdlType, IdlAttribute and
IdlOperation here. At some point we may want to use actual instances of those
classes. I think we can do that by adding default constructors which don't take
AST nodes. For example:

class IdlAttribute(TypedObject):
  def __init__(self, node=None):
    ...
    self.name = None
    if node:
      self.name = node.GetName()
    ...

def make_stub_idl_attribute(self, name, return_type):
  attribute = IdlAttribute()
  attribute.name = name
  attribute.idl_type = IdlType(return_type)
  return attribute

[dglazkov, 2016-12-12 05:08:22]

dglazkov@chromium.org changed reviewers:
- bashi@chromium.org

[dglazkov, 2016-12-12 05:08:22]

https://codereview.chromium.org/2568693002/diff/20001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/run-bindings-tests (right):

https://codereview.chromium.org/2568693002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/run-bindings-tests:43: argument_parser =
typ.ArgumentParser()
On 2016/12/12 at 04:43:10, peria wrote:
> optional:
> I prefer to put a routine to parse options into a separate function.

SG, I'll think of a way to break it up better.

https://codereview.chromium.org/2568693002/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/run-bindings-tests:57: # First, run bindings
unit tests.
On 2016/12/12 at 04:43:10, peria wrote:
> Why don't you use typ.main() as you did in
https://codereview.chromium.org/2554503004/?
> 
> I think it good to check if one or more tests exist,
> but it is a bit too aggressive to run tests directly.

Ah, turns out, I can't. I need to effectively break apart typ.main, since I need
to inform typ that the embedding app has additional arguments (--reset-results),
and feed it its own argument parser. First, I had to go teach typ that this is
possible
(https://github.com/dpranke/typ/commit/c8845b2354afb6350244beb2d4430da3a2b203e3)
and Dirk graciously rolled it in to chromium over the weekend. Nesting yak
shaves! :)

I didn't realize this was necessary in
https://codereview.chromium.org/2554503004/, not until I actually tried using
--reset-results :D

Otherwise, supplying "--reset-results" as a command argument results in typ
throwing an exception, complaining of unknown argument.

[dglazkov, 2016-12-12 05:18:21]

On 2016/12/12 at 05:04:38, bashi wrote:
> This CL lgtm (but I have one question).
> 
>
https://codereview.chromium.org/2568693002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/scripts/code_generator_web_module.py
(right):
> 
>
https://codereview.chromium.org/2568693002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/scripts/code_generator_web_module.py:81:
return_type = idl_operation.idl_type.preprocessed_type.base_type
> Not directly related to this CL but do we need preprocessed_type? Maybe we
need a function like "return_type_for_type(idl_type)" instead? (As you did for
includes_for_type())
> 
> |preprocessed_type| is installed in v8_types.py, which means that it includes
V8 specific logic. I'm not sure what's a right value of 'return_type' for web
modules.
> 
>
https://codereview.chromium.org/2568693002/diff/20001/third_party/WebKit/Sour...
> File
third_party/WebKit/Source/bindings/scripts/code_generator_web_module_test.py
(right):
> 
>
https://codereview.chromium.org/2568693002/diff/20001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/bindings/scripts/code_generator_web_module_test.py:15:
"""A collection of stub makers and helper utils to make testing code
> Using stubs for definitions sounds a good idea but I'm a bit worrying about
we'll end up creating almost the same objects of IdlType, IdlAttribute and
IdlOperation here. At some point we may want to use actual instances of those
classes. I think we can do that by adding default constructors which don't take
AST nodes. For example:
> 
> class IdlAttribute(TypedObject):
>   def __init__(self, node=None):
>     ...
>     self.name = None
>     if node:
>       self.name = node.GetName()
>     ...
> 
> def make_stub_idl_attribute(self, name, return_type):
>   attribute = IdlAttribute()
>   attribute.name = name
>   attribute.idl_type = IdlType(return_type)
>   return attribute

That sounds amazing. I was daunted by the complexity in idl_definitions.py, and
chickened out. I agree actual instances are better.

[dglazkov, 2016-12-12 05:18:45]

Filed http://crbug.com/673213 and http://crbug.com/673214 as follow-ups.

[bashi, 2016-12-12 05:19:55]

Description was changed from

==========
Add first bindings unit test and enable testing.

Also:
* Quick refactor of jinja2 context creation into a builder.
* Switched Attribute/Method to dicts.
* Added a bit more meat more to header template.
* Added nicer parameter handling to run-bindings-tests
* Added stub helper for unit tests.

BUG=654129
R=bashi,yukishiino,haraken
==========

to

==========
Add first bindings unit test and enable testing.

Also:
* Quick refactor of jinja2 context creation into a builder.
* Switched Attribute/Method to dicts.
* Added a bit more meat more to header template.
* Added nicer parameter handling to run-bindings-tests
* Added stub helper for unit tests.

BUG=654129
R=bashi,yukishiino,haraken
==========

[bashi, 2016-12-12 05:19:55]

bashi@chromium.org changed reviewers:
+ yukishiino@chromium.org

[bashi, 2016-12-12 05:20:37]

On 2016/12/12 05:18:45, dglazkov wrote:
> Filed http://crbug.com/673213 and http://crbug.com/673214 as follow-ups.

Thank you!

(+yukishiino@ just in case)

[commit-bot: I haz the power, 2016-12-12 05:24:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-12 05:24:17]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[peria, 2016-12-12 05:35:18]

On 2016/12/12 05:08:22, dglazkov wrote:
>
https://codereview.chromium.org/2568693002/diff/20001/third_party/WebKit/Tool...
> File third_party/WebKit/Tools/Scripts/run-bindings-tests (right):
> 
>
https://codereview.chromium.org/2568693002/diff/20001/third_party/WebKit/Tool...
> third_party/WebKit/Tools/Scripts/run-bindings-tests:43: argument_parser =
> typ.ArgumentParser()
> On 2016/12/12 at 04:43:10, peria wrote:
> > optional:
> > I prefer to put a routine to parse options into a separate function.
> 
> SG, I'll think of a way to break it up better.
> 
>
https://codereview.chromium.org/2568693002/diff/20001/third_party/WebKit/Tool...
> third_party/WebKit/Tools/Scripts/run-bindings-tests:57: # First, run bindings
> unit tests.
> On 2016/12/12 at 04:43:10, peria wrote:
> > Why don't you use typ.main() as you did in
> https://codereview.chromium.org/2554503004/
> > 
> > I think it good to check if one or more tests exist,
> > but it is a bit too aggressive to run tests directly.
> 
> Ah, turns out, I can't. I need to effectively break apart typ.main, since I
need
> to inform typ that the embedding app has additional arguments
(--reset-results),
> and feed it its own argument parser. First, I had to go teach typ that this is
> possible
>
(https://github.com/dpranke/typ/commit/c8845b2354afb6350244beb2d4430da3a2b203e3)
> and Dirk graciously rolled it in to chromium over the weekend. Nesting yak
> shaves! :)
> 
> I didn't realize this was necessary in
> https://codereview.chromium.org/2554503004/, not until I actually tried using
> --reset-results :D
> 
> Otherwise, supplying "--reset-results" as a command argument results in typ
> throwing an exception, complaining of unknown argument.

Sad yak shaves..:(
lgtm

[Yuki, 2016-12-12 07:12:38]

LGTM deferring details to bashi@.

[dglazkov, 2016-12-13 02:19:48]

The CQ bit was checked by dglazkov@chromium.org

[commit-bot: I haz the power, 2016-12-13 02:20:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 02:27:41]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481595588506970,
"parent_rev": "f60dd52e5e13f4eefe39a31b56875e2b36bc1046", "commit_rev":
"c8f6f73523186c3b5c93b93604b7f06a9e609009"}

[commit-bot: I haz the power, 2016-12-13 02:28:31]

Description was changed from

==========
Add first bindings unit test and enable testing.

Also:
* Quick refactor of jinja2 context creation into a builder.
* Switched Attribute/Method to dicts.
* Added a bit more meat more to header template.
* Added nicer parameter handling to run-bindings-tests
* Added stub helper for unit tests.

BUG=654129
R=bashi,yukishiino,haraken
==========

to

==========
Add first bindings unit test and enable testing.

Also:
* Quick refactor of jinja2 context creation into a builder.
* Switched Attribute/Method to dicts.
* Added a bit more meat more to header template.
* Added nicer parameter handling to run-bindings-tests
* Added stub helper for unit tests.

BUG=654129
R=bashi,yukishiino,haraken

Review-Url: https://codereview.chromium.org/2568693002
==========

[commit-bot: I haz the power, 2016-12-13 02:28:32]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-13 02:31:35]

Description was changed from

==========
Add first bindings unit test and enable testing.

Also:
* Quick refactor of jinja2 context creation into a builder.
* Switched Attribute/Method to dicts.
* Added a bit more meat more to header template.
* Added nicer parameter handling to run-bindings-tests
* Added stub helper for unit tests.

BUG=654129
R=bashi,yukishiino,haraken

Review-Url: https://codereview.chromium.org/2568693002
==========

to

==========
Add first bindings unit test and enable testing.

Also:
* Quick refactor of jinja2 context creation into a builder.
* Switched Attribute/Method to dicts.
* Added a bit more meat more to header template.
* Added nicer parameter handling to run-bindings-tests
* Added stub helper for unit tests.

BUG=654129
R=bashi,yukishiino,haraken

Committed: https://crrev.com/0c7a8ec99ddb52ef1e0db1423b7a774f85506bc2
Cr-Commit-Position: refs/heads/master@{#437989}
==========

[commit-bot: I haz the power, 2016-12-13 02:31:36]

Patchset 2 (id:??) landed as
https://crrev.com/0c7a8ec99ddb52ef1e0db1423b7a774f85506bc2
Cr-Commit-Position: refs/heads/master@{#437989}