Merge lp:~jml/subunit/filter-tags into lp:~subunit/subunit/trunk

[Jonathan Lange]

Add options to filter streams by tags.

This branch adds options to subunit-filter to have it filter by tags. It has three main components:

1. Separating out a _PredicateFilter from TestResultFilter.

This simplifies TestResultFilter, leaving it managing two concerns: assembling options into a predicate and transforming results. It will be much easier to separate out the 'transforming results' concern after this change.

It is likely that _PredicateFilter could also be written in terms of TestByTestResult at some point.

2. Extending the predicate function to take 'tags'.

I've tried to do this in a way that preserves backwards compatibility and allows any users of TestResultFilter who've had a predicate function that does not support tags to continue using their code unchanged. Perhaps this is not needed.

3. Cleaning up subunit-filter

It behaves a little differently to the other filter commands, and has a different set of options, but these cleanups share code where we can and make the differences -- I hope -- more obvious.

None of this works without the fixes to TagCollapsingDecorator.

jml

[lp:~jml/subunit/filter-tags]

185. By Jonathan Lange on 2012-04-20

Factor a TagsMixin out of TagCollapsingDecorator

186. By Jonathan Lange on 2012-04-20

Use the TagsMixin on the predicate so local and global tags are tracked correctly.

[Robert Collins (lifeless)]

I don't get
353 + # XXX: ExtendedToOriginalDecorator doesn't properly wrap current_tags.
354 + # https://bugs.launchpad.net/testtools/+bug/978027

review: Needs Information

[Jonathan Lange (jml)]

On 26 April 2012 03:07, Robert Collins <email address hidden> wrote:
> Review: Needs Information
>
> I don't get
> 353     + # XXX: ExtendedToOriginalDecorator doesn't properly wrap current_tags.
> 354     + # https://bugs.launchpad.net/testtools/+bug/978027
>

Detritus from an earlier version. Have removed it.

jml

[lp:~jml/subunit/filter-tags]

187. By Jonathan Lange on 2012-04-26

Merge trunk

188. By Jonathan Lange on 2012-04-26

Fix up some XXX comments.

[Robert Collins (lifeless)]

I merged this to a worktree tree of trunk. There was a trivial conflict (mis-bound hunks in test_results.py), but tests failed:

======================================================================
FAIL: test_time_ordering_preserved (subunit.tests.test_subunit_filter.TestTestResultFilter)
subunit.tests.test_subunit_filter.TestTestResultFilter.test_time_ordering_preserved
----------------------------------------------------------------------
_StringException: Traceback (most recent call last):
  File "/home/robertc/source/unittest/subunit/working/python/subunit/tests/test_subunit_filter.py", line 240, in test_time_ordering_preserved
    ('time', date_c)], result._events)
AssertionError: Sequences differ: [('time', datetime.datetime(20... != [('time', datetime.datetime(20...

First differing element 1:
('time', datetime.datetime(2000, 1, 2, 0, 0, tzinfo=<subunit.iso8601.Utc object at 0x194af10>))
('startTest', <subunit.RemotedTestCase description='foo'>)

+ [('time', datetime.datetime(2000, 1, 1, 0, 0, tzinfo=<FixedOffset '+00:00'>)),
- [('time',
- datetime.datetime(2000, 1, 1, 0, 0, tzinfo=<subunit.iso8601.Utc object at 0x194af10>)),
- ('time',
- datetime.datetime(2000, 1, 2, 0, 0, tzinfo=<subunit.iso8601.Utc object at 0x194af10>)),
   ('startTest', <subunit.RemotedTestCase description='foo'>),
+ ('time', datetime.datetime(2000, 1, 2, 0, 0, tzinfo=<FixedOffset '+00:00'>)),
   ('addError', <subunit.RemotedTestCase description='foo'>, {}),
   ('stopTest', <subunit.RemotedTestCase description='foo'>),
+ ('time', datetime.datetime(2000, 1, 3, 0, 0, tzinfo=<FixedOffset '+00:00'>))]
- ('time',
- datetime.datetime(2000, 1, 3, 0, 0, tzinfo=<subunit.iso8601.Utc object at 0x194af10>))]

review: Needs Fixing

[Robert Collins (lifeless)]

The TestResultFilter :predicate: parameter docs need updating too:

        :param filter_predicate: A callable taking (test, outcome, err,
            details, tags) and returning True if the result should be passed
            through. err and details may be none if no error or extra
            metadata is available. outcome is the name of the outcome such
            as 'success' or 'failure'. tags is new in 0.0.8; 0.0.7 filters
            are still supported but should be updated to accept the tags
            parameter for efficiency.

[Robert Collins (lifeless)]

The FixedOffset class is also coming from iso8601.py.

[Robert Collins (lifeless)]

Ah, redundant time statement dropped.

[Robert Collins (lifeless)]

(where by dropped I mean 'I am tired and it moved').

[Jonathan Lange (jml)]

Thanks for landing this.

> "The FixedOffset class is also coming from iso8601.py."

I don't know what this means or is in relation to. Mentioned on IRC, but putting here as it's better suited to async comms.

=== modified file 'filters/subunit-filter'
--- filters/subunit-filter	2011-06-30 11:58:30 +0000
+++ filters/subunit-filter	2012-04-26 10:54:20 +0000
@@ -36,41 +36,59 @@
     TestProtocolClient,
     read_test_list,
     )
-from subunit.test_results import TestResultFilter
-
-parser = OptionParser(description=__doc__)
-parser.add_option("--error", action="store_false",
-    help="include errors", default=False, dest="error")
-parser.add_option("-e", "--no-error", action="store_true",
-    help="exclude errors", dest="error")
-parser.add_option("--failure", action="store_false",
-    help="include failures", default=False, dest="failure")
-parser.add_option("-f", "--no-failure", action="store_true",
-    help="exclude failures", dest="failure")
-parser.add_option("--passthrough", action="store_false",
-    help="Show all non subunit input.", default=False, dest="no_passthrough")
-parser.add_option("--no-passthrough", action="store_true",
-    help="Hide all non subunit input.", default=False, dest="no_passthrough")
-parser.add_option("-s", "--success", action="store_false",
-    help="include successes", dest="success")
-parser.add_option("--no-success", action="store_true",
-    help="exclude successes", default=True, dest="success")
-parser.add_option("--no-skip", action="store_true",
-    help="exclude skips", dest="skip")
-parser.add_option("--xfail", action="store_false",
-    help="include expected falures", default=True, dest="xfail")
-parser.add_option("--no-xfail", action="store_true",
-    help="exclude expected falures", default=True, dest="xfail")
-parser.add_option("-m", "--with", type=str,
-    help="regexp to include (case-sensitive by default)",
-    action="append", dest="with_regexps")
-parser.add_option("--fixup-expected-failures", type=str,
-    help="File with list of test ids that are expected to fail; on failure "
-         "their result will be changed to xfail; on success they will be "
-         "changed to error.", dest="fixup_expected_failures", action="append")
-parser.add_option("--without", type=str,
-    help="regexp to exclude (case-sensitive by default)",
-    action="append", dest="without_regexps")
+from subunit.filters import filter_by_result
+from subunit.test_results import (
+    and_predicates,
+    _make_tag_filter,
+    TestResultFilter,
+    )
+
+
+def make_options(description):
+    parser = OptionParser(description=__doc__)
+    parser.add_option("--error", action="store_false",
+        help="include errors", default=False, dest="error")
+    parser.add_option("-e", "--no-error", action="store_true",
+        help="exclude errors", dest="error")
+    parser.add_option("--failure", action="store_false",
+        help="include failures", default=False, dest="failure")
+    parser.add_option("-f", "--no-failure", action="store_true",
+        help="exclude failures", dest="failure")
+    parser.add_option("--passthrough", action="store_false",
+        help="Show all non subunit input.", default=False, dest="no_passthrough")
+    parser.add_option("--no-passthrough", action="store_true",
+        help="Hide all non subunit input.", default=False, dest="no_passthrough")
+    parser.add_option("-s", "--success", action="store_false",
+        help="include successes", dest="success")
+    parser.add_option("--no-success", action="store_true",
+        help="exclude successes", default=True, dest="success")
+    parser.add_option("--no-skip", action="store_true",
+        help="exclude skips", dest="skip")
+    parser.add_option("--xfail", action="store_false",
+        help="include expected falures", default=True, dest="xfail")
+    parser.add_option("--no-xfail", action="store_true",
+        help="exclude expected falures", default=True, dest="xfail")
+    parser.add_option(
+        "--with-tag", type=str,
+        help="include tests with these tags", action="append", dest="with_tags")
+    parser.add_option(
+        "--without-tag", type=str,
+        help="exclude tests with these tags", action="append", dest="without_tags")
+    parser.add_option("-m", "--with", type=str,
+        help="regexp to include (case-sensitive by default)",
+        action="append", dest="with_regexps")
+    parser.add_option("--fixup-expected-failures", type=str,
+        help="File with list of test ids that are expected to fail; on failure "
+             "their result will be changed to xfail; on success they will be "
+             "changed to error.", dest="fixup_expected_failures", action="append")
+    parser.add_option("--without", type=str,
+        help="regexp to exclude (case-sensitive by default)",
+        action="append", dest="without_regexps")
+    parser.add_option("-F", "--only-genuine-failures", action="callback",
+        callback=only_genuine_failures_callback,
+        help="Only pass through failures and exceptions.")
+    return parser
+
 
 def only_genuine_failures_callback(option, opt, value, parser):
     parser.rargs.insert(0, '--no-passthrough')
@@ -78,11 +96,6 @@
     parser.rargs.insert(0, '--no-skip')
     parser.rargs.insert(0, '--no-success')
 
-parser.add_option("-F", "--only-genuine-failures", action="callback",
-    callback=only_genuine_failures_callback,
-    help="Only pass through failures and exceptions.")
-
-(options, args) = parser.parse_args()
 
 def _compile_re_from_list(l):
     return re.compile("|".join(l), re.MULTILINE)
@@ -97,7 +110,7 @@
     with_re = with_regexps and _compile_re_from_list(with_regexps)
     without_re = without_regexps and _compile_re_from_list(without_regexps)
 
-    def check_regexps(test, outcome, err, details):
+    def check_regexps(test, outcome, err, details, tags):
         """Check if this test and error match the regexp filters."""
         test_str = str(test) + outcome + str(err) + str(details)
         if with_re and not with_re.search(test_str):
@@ -108,21 +121,38 @@
     return check_regexps
 
 
-regexp_filter = _make_regexp_filter(options.with_regexps,
-        options.without_regexps)
-fixup_expected_failures = set()
-for path in options.fixup_expected_failures or ():
-    fixup_expected_failures.update(read_test_list(path))
-result = TestProtocolClient(sys.stdout)
-result = TestResultFilter(result, filter_error=options.error,
-    filter_failure=options.failure, filter_success=options.success,
-    filter_skip=options.skip, filter_xfail=options.xfail,
-    filter_predicate=regexp_filter,
-    fixup_expected_failures=fixup_expected_failures)
-if options.no_passthrough:
-    passthrough_stream = DiscardStream()
-else:
-    passthrough_stream = None
-test = ProtocolTestCase(sys.stdin, passthrough=passthrough_stream)
-test.run(result)
-sys.exit(0)
+def _make_result(output, options, predicate):
+    """Make the result that we'll send the test outcomes to."""
+    fixup_expected_failures = set()
+    for path in options.fixup_expected_failures or ():
+        fixup_expected_failures.update(read_test_list(path))
+    return TestResultFilter(
+        TestProtocolClient(output),
+        filter_error=options.error,
+        filter_failure=options.failure,
+        filter_success=options.success,
+        filter_skip=options.skip,
+        filter_xfail=options.xfail,
+        filter_predicate=predicate,
+        fixup_expected_failures=fixup_expected_failures)
+
+
+def main():
+    parser = make_options(__doc__)
+    (options, args) = parser.parse_args()
+
+    regexp_filter = _make_regexp_filter(
+        options.with_regexps, options.without_regexps)
+    tag_filter = _make_tag_filter(options.with_tags, options.without_tags)
+    filter_predicate = and_predicates([regexp_filter, tag_filter])
+
+    filter_by_result(
+        lambda output_to: _make_result(sys.stdout, options, filter_predicate),
+        output_path=None,
+        passthrough=(not options.no_passthrough),
+        forward=False)
+    sys.exit(0)
+
+
+if __name__ == '__main__':
+    main()

=== modified file 'python/subunit/test_results.py'
--- python/subunit/test_results.py	2012-04-20 11:32:41 +0000
+++ python/subunit/test_results.py	2012-04-26 10:54:20 +0000
@@ -20,6 +20,7 @@
 import datetime
 
 import testtools
+from testtools.compat import all
 from testtools.content import (
     text_content,
     TracebackContent,
@@ -39,6 +40,9 @@
     or features by degrading them.
     """
 
+    # XXX: Since lp:testtools r250, this is in testtools. Once it's released,
+    # we should gut this and just use that.
+
     def __init__(self, decorated):
         """Create a TestResultDecorator forwarding to decorated."""
         # Make every decorator degrade gracefully.
@@ -205,48 +209,45 @@
         return self.decorated.time(a_datetime)
 
 
-class TagCollapsingDecorator(HookedTestResultDecorator):
-    """Collapses many 'tags' calls into one where possible."""
+class TagsMixin(object):
 
-    def __init__(self, result):
-        super(TagCollapsingDecorator, self).__init__(result)
+    def __init__(self):
         self._clear_tags()
 
     def _clear_tags(self):
         self._global_tags = set(), set()
-        self._current_test_tags = None
-
-    def _get_current_tags(self):
-        if self._current_test_tags:
-            return self._current_test_tags
+        self._test_tags = None
+
+    def _get_active_tags(self):
+        global_new, global_gone = self._global_tags
+        if self._test_tags is None:
+            return set(global_new)
+        test_new, test_gone = self._test_tags
+        return global_new.difference(test_gone).union(test_new)
+
+    def _get_current_scope(self):
+        if self._test_tags:
+            return self._test_tags
         return self._global_tags
 
-    def startTestRun(self):
-        super(TagCollapsingDecorator, self).startTestRun()
-        self._clear_tags()
-
-    def startTest(self, test):
-        """Start a test.
-
-        Not directly passed to the client, but used for handling of tags
-        correctly.
-        """
-        super(TagCollapsingDecorator, self).startTest(test)
-        self._current_test_tags = set(), set()
-
-    def stopTest(self, test):
-        super(TagCollapsingDecorator, self).stopTest(test)
-        self._current_test_tags = None
-
-    def _before_event(self):
-        new_tags, gone_tags = self._get_current_tags()
+    def _flush_current_scope(self, tag_receiver):
+        new_tags, gone_tags = self._get_current_scope()
         if new_tags or gone_tags:
-            self.decorated.tags(new_tags, gone_tags)
-        if self._current_test_tags:
-            self._current_test_tags = set(), set()
+            tag_receiver.tags(new_tags, gone_tags)
+        if self._test_tags:
+            self._test_tags = set(), set()
         else:
             self._global_tags = set(), set()
 
+    def startTestRun(self):
+        self._clear_tags()
+
+    def startTest(self, test):
+        self._test_tags = set(), set()
+
+    def stopTest(self, test):
+        self._test_tags = None
+
     def tags(self, new_tags, gone_tags):
         """Handle tag instructions.
 
@@ -256,13 +257,27 @@
         :param new_tags: Tags to add,
         :param gone_tags: Tags to remove.
         """
-        current_new_tags, current_gone_tags = self._get_current_tags()
+        current_new_tags, current_gone_tags = self._get_current_scope()
         current_new_tags.update(new_tags)
         current_new_tags.difference_update(gone_tags)
         current_gone_tags.update(gone_tags)
         current_gone_tags.difference_update(new_tags)
 
 
+class TagCollapsingDecorator(HookedTestResultDecorator, TagsMixin):
+    """Collapses many 'tags' calls into one where possible."""
+
+    def __init__(self, result):
+        super(TagCollapsingDecorator, self).__init__(result)
+        self._clear_tags()
+
+    def _before_event(self):
+        self._flush_current_scope(self.decorated)
+
+    def tags(self, new_tags, gone_tags):
+        TagsMixin.tags(self, new_tags, gone_tags)
+
+
 class TimeCollapsingDecorator(HookedTestResultDecorator):
     """Only pass on the first and last of a consecutive sequence of times."""
 
@@ -288,12 +303,132 @@
         self._last_received_time = a_time
 
 
-def all_true(bools):
-    """Return True if all of 'bools' are True. False otherwise."""
-    for b in bools:
-        if not b:
-            return False
-    return True
+def and_predicates(predicates):
+    """Return a predicate that is true iff all predicates are true."""
+    # XXX: Should probably be in testtools to be better used by matchers. jml
+    return lambda *args, **kwargs: all(p(*args, **kwargs) for p in predicates)
+
+
+def _make_tag_filter(with_tags, without_tags):
+    """Make a callback that checks tests against tags."""
+
+    with_tags = with_tags and set(with_tags) or None
+    without_tags = without_tags and set(without_tags) or None
+
+    def check_tags(test, outcome, err, details, tags):
+        if with_tags and not with_tags <= tags:
+            return False
+        if without_tags and bool(without_tags & tags):
+            return False
+        return True
+
+    return check_tags
+
+
+class _PredicateFilter(TestResultDecorator, TagsMixin):
+
+    def __init__(self, result, predicate):
+        super(_PredicateFilter, self).__init__(result)
+        self._clear_tags()
+        self.decorated = TimeCollapsingDecorator(
+            TagCollapsingDecorator(self.decorated))
+        self._predicate = predicate
+        # The current test (for filtering tags)
+        self._current_test = None
+        # Has the current test been filtered (for outputting test tags)
+        self._current_test_filtered = None
+        # Calls to this result that we don't know whether to forward on yet.
+        self._buffered_calls = []
+
+    def filter_predicate(self, test, outcome, error, details):
+        return self._predicate(
+            test, outcome, error, details, self._get_active_tags())
+
+    def addError(self, test, err=None, details=None):
+        if (self.filter_predicate(test, 'error', err, details)):
+            self._buffered_calls.append(
+                ('addError', [test, err], {'details': details}))
+        else:
+            self._filtered()
+
+    def addFailure(self, test, err=None, details=None):
+        if (self.filter_predicate(test, 'failure', err, details)):
+            self._buffered_calls.append(
+                ('addFailure', [test, err], {'details': details}))
+        else:
+            self._filtered()
+
+    def addSkip(self, test, reason=None, details=None):
+        if (self.filter_predicate(test, 'skip', reason, details)):
+            self._buffered_calls.append(
+                ('addSkip', [test, reason], {'details': details}))
+        else:
+            self._filtered()
+
+    def addExpectedFailure(self, test, err=None, details=None):
+        if self.filter_predicate(test, 'expectedfailure', err, details):
+            self._buffered_calls.append(
+                ('addExpectedFailure', [test, err], {'details': details}))
+        else:
+            self._filtered()
+
+    def addUnexpectedSuccess(self, test, details=None):
+        self._buffered_calls.append(
+            ('addUnexpectedSuccess', [test], {'details': details}))
+
+    def addSuccess(self, test, details=None):
+        if (self.filter_predicate(test, 'success', None, details)):
+            self._buffered_calls.append(
+                ('addSuccess', [test], {'details': details}))
+        else:
+            self._filtered()
+
+    def _filtered(self):
+        self._current_test_filtered = True
+
+    def startTest(self, test):
+        """Start a test.
+
+        Not directly passed to the client, but used for handling of tags
+        correctly.
+        """
+        TagsMixin.startTest(self, test)
+        self._current_test = test
+        self._current_test_filtered = False
+        self._buffered_calls.append(('startTest', [test], {}))
+
+    def stopTest(self, test):
+        """Stop a test.
+
+        Not directly passed to the client, but used for handling of tags
+        correctly.
+        """
+        if not self._current_test_filtered:
+            for method, args, kwargs in self._buffered_calls:
+                getattr(self.decorated, method)(*args, **kwargs)
+            self.decorated.stopTest(test)
+        self._current_test = None
+        self._current_test_filtered = None
+        self._buffered_calls = []
+        TagsMixin.stopTest(self, test)
+
+    def tags(self, new_tags, gone_tags):
+        TagsMixin.tags(self, new_tags, gone_tags)
+        if self._current_test is not None:
+            self._buffered_calls.append(('tags', [new_tags, gone_tags], {}))
+        else:
+            return super(_PredicateFilter, self).tags(new_tags, gone_tags)
+
+    def time(self, a_time):
+        if self._current_test is not None:
+            self._buffered_calls.append(('time', [a_time], {}))
+        else:
+            return self.decorated.time(a_time)
+
+    def id_to_orig_id(self, id):
+        if id.startswith("subunit.RemotedTestCase."):
+            return id[len("subunit.RemotedTestCase."):]
+        return id
 
 
 class TestResultFilter(TestResultDecorator):
@@ -326,129 +461,62 @@
         :param fixup_expected_failures: Set of test ids to consider known
             failing.
         """
-        super(TestResultFilter, self).__init__(result)
-        self.decorated = TimeCollapsingDecorator(
-            TagCollapsingDecorator(self.decorated))
         predicates = []
         if filter_error:
-            predicates.append(lambda t, outcome, e, d: outcome != 'error')
+            predicates.append(
+                lambda t, outcome, e, d, tags: outcome != 'error')
         if filter_failure:
-            predicates.append(lambda t, outcome, e, d: outcome != 'failure')
+            predicates.append(
+                lambda t, outcome, e, d, tags: outcome != 'failure')
         if filter_success:
-            predicates.append(lambda t, outcome, e, d: outcome != 'success')
+            predicates.append(
+                lambda t, outcome, e, d, tags: outcome != 'success')
         if filter_skip:
-            predicates.append(lambda t, outcome, e, d: outcome != 'skip')
+            predicates.append(
+                lambda t, outcome, e, d, tags: outcome != 'skip')
         if filter_xfail:
-            predicates.append(lambda t, outcome, e, d: outcome != 'expectedfailure')
+            predicates.append(
+                lambda t, outcome, e, d, tags: outcome != 'expectedfailure')
         if filter_predicate is not None:
-            predicates.append(filter_predicate)
-        self.filter_predicate = (
-            lambda test, outcome, err, details:
-                all_true(p(test, outcome, err, details) for p in predicates))
-        # The current test (for filtering tags)
-        self._current_test = None
-        # Has the current test been filtered (for outputting test tags)
-        self._current_test_filtered = None
-        # Calls to this result that we don't know whether to forward on yet.
-        self._buffered_calls = []
+            def compat(test, outcome, error, details, tags):
+                # 0.0.7 and earlier did not support the 'tags' parameter.
+                try:
+                    return filter_predicate(
+                        test, outcome, error, details, tags)
+                except TypeError:
+                    return filter_predicate(test, outcome, error, details)
+            predicates.append(compat)
+        predicate = and_predicates(predicates)
+        super(TestResultFilter, self).__init__(
+            _PredicateFilter(result, predicate))
         if fixup_expected_failures is None:
             self._fixup_expected_failures = frozenset()
         else:
             self._fixup_expected_failures = fixup_expected_failures
 
     def addError(self, test, err=None, details=None):
-        if (self.filter_predicate(test, 'error', err, details)):
-            if self._failure_expected(test):
-                self._buffered_calls.append(
-                    ('addExpectedFailure', [test, err], {'details': details}))
-            else:
-                self._buffered_calls.append(
-                    ('addError', [test, err], {'details': details}))
+        if self._failure_expected(test):
+            self.addExpectedFailure(test, err=err, details=details)
         else:
-            self._filtered()
+            super(TestResultFilter, self).addError(
+                test, err=err, details=details)
 
     def addFailure(self, test, err=None, details=None):
-        if (self.filter_predicate(test, 'failure', err, details)):
-            if self._failure_expected(test):
-                self._buffered_calls.append(
-                    ('addExpectedFailure', [test, err], {'details': details}))
-            else:
-                self._buffered_calls.append(
-                    ('addFailure', [test, err], {'details': details}))
-        else:
-            self._filtered()
-
-    def addSkip(self, test, reason=None, details=None):
-        if (self.filter_predicate(test, 'skip', reason, details)):
-            self._buffered_calls.append(
-                ('addSkip', [test, reason], {'details': details}))
-        else:
-            self._filtered()
+        if self._failure_expected(test):
+            self.addExpectedFailure(test, err=err, details=details)
+        else:
+            super(TestResultFilter, self).addFailure(
+                test, err=err, details=details)
 
     def addSuccess(self, test, details=None):
-        if (self.filter_predicate(test, 'success', None, details)):
-            if self._failure_expected(test):
-                self._buffered_calls.append(
-                    ('addUnexpectedSuccess', [test], {'details': details}))
-            else:
-                self._buffered_calls.append(
-                    ('addSuccess', [test], {'details': details}))
-        else:
-            self._filtered()
-
-    def addExpectedFailure(self, test, err=None, details=None):
-        if self.filter_predicate(test, 'expectedfailure', err, details):
-            self._buffered_calls.append(
-                ('addExpectedFailure', [test, err], {'details': details}))
-        else:
-            self._filtered()
-
-    def addUnexpectedSuccess(self, test, details=None):
-        self._buffered_calls.append(
-            ('addUnexpectedSuccess', [test], {'details': details}))
-
-    def _filtered(self):
-        self._current_test_filtered = True
+        if self._failure_expected(test):
+            self.addUnexpectedSuccess(test, details=details)
+        else:
+            super(TestResultFilter, self).addSuccess(test, details=details)
 
     def _failure_expected(self, test):
         return (test.id() in self._fixup_expected_failures)
 
-    def startTest(self, test):
-        """Start a test.
-
-        Not directly passed to the client, but used for handling of tags
-        correctly.
-        """
-        self._current_test = test
-        self._current_test_filtered = False
-        self._buffered_calls.append(('startTest', [test], {}))
-
-    def stopTest(self, test):
-        """Stop a test.
-
-        Not directly passed to the client, but used for handling of tags
-        correctly.
-        """
-        if not self._current_test_filtered:
-            # Tags to output for this test.
-            for method, args, kwargs in self._buffered_calls:
-                getattr(self.decorated, method)(*args, **kwargs)
-            self.decorated.stopTest(test)
-        self._current_test = None
-        self._current_test_filtered = None
-        self._buffered_calls = []
-
-    def time(self, a_time):
-        if self._current_test is not None:
-            self._buffered_calls.append(('time', [a_time], {}))
-        else:
-            return self.decorated.time(a_time)
-
-    def id_to_orig_id(self, id):
-        if id.startswith("subunit.RemotedTestCase."):
-            return id[len("subunit.RemotedTestCase."):]
-        return id
-
 
 class TestIdPrintingResult(testtools.TestResult):
 
@@ -513,7 +581,8 @@
 class TestByTestResult(testtools.TestResult):
     """Call something every time a test completes."""
 
-    # XXX: Arguably belongs in testtools.
+# XXX: In testtools since lp:testtools r249.  Once that's released, just
+# import that.
 
     def __init__(self, on_test):
         """Construct a ``TestByTestResult``.

=== modified file 'python/subunit/tests/test_subunit_filter.py'
--- python/subunit/tests/test_subunit_filter.py	2011-05-09 21:00:42 +0000
+++ python/subunit/tests/test_subunit_filter.py	2012-04-26 10:54:20 +0000
@@ -17,15 +17,18 @@
 """Tests for subunit.TestResultFilter."""
 
 from datetime import datetime
+import os
+import subprocess
+import sys
 from subunit import iso8601
 import unittest
 
 from testtools import TestCase
-from testtools.compat import _b, BytesIO, StringIO
+from testtools.compat import _b, BytesIO
 from testtools.testresult.doubles import ExtendedTestResult
 
 import subunit
-from subunit.test_results import TestResultFilter
+from subunit.test_results import _make_tag_filter, TestResultFilter
 
 
 class TestTestResultFilter(TestCase):
@@ -77,6 +80,40 @@
             filtered_result.failures])
         self.assertEqual(4, filtered_result.testsRun)
 
+    def test_tag_filter(self):
+        tag_filter = _make_tag_filter(['global'], ['local'])
+        result = ExtendedTestResult()
+        result_filter = TestResultFilter(
+            result, filter_success=False, filter_predicate=tag_filter)
+        self.run_tests(result_filter)
+        tests_included = [
+            event[1] for event in result._events if event[0] == 'startTest']
+        tests_expected = map(
+            subunit.RemotedTestCase,
+            ['passed', 'error', 'skipped', 'todo'])
+        self.assertEquals(tests_expected, tests_included)
+
+    def test_tags_tracked_correctly(self):
+        tag_filter = _make_tag_filter(['a'], [])
+        result = ExtendedTestResult()
+        result_filter = TestResultFilter(
+            result, filter_success=False, filter_predicate=tag_filter)
+        input_stream = (
+            "test: foo\n"
+            "tags: a\n"
+            "successful: foo\n"
+            "test: bar\n"
+            "successful: bar\n")
+        self.run_tests(result_filter, input_stream)
+        foo = subunit.RemotedTestCase('foo')
+        self.assertEquals(
+            [('startTest', foo),
+             ('tags', set(['a']), set()),
+             ('addSuccess', foo),
+             ('stopTest', foo),
+             ],
+            result._events)
+
     def test_exclude_errors(self):
         filtered_result = unittest.TestResult()
         result_filter = TestResultFilter(filtered_result, filter_error=True)
@@ -151,6 +188,8 @@
 
     def test_filter_predicate(self):
         """You can filter by predicate callbacks"""
+        # 0.0.7 and earlier did not support the 'tags' parameter, so we need
+        # to test that we still support behaviour without it.
         filtered_result = unittest.TestResult()
         def filter_cb(test, outcome, err, details):
             return outcome == 'success'
@@ -161,6 +200,18 @@
         # Only success should pass
         self.assertEqual(1, filtered_result.testsRun)
 
+    def test_filter_predicate_with_tags(self):
+        """You can filter by predicate callbacks that accept tags"""
+        filtered_result = unittest.TestResult()
+        def filter_cb(test, outcome, err, details, tags):
+            return outcome == 'success'
+        result_filter = TestResultFilter(filtered_result,
+            filter_predicate=filter_cb,
+            filter_success=False)
+        self.run_tests(result_filter)
+        # Only success should pass
+        self.assertEqual(1, filtered_result.testsRun)
+
     def test_time_ordering_preserved(self):
         # Passing a subunit stream through TestResultFilter preserves the
         # relative ordering of 'time' directives and any other subunit
@@ -202,6 +253,86 @@
              ('stopTest', foo), ], result._events)
 
 
+class TestFilterCommand(TestCase):
+
+    example_subunit_stream = _b("""\
+tags: global
+test passed
+success passed
+test failed
+tags: local
+failure failed
+test error
+error error [
+error details
+]
+test skipped
+skip skipped
+test todo
+xfail todo
+""")
+
+    def run_command(self, args, stream):
+        root = os.path.dirname(
+            os.path.dirname(os.path.dirname(os.path.dirname(__file__))))
+        script_path = os.path.join(root, 'filters', 'subunit-filter')
+        command = [sys.executable, script_path] + list(args)
+        ps = subprocess.Popen(
+            command, stdin=subprocess.PIPE, stdout=subprocess.PIPE,
+            stderr=subprocess.PIPE)
+        out, err = ps.communicate(stream)
+        if ps.returncode != 0:
+            raise RuntimeError("%s failed: %s" % (command, err))
+        return out
+
+    def to_events(self, stream):
+        test = subunit.ProtocolTestCase(BytesIO(stream))
+        result = ExtendedTestResult()
+        test.run(result)
+        return result._events
+
+    def test_default(self):
+        output = self.run_command([], (
+                "test: foo\n"
+                "skip: foo\n"
+                ))
+        events = self.to_events(output)
+        foo = subunit.RemotedTestCase('foo')
+        self.assertEqual(
+            [('startTest', foo),
+             ('addSkip', foo, {}),
+             ('stopTest', foo)],
+            events)
+
+    def test_tags(self):
+        output = self.run_command(['-s', '--with-tag', 'a'], (
+                "tags: a\n"
+                "test: foo\n"
+                "success: foo\n"
+                "tags: -a\n"
+                "test: bar\n"
+                "success: bar\n"
+                "test: baz\n"
+                "tags: a\n"
+                "success: baz\n"
+                ))
+        events = self.to_events(output)
+        foo = subunit.RemotedTestCase('foo')
+        baz = subunit.RemotedTestCase('baz')
+        self.assertEqual(
+            [('tags', set(['a']), set()),
+             ('startTest', foo),
+             ('addSuccess', foo),
+             ('stopTest', foo),
+             ('tags', set(), set(['a'])),
+             ('startTest', baz),
+             ('tags', set(['a']), set()),
+             ('addSuccess', baz),
+             ('stopTest', baz),
+             ],
+            events)
+
+
 def test_suite():
     loader = subunit.tests.TestUtil.TestLoader()
     result = loader.loadTestsFromName(__name__)