[MERGE] test framework distinguishes skips

[Martin Pool]

On  6 Jul 2006, Aaron Bentley <aaron.bentley at utoronto.ca> wrote:
> Hi all,
> 
> This patch introduces three new subclasses of TestSkipped, and test case
> decorators to auto-raise them in the right circumstances.  It also
> applies these decorators to our current set of test cases.

That's very nice.

> The fails_with and may_fail_with decorators will raise ExpectedFailure.
>  This indicates that a test failure is expected some or all of the time.
> fails_with can be used to introduce failing test cases without breaking
> the test suite.  may_fail_with supports occasional errors like
> UninitializableFormat, which is permitted for format 0.0.4, but no
> others.  This class of skips cannot be, and is not intended to be, run,
> without changing the source code.
> 
> The dependency_failure decorator indicates that the failure was due to a
> missing dependency, e.g. lsprof or paramiko.  This class of skips can be
> run if the user installs the dependency.

Nice function, but maybe not quite the right name.  How about
@test_requires or @test_dependency?

> The platform_requirement decorator indicates that this test requires
> facilities that are not supported by some platforms.  This class of
> skips can be run on another platform.

I think this might be simpler as a regular method, not a decorator.
Just like

  def test_something_with_symlinks(self):
    self.requires_symlinks()

There's a certain complexity cost to using decorators so I think regular
methods should be preferred if they can do the job.  For the cases where
you need to insert code both before and after the method (such as the
try/except or try/finally) then they're much more reasonable.

> +
> +
> +def platform_requirement(symlinks=None, file_modes=None, unicode=None):
> +    """Throw a PlatformDefect if specified features aren't supported
> +
> +    :param symlinks: Symlink support
> +    :param file modes: Unix file modes
> +    :param unicode: Filesystem and console support unicode
> +    """ 
> +    assert (symlinks is not None or file_modes is not None or 
> +            unicode is not None)
> +    assert symlinks is not False
> +    assert file_modes is not False
> +    assert unicode is not False

This smells like this method is trying to do too many things.  Why not
just make three of them?  (Well, because stacking three decorators might
look bad, but that's kind of a point against decorators.)

Suppose we have a test which creates unicode filenames.  We expect it
to be skipped on systems where you can't do that.  But suppose someone
introduces a bug that causes a UnicodeError in code unrelated to the
filesystem - we won't see the failure because it will just be skipped on
all platforms.

The heart of the problem is that builtin exceptions which can be raised
from many places should be caught in tight scopes, otherwise you're
likely to catch something you didn't want.

So even though the reduction in size is very nice I don't think we
should put that in.

Maybe we can instead have an option to TestCase.build_tree that makes it
skip if theres a unicode error while creating the directory?  That would
cover many cases and be less risky.

For some of these, like lsprof, I'd think you could just check the
dependency in the test class setup, can't you?

> +    def decorator_factory(func):
> +        def decorator(*args, **kwargs):
> +            from bzrlib import tests
> +            if symlinks and not has_symlinks():
> +                raise tests.PlatformDefect('Symlinks are not supported on'
> +                                           ' this platform')
> +            if file_modes and sys.platform == 'win32':
> +                raise tests.PlatformDefect('Unix file modes not supported')
> +            else:
> +                try:
> +                    return func(*args, **kwargs)
> +                except UnicodeError:
> +                    if unicode:
> +                        raise tests.PlatformDefect('Unicode support')
> +                    else:
> +                        raise
> +        return decorator
> +    return decorator_factory

Is it ever really reasonable to catch all UnicodeErrors, rather than
just expecting them for particular calls?

> +    def test_expected_failure(self):
> +        self.assertRaises(ExpectedFailure, self.raises_expected)
> +        self.assertRaises(ExpectedFailure, self.raises_expected_match)
> +        self.assertRaises(errors.BzrCommandError, self.raises_expected_nomatch)
> +        self.assertRaises(errors.BzrCommandError, self.raises_unexpected)
> +        self.assertRaises(errors.FailedFailure, self.doesnt_raise)
> +        self.assertEqual("I didn't raise", self.doesnt_raise_permitted())

Very nice to see this tested this way.

I do like the cleanup a lot but I'd like to resolve those issues before
merging it.

-- 
Martin