Whitelist allowed test names

Whitelist allowed test names

Forces test names to be only alphanumeric. Works around a bug in depot_tools
patching logic, which didn't like filenames with spaces.

BUG=693779

[martiniss, 2017-02-18 00:08:09]

Description was changed from

==========
Whitelist allowed test names

Forces test names to be only alphanumeric. Helps with a bug in depot_tools
patching logic, which didn't like filenames with spaces.

BUG=693779
==========

to

==========
Whitelist allowed test names

Forces test names to be only alphanumeric. Works around a bug in depot_tools
patching logic, which didn't like filenames with spaces.

BUG=693779
==========

[martiniss, 2017-02-18 00:08:09]

martiniss@chromium.org changed reviewers:
+ dnj@chromium.org, iannucci@chromium.org, phajdan.jr@chromium.org

[martiniss, 2017-02-18 00:11:20]

PTAL

[dnj, 2017-02-18 00:15:29]

https://codereview.chromium.org/2707593002/diff/20001/recipe_engine/recipe_te...
File recipe_engine/recipe_test_api.py (right):

https://codereview.chromium.org/2707593002/diff/20001/recipe_engine/recipe_te...
recipe_engine/recipe_test_api.py:193: match = re.match(r'(\w)*', name)
Why not: "name.isalnum()"? Also will reject whitespaces.

[martiniss, 2017-02-18 00:25:13]

On 2017/02/18 at 00:15:29, dnj wrote:
>
https://codereview.chromium.org/2707593002/diff/20001/recipe_engine/recipe_te...
> File recipe_engine/recipe_test_api.py (right):
> 
>
https://codereview.chromium.org/2707593002/diff/20001/recipe_engine/recipe_te...
> recipe_engine/recipe_test_api.py:193: match = re.match(r'(\w)*', name)
> Why not: "name.isalnum()"? Also will reject whitespaces.

Doesn't match underscore. I could split on underscore though.

[dnj, 2017-02-18 01:27:41]

https://codereview.chromium.org/2707593002/diff/20001/recipe_engine/recipe_te...
File recipe_engine/recipe_test_api.py (right):

https://codereview.chromium.org/2707593002/diff/20001/recipe_engine/recipe_te...
recipe_engine/recipe_test_api.py:193: match = re.match(r'(\w)*', name)
On 2017/02/18 00:15:29, dnj wrote:
> Why not: "name.isalnum()"? Also will reject whitespaces.

Ah boo :(

Could do:
re.match(r'[a-zA-Z0-9_]*')

I really just don't like the whitespace check at the end.

[iannucci, 2017-02-18 10:08:20]

won't this patching logic be irrelevant if we switch to gerrit?

[Paweł Hajdan Jr., 2017-02-21 09:53:31]

LGTM w/comments

IMO this is a good change even when we switch to Gerrit. Limiting valid test
names helps avoid a class of problems, and I don't see a big benefit of allowing
weird test names.

https://codereview.chromium.org/2707593002/diff/20001/recipe_engine/recipe_te...
File recipe_engine/recipe_test_api.py (right):

https://codereview.chromium.org/2707593002/diff/20001/recipe_engine/recipe_te...
recipe_engine/recipe_test_api.py:191: # * is there to make the match greedy, so
it matches the whole word, rather
nit: No need to explain regex syntax IMO.

https://codereview.chromium.org/2707593002/diff/20001/recipe_engine/recipe_te...
recipe_engine/recipe_test_api.py:193: match = re.match(r'(\w)*', name)
Let's use ^, $, and +, i.e.

r'^(\w)+$'

Then we don't need check at the end, and we also don't match an empty string:

%>>> re.match(r'(\w)*', ' ')
%<_sre.SRE_Match object at 0x101c92030>
%>>> re.match(r'^(\w)+$', ' ')
%>>>

%>>> re.match(r'(\w)*', '')
%<_sre.SRE_Match object at 0x101c8fb70>
%>>> re.match(r'^(\w)+$', '')
%>>>

https://codereview.chromium.org/2707593002/diff/20001/recipe_engine/recipe_te...
recipe_engine/recipe_test_api.py:207: raise ValueError("Invalid test name %r" %
name)
nit: Single quotes ' instead of double "

Also, explain what valid is (alphanumeric and underscores only).

https://codereview.chromium.org/2707593002/diff/20001/recipe_engine/unittests...
File recipe_engine/unittests/recipe_test_api_test.py (right):

https://codereview.chromium.org/2707593002/diff/20001/recipe_engine/unittests...
recipe_engine/unittests/recipe_test_api_test.py:20: test_data =
recipe_test_api.TestData("test")
nit: Single quotes ' instead of double " (applies to entire CL).

[martiniss, 2017-02-21 18:50:29]

On 2017/02/18 at 10:08:20, iannucci wrote:
> won't this patching logic be irrelevant if we switch to gerrit?

I think so? I agree with pawel though, that I think restricting test names makes
sense anyways.

[Paweł Hajdan Jr., 2017-02-22 17:15:22]

On 2017/02/21 18:50:29, martiniss wrote:
> On 2017/02/18 at 10:08:20, iannucci wrote:
> > won't this patching logic be irrelevant if we switch to gerrit?
> 
> I think so? I agree with pawel though, that I think restricting test names
makes
> sense anyways.

Especially that we have the CL now.

To me it means we don't need to ask whether the work is worth it, since it's
already done.

Any possible remaining questions might be about code complexity or related
matters, but IMO we do so many other really complex things, that this CL
shouldn't raise such concerns.

I suggest we land this CL, unless someone has a really strong opinion (and
preferably, equally strong justification).