Merge lp:~yellow/subunit/test-count into lp:~subunit/subunit/trunk

[Gary Poster]

This branch changes the semantics of testsRun in a TestResultFilter to count the total number of tests run, not the number of tests filtered.

Whether these semantics are correct is up for debate. We're obviously proposing that they are. That said, these changes primarily exist to make the testrepository change in lp:~yellow/testrepository/bug988481 cleaner. We don't know how to continue to subclass in testrepository.

This is work done by Benji York and Brad Crittenden; I'm just helping out with the MP.

[Jonathan Lange (jml)]

I'm not 100% sure about this change, but I can't think of any actual use case for which this would be a regression.

review: Approve

[Jonathan Lange (jml)]

Uaenusthe.
Wrong MP sorry.

review: Abstain

[Jonathan Lange (jml)]

Hey guys,

Thanks for looking into this one. I don't think that this change should go into subunit. The _get_concrete_result thing is just too hideous. I wrote it originally so I hope you aren't offended by me saying so.

What testrepository needs is a way to increment testsRun on its CLITestResult instance even when tests are filtered out of the stream. The right way to do this is to pass that result into TestResultFilter somehow. Here are some options:

  class MyTestResultFilter(TestResultFilter):

    def __init__(self, result, on_filter, **kwargs):
      super(MyTestResultFilter, self).__init__(result, **kwargs)
      self._on_filter = on_filter

    def _filtered(self):
      super(MyTestResultFilter, self)._filtered()
      self._on_filter()

Or you could make the last line:

      self._on_filter(self._current_test)

Instead of writing that as a subclass, it could be a patch to TestResultFilter in subunit. In which case it would have to be an optional argument. Then you could call TestResultFilter(cli_test_result, lambda test: cli_test_result.testsRun += 1) in testrepository. (Except that won't work because of Python's statement / expression hang up.)

Alternatively, you could make a more specific version and have it live in testrepository:

  class MyTestResultFilter(TestResultFilter):

    def __init__(self, result, cli_test_result, **kwargs):
      super(MyTestResultFilter, self).__init__(result, **kwargs)
      self._cli_test_result = cli_test_result

    def _filtered(self):
      super(MyTestResultFilter, self)._filtered()
      self._cli_test_result.testsRun += 1

I'm sorry to bounce back so hard on this, but the getattr() searching is really quite fragile.

Thanks,
jml

review: Disapprove

[Gary Poster (gary)]

Hi Jono.

While this change is stand-alone from subunit's perspective, we actually were working with https://code.launchpad.net/~jml/subunit/filter-tags/+merge/102840 .

Without the filter-tags branch, http://pastebin.ubuntu.com/950663/ is a sufficient change to get the testrepository tests to pass with subunit's trunk (after your already-completed merge of https://code.launchpad.net/+branch/~yellow/subunit/real-time).

However, with the filter-tags branch and the testrepository patch above, we still need to address additional failures: http://pastebin.ubuntu.com/951133/ . Those are the failures that this branch tried to fix.

The _PredicateFilter refactoring in the filter-tags branch makes it harder to come up with a solution. The _filtered override changes you suggested will break just as much as the existing _filtered method override does. If instead we override addFailure, addError, and addSkip to increment testsRun there, we double count tests that matched the filter.

I think the best approach will be a subunit change like the one you gave...except that I think the argument will need to be something on the order of "on_filter," because we need to increment testsRun manually only when a test is filtered out in order to get the behavior testrepository desires. I'll make a new branch that does that.

Thanks,

Gary

[Robert Collins (lifeless)]

So, I'm going to disagree with this patch from another angle: its
inappropriate coupling.

TestRepository can just use a Y join to get a separate count vs
filtered tests. (And BTW, the *filtered* count there is appropriate
for UI consumption, so I don't see how this would help with it
anyway).

[Jonathan Lange (jml)]

The filtered count (i.e. the number of tests that we display in the output) might be appropriate for UI consumption (e.g. 7 failing tests), but the UI also needs to have the unfiltered count (e.g. 100 tests run).

The UI layer can use a Y join, I guess. I can't fully connect the dots in my head but I can see how it might work.

[Robert Collins (lifeless)]

To join the dots, see my whiteboard image on the list.

Unmerged revisions

164. By Benji York on 2012-04-26

make filtered test results report the total number of tests

=== 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 15:20:26 +0000
@@ -407,8 +407,20 @@
         self._buffered_calls.append(
             ('addUnexpectedSuccess', [test], {'details': details}))
 
+    def _get_concrete_result(self):
+        # This assumes that the test result we actually care about is
+        # decorated and that we can find our way to the one we care about.
+        concrete = self
+        while True:
+            next = getattr(concrete, 'decorated', None)
+            if next is None:
+                return concrete
+            concrete = next
+
     def _filtered(self):
         self._current_test_filtered = True
+        concrete = self._get_concrete_result()
+        concrete.testsRun += 1
 
     def _failure_expected(self, test):
         return (test.id() in self._fixup_expected_failures)

=== 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 15:20:26 +0000
@@ -75,7 +75,6 @@
         self.assertEqual(['failed'],
             [failure[0].id() for failure in
             filtered_result.failures])
-        self.assertEqual(4, filtered_result.testsRun)
 
     def test_exclude_errors(self):
         filtered_result = unittest.TestResult()
@@ -86,7 +85,6 @@
         self.assertEqual(['failed'],
             [failure[0].id() for failure in
             filtered_result.failures])
-        self.assertEqual(3, filtered_result.testsRun)
 
     def test_fixup_expected_failures(self):
         filtered_result = unittest.TestResult()
@@ -96,7 +94,6 @@
         self.assertEqual(['failed', 'todo'],
             [failure[0].id() for failure in filtered_result.expectedFailures])
         self.assertEqual([], filtered_result.failures)
-        self.assertEqual(4, filtered_result.testsRun)
 
     def test_fixup_expected_errors(self):
         filtered_result = unittest.TestResult()
@@ -106,7 +103,6 @@
         self.assertEqual(['error', 'todo'],
             [failure[0].id() for failure in filtered_result.expectedFailures])
         self.assertEqual([], filtered_result.errors)
-        self.assertEqual(4, filtered_result.testsRun)
 
     def test_fixup_unexpected_success(self):
         filtered_result = unittest.TestResult()
@@ -115,7 +111,6 @@
         self.run_tests(result_filter)
         self.assertEqual(['passed'],
             [passed.id() for passed in filtered_result.unexpectedSuccesses])
-        self.assertEqual(5, filtered_result.testsRun)
 
     def test_exclude_failure(self):
         filtered_result = unittest.TestResult()
@@ -126,7 +121,6 @@
         self.assertEqual([],
             [failure[0].id() for failure in
             filtered_result.failures])
-        self.assertEqual(3, filtered_result.testsRun)
 
     def test_exclude_skips(self):
         filtered_result = subunit.TestResultStats(None)
@@ -134,7 +128,6 @@
         self.run_tests(result_filter)
         self.assertEqual(0, filtered_result.skipped_tests)
         self.assertEqual(2, filtered_result.failed_tests)
-        self.assertEqual(3, filtered_result.testsRun)
 
     def test_include_success(self):
         """Successes can be included if requested."""
@@ -153,13 +146,13 @@
         """You can filter by predicate callbacks"""
         filtered_result = unittest.TestResult()
         def filter_cb(test, outcome, err, details):
-            return outcome == 'success'
+            return outcome == 'error'
         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)
+        # Only errors should pass
+        self.assertEqual(1, len(filtered_result.errors))
 
     def test_time_ordering_preserved(self):
         # Passing a subunit stream through TestResultFilter preserves the