[Piglit] [PATCH 7/7] grouptools.py: assert if \\ or / is in a group

[Jose Fonseca]

I see.

Now that we use a different character as seperator, there's not much 
harm in having '/' in test name/group.  At least, as far junit backend 
is concerned.  It might cause confusion in json though.

One solution would be to replace with `assert '/' not in foo` with a 
warning to stderr.

Another would be to continue to forbid '/' and '\\' in test names paths, 
and just replace them with something else, like underscore '_', or 
'<slash>', '<div>'

Jose

On 16/03/15 23:13, Dylan Baker wrote:
> So, I did a full run before I pushed this, and discovered an interesting
> issue.
>
> Some test have '/' in their names. This makes a lot of interesting
> problems. For one, the update pass I've written is wrong, since it
> naively replaces all '/' and '\\' with '@', which means that tests could
> end up with different names before and after.
>
> It also means that the asserts here are not safe to use.
>
> I guess I'll send out a v2 later today when I decide what the right way
> to handle this is.
>
> Dylan
>
> On Thu, Mar 12, 2015 at 03:42:10PM -0700, Dylan Baker wrote:
>> This removes the _normalize function and adds an assertion for \\ or for
>> /
>>
>> It also adds some tests for the assertions in grouptools.
>>
>> Signed-off-by: Dylan Baker <dylanx.c.baker at intel.com>
>> ---
>>   framework/grouptools.py             | 27 +++-------------------
>>   framework/tests/grouptools_tests.py | 46 +++++++++++++++++++++++++++++++++++++
>>   2 files changed, 49 insertions(+), 24 deletions(-)
>>
>> diff --git a/framework/grouptools.py b/framework/grouptools.py
>> index abe52cf..d6fc921 100644
>> --- a/framework/grouptools.py
>> +++ b/framework/grouptools.py
>> @@ -29,8 +29,6 @@ posix paths they may not start with a leading '/'.
>>
>>   """
>>
>> -import os.path
>> -
>>   __all__ = [
>>       'commonprefix',
>>       'from_path',
>> @@ -44,27 +42,14 @@ __all__ = [
>>   SEPERATOR = '@'
>>
>>
>> -def _normalize(group):
>> -    """Helper to normalize group paths on Windows.
>> -
>> -    Although grouptools' heart is in the right place, the fact is that it fails
>> -    to spot when developers mistakedly use os.path.join for groups on Posix
>> -    systems.
>> -
>> -    So until this is improved somehow, make grouptools behavior on Windows
>> -    match Linux, ie, just silently ignore mixed use of grouptools and os.path.
>> -    """
>> -    if os.path.sep != '/':
>> -        group = group.replace(os.path.sep, '/')
>> -    return group
>> -
>> -
>>   def _assert_illegal(group):
>>       """Helper that checks for illegal characters in input."""
>>       assert isinstance(group, (str, unicode)), \
>>           'Type must be string or unicode but was {}'.format(type(group))
>>       assert '\\' not in group, \
>>           'Groups are not paths and cannot contain \\.  ({})'.format(group)
>> +    assert '/' not in group, \
>> +        'Groups are not paths and cannot contain /.  ({})'.format(group)
>>
>>
>>   def testname(group):
>> @@ -77,7 +62,6 @@ def testname(group):
>>       Analogous to os.path.basename
>>
>>       """
>> -    group = _normalize(group)
>>       _assert_illegal(group)
>>
>>       return splitname(group)[1]
>> @@ -93,7 +77,6 @@ def groupname(group):
>>       Analogous to os.path.dirname
>>
>>       """
>> -    group = _normalize(group)
>>       _assert_illegal(group)
>>
>>       return splitname(group)[0]
>> @@ -101,7 +84,6 @@ def groupname(group):
>>
>>   def splitname(group):
>>       """Split a group name, Returns tuple "(group, test)"."""
>> -    group = _normalize(group)
>>       _assert_illegal(group)
>>
>>       i = group.rfind(SEPERATOR) + 1
>> @@ -113,7 +95,6 @@ def splitname(group):
>>
>>   def commonprefix(args):
>>       """Given a list of groups, returns the longest common leading component."""
>> -    args = [_normalize(group) for group in args]
>>       for group in args:
>>           _assert_illegal(group)
>>
>> @@ -148,11 +129,10 @@ def join(first, *args):
>>       conincedently very similar to the way posixpath.join is implemented.
>>
>>       """
>> -    args = [_normalize(group) for group in args]
>> -    first = _normalize(first)
>>       _assert_illegal(first)
>>       for group in args:
>>           _assert_illegal(group)
>> +
>>           # Only append things if the group is not empty, otherwise we'll get
>>           # extra SEPERATORs where we don't want them
>>           if group:
>> @@ -169,7 +149,6 @@ def split(group):
>>       If input is '' return an empty list
>>
>>       """
>> -    group = _normalize(group)
>>       _assert_illegal(group)
>>       if group == '':
>>           return []
>> diff --git a/framework/tests/grouptools_tests.py b/framework/tests/grouptools_tests.py
>> index a8b55fb..5377749 100644
>> --- a/framework/tests/grouptools_tests.py
>> +++ b/framework/tests/grouptools_tests.py
>> @@ -60,6 +60,52 @@ def generate_tests():
>>           yield test, func, args, expect
>>
>>
>> + at utils.nose_generator
>> +def generate_error_tests():
>> +    """Generate tests for the various failure problems."""
>> +
>> +    @nt.raises(AssertionError)
>> +    def test(func, args):
>> +        """The input must be either a str or unicode."""
>> +        func(args)
>> +
>> +    tests = [
>> +        ('testname', grouptools.testname),
>> +        ('groupname', grouptools.groupname),
>> +        ('splitname', grouptools.splitname),
>> +        ('split', grouptools.split),
>> +        ('join', grouptools.join),
>> +    ]
>> +
>> +    for name, func in tests:
>> +        test.description = \
>> +            'grouptools.{}: raises an error if args are invalid'.format(name)
>> +        # This must be a list of lists to trigger commonprefix
>> +        yield test, func, ['foo']
>> +
>> +        test.description = \
>> +            'grouptools.{}: raises an error if / is in the args'.format(name)
>> +        yield test, func, 'foo/bar'
>> +
>> +        test.description = \
>> +            'grouptools.{}: raises an error if \\ is in the args'.format(name)
>> +        yield test, func, 'foo\\bar'
>> +
>> +    # commonprefix takes a list of things rather than things, so it must be
>> +    # tested separately
>> +    test.description = \
>> +        'grouptools.commonprefix: raises an error if args are invalid'
>> +    yield test, grouptools.commonprefix, [['foo']]
>> +
>> +    test.description = \
>> +        'grouptools.commonprefix: raises an error if / is in args'
>> +    yield test, grouptools.commonprefix, ['foo/bar', 'boink']
>> +
>> +    test.description = \
>> +        'grouptools.commonprefix: raises an error if \\ is in args'
>> +    yield test, grouptools.commonprefix, ['foo\\bar', 'boink']
>> +
>> +
>>   def test_grouptools_join():
>>       """grouptools.join: works correctly."""
>>       # XXX: this hardcoded / needs to be fixed
>> --
>> 2.3.1