On Fri, Feb 4, 2011 at 2:11 PM, Hyrum K Wright <hy...@hyrumwright.org> wrote: > On Fri, Feb 4, 2011 at 7:03 PM, Blair Zajac <bl...@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, > ]]] >
