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

[Dylan Baker]

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
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20150316/a0b0cace/attachment.sig>