Created infra/tools/new_tool

Created infra/tools/new_tool

Use this tool to generate a stub in infra/tools/<toolname>

BUG=498379
Committed: https://chromium.googlesource.com/infra/infra/+/40b336ae8afa6ade68584e79dab9ee2be5fa7c40

[pgervais, 2015-06-09 22:44:56]

pgervais@chromium.org changed reviewers:
+ hinoka@chromium.org, sergeyberezin@chromium.org

[pgervais, 2015-06-09 22:44:57]

hinoka: This is what I used to generate the Antibody stub.

[Ryan Tseng, 2015-06-09 23:03:04]

hinoka@google.com changed reviewers:
+ hinoka@google.com

[Ryan Tseng, 2015-06-09 23:03:05]

https://chromiumcodereview.appspot.com/1172053003/diff/1/infra/tools/new_tool...
File infra/tools/new_tool/new_tool.py (right):

https://chromiumcodereview.appspot.com/1172053003/diff/1/infra/tools/new_tool...
infra/tools/new_tool/new_tool.py:1: # Copyright 2015 The Chromium Authors. All
rights reserved.
I think i'd prefer a filename.template file with a bunch of %(foobar)s filled in
by this file.

[Sergey Berezin, 2015-06-10 00:32:37]

Overall looks good. One comment, and please add tests.

https://chromiumcodereview.appspot.com/1172053003/diff/1/infra/tools/new_tool...
File infra/tools/new_tool/new_tool.py (right):

https://chromiumcodereview.appspot.com/1172053003/diff/1/infra/tools/new_tool...
infra/tools/new_tool/new_tool.py:159: ##     return hash(MyTest)
nit: this looks confusing to me. Why would I want my test to fail?

I'd add a copy of test_arguments() rewritten to use expectations (e.g. returning
args.my_argument).

[pgervais, 2015-06-10 00:57:20]

Alright, by refactoring. The code is shorter, and there are now separate
template files. ptal

[Sergey Berezin, 2015-06-10 23:00:42]

Looks great, looking forward to the tests :-)

[pgervais, 2015-06-11 17:09:03]

On 2015/06/10 23:00:42, Sergey Berezin wrote:
> Looks great, looking forward to the tests :-)

Tests added. ptal

[Sergey Berezin, 2015-06-11 17:33:17]

Thanks for the tests! Looks pretty good.

Can we also have a smoke test for __main__.py please? :-) It'd also take care of
the 'no cover' comment below.

https://codereview.chromium.org/1172053003/diff/40001/infra/tools/new_tool/ne...
File infra/tools/new_tool/new_tool.py (right):

https://codereview.chromium.org/1172053003/diff/40001/infra/tools/new_tool/ne...
infra/tools/new_tool/new_tool.py:19: def add_argparse_options(parser):  #
pragma: no cover
why no cover? I'd add at least a smoke test, just to avoid silly typos in this
code.

https://codereview.chromium.org/1172053003/diff/40001/infra/tools/new_tool/te...
File infra/tools/new_tool/test/new_tool_test.py (right):

https://codereview.chromium.org/1172053003/diff/40001/infra/tools/new_tool/te...
infra/tools/new_tool/test/new_tool_test.py:76: self.assertEqual(content,
expected_content)
nit: this is effectively an encoded expectation. Maybe use actual expectations
here?

[pgervais, 2015-06-11 17:46:54]

Tested the rest. ptal

https://codereview.chromium.org/1172053003/diff/40001/infra/tools/new_tool/te...
File infra/tools/new_tool/test/new_tool_test.py (right):

https://codereview.chromium.org/1172053003/diff/40001/infra/tools/new_tool/te...
infra/tools/new_tool/test/new_tool_test.py:76: self.assertEqual(content,
expected_content)
On 2015/06/11 17:33:16, Sergey Berezin wrote:
> nit: this is effectively an encoded expectation. Maybe use actual expectations
> here?

Several reasons:
- this expectation is not supposed to change.
- if something changed, it's hard for someone not familiar with the test to tell
whether the change is valid or not. 
- training the tests adds some overhead. I don't see any large benefit that
would compensate for this.

[Sergey Berezin, 2015-06-11 17:55:06]

Cool, thx! LGTM

https://codereview.chromium.org/1172053003/diff/40001/infra/tools/new_tool/te...
File infra/tools/new_tool/test/new_tool_test.py (right):

https://codereview.chromium.org/1172053003/diff/40001/infra/tools/new_tool/te...
infra/tools/new_tool/test/new_tool_test.py:76: self.assertEqual(content,
expected_content)
On 2015/06/11 17:46:54, pgervais wrote:
> On 2015/06/11 17:33:16, Sergey Berezin wrote:
> > nit: this is effectively an encoded expectation. Maybe use actual
expectations
> > here?
> 
> Several reasons:
> - this expectation is not supposed to change.
> - if something changed, it's hard for someone not familiar with the test to
tell
> whether the change is valid or not. 
> - training the tests adds some overhead. I don't see any large benefit that
> would compensate for this.

Acknowledged.

[pgervais, 2015-06-11 18:03:44]

The CQ bit was checked by pgervais@chromium.org

[commit-bot: I haz the power, 2015-06-11 18:04:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1172053003/60001

[commit-bot: I haz the power, 2015-06-11 18:17:10]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/infra/infra/+/40b336ae8afa6ade68584e79dab9e...