Re: Proposal: Use decorators in test suite for XFail, Skip, etc

On Fri, Feb 4, 2011 at 2:11 PM, Hyrum K Wright <hyrum_at_hyrumwright.org> wrote:
> On Fri, Feb 4, 2011 at 7:03 PM, Blair Zajac <blair_at_orcaware.com> wrote:
>>
>> On Feb 4, 2011, at 9:15 AM, Hyrum K Wright wrote:
>>
>>> We currently mark tests XFail (or Skip, or something else) by wrapping
>>> them in the test_list in the test suite.  Rather than doing it there,
>>> I think it makes more sense to use Python's decorator syntax to mark
>>> tests as XFail right at their definition, rather than down in the test
>>> list.  Keeping all attributes of a test in close proximity is a Good
>>> Thing, imo.  Attached is a patch which demonstrates this.
>>>
>>> Decorators were added to Python in 2.4, which is the minimal version
>>> required for our test suite.  In addition to the functional
>>> decorators, we should be able to add ones which record other
>>> information, such as the issues which the tests were added for.  (In
>>> some future world, we could also divide up the test suite into
>>> "levels", and decorators could be added to indicate that.)
>>>
>>> Thoughts?
>>
>> Sounds good to me.
>>
>> The decorators could have a required issue version number as a parameter, thereby forcing an issue to exist in the issue tracker.
>
> I don't think we should require all tests to have an issue number
> associated with them,

Not *every* issue, I agree, but perhaps requiring *XFails* to have an
associated issue wouldn't be such a bad idea (once we have issues
associated with all the current XFails of course). It would certainly
make the "What XFailing tests are release blockers?" question a lot
easier to answer.

Paul

> but having this information programatically is a Good Thing.
> I've updated the patch to introduce an "issues"
> decorator, and resolve conflicts introduced by Paul's recent commit.
>
> [I plan on cleaning up references and the like before committing.]
>
> -Hyrum
>
> [[[
>
> Index: subversion/tests/cmdline/svntest/testcase.py
> ===================================================================
> --- subversion/tests/cmdline/svntest/testcase.py        (revision 1067239)
> +++ subversion/tests/cmdline/svntest/testcase.py        (working copy)
> @@ -108,6 +108,13 @@ class TestCase:
>     """
>     return self._delegate.get_sandbox_name()
>
> +  def set_issues(self, issues):
> +    """Set the issues associated with this test."""
> +    if type(issues) == type(0):
> +      self.issues = [issues]
> +    else:
> +      self.issues = issues
> +
>   def run(self, sandbox):
>     """Run the test within the given sandbox."""
>     return self._delegate.run(sandbox)
> @@ -134,7 +141,7 @@ class FunctionTestCase(TestCase):
>   is derived from the file name in which FUNC was defined)
>   """
>
> -  def __init__(self, func):
> +  def __init__(self, func, issues=None):
>     # it better be a function that accepts an sbox parameter and has a
>     # docstring on it.
>     assert isinstance(func, types.FunctionType)
> @@ -158,7 +165,7 @@ class FunctionTestCase(TestCase):
>     assert doc[0].lower() == doc[0], \
>         "%s's docstring should not be capitalized" % name
>
> -    TestCase.__init__(self, doc=doc)
> +    TestCase.__init__(self, doc=doc, issues=issues)
>     self.func = func
>
>   def get_function_name(self):
> @@ -207,6 +214,27 @@ class XFail(TestCase):
>     return self._delegate.list_mode() or 'XFAIL'
>
>
> +def xfail_deco(func):
> +  if isinstance(func, TestCase):
> +    return XFail(func, issues=func.issues)
> +  else:
> +    return XFail(func)
> +
> +
> +def issue_deco(issues):
> +  def _second(func):
> +    if isinstance(func, TestCase):
> +      # if the wrapped thing is already a test case, just set the issues
> +      func.set_issues(issues)
> +      return func
> +
> +    else:
> +      # we need to wrap the function
> +      return create_test_case(func, issues=issues)
> +
> +  return _second
> +
> +
>  class Wimp(XFail):
>   """Like XFail, but indicates a work-in-progress: an unexpected pass
>   is not considered a test failure."""
> @@ -259,8 +287,8 @@ class SkipUnless(Skip):
>     Skip.__init__(self, test_case, lambda c=cond_func: not c())
>
>
> -def create_test_case(func):
> +def create_test_case(func, issues=None):
>   if isinstance(func, TestCase):
>     return func
>   else:
> -    return FunctionTestCase(func)
> +    return FunctionTestCase(func, issues=issues)
> Index: subversion/tests/cmdline/basic_tests.py
> ===================================================================
> --- subversion/tests/cmdline/basic_tests.py     (revision 1067239)
> +++ subversion/tests/cmdline/basic_tests.py     (working copy)
> @@ -1961,6 +1961,8 @@ def basic_rm_urls_one_repo(sbox):
>                                         expected_status)
>
>  # Test for issue #1199
> +@svntest.testcase.xfail_deco
> +@svntest.testcase.issue_deco(1199)
>  def basic_rm_urls_multi_repos(sbox):
>   "remotely remove directories from two repositories"
>
> @@ -2721,7 +2723,7 @@ test_list = [ None,
>               delete_keep_local_twice,
>               windows_paths_in_repos,
>               basic_rm_urls_one_repo,
> -              XFail(basic_rm_urls_multi_repos, issues=1199),
> +              basic_rm_urls_multi_repos,
>               automatic_conflict_resolution,
>               info_nonexisting_file,
>               basic_relative_url_using_current_dir,
> ]]]
>
Received on 2011-02-04 20:21:06 CET