Make test categories composable

[labath]

Summary

Previously the add_test_categories would simply overwrite the current set of categories for a
method. This change makes the decorator truly "add" categories, by extending the current set of
categories instead of replacing it.

To do this, I have:

• replaced the getCategories() property on a method (which was itself a method), with a simple list property "categories". This makes add_test_categories easier to implement, and test categories isn't something which should change between calls anyway.
• rewritten the getCategoriesForTest function to merge method categories with the categories of the test case. Previously, it would just use the method categories if they were present. I have also greatly simplified this method. Originally, it would use a lot of introspection to enable it being called on various types of objects. Based on my tests, it was only ever being called on a test case. The new function uses much less introspection then the preivous one, so we should easily catch any stray uses, if there are any, as they will generate exceptions now.

[labath]

I've changed my mind and I'm moving back the getCategoriesForTest function into test_result.py.
The function only works when the test is currently executing (because of the magic
_testMethodName hack), so it's usability as a general utility function is very limited. Let's keep it here until we have a reason to do otherwise.

[tberghammer]

LGTM

packages/Python/lldbsuite/test/lldbtest.py

512 ↗

(On Diff #42522)

Can you be more specific about where can we add this decorator? "item" doesn't really tell anything to me

[tfiala]

LGTM. I added a few optional comments on possible improvements, but I think they're orthogonal to what you're doing here.

packages/Python/lldbsuite/test/lldbtest.py

512 ↗

(On Diff #42522)

I think we're talking TestCase test method, right?

518 ↗

(On Diff #42522)

This code could potentially accept a single category and drop the need for the list. It could check the type of the input (i.e. the categories input) and, if a single element rather than a list, it could just cat.append() it.

That would enable a single argument passed to @add_test_categories()

Totally minor and you can skip this, it just makes for a nice usability enhancement provided the decorator is documented as such.

[tberghammer]

packages/Python/lldbsuite/test/lldbtest.py

518 ↗

(On Diff #42522)

+1: I think we will (accidentally) call the function by a single string in some case so accepting it would be a good idea.

[labath]

packages/Python/lldbsuite/test/lldbtest.py

518 ↗

(On Diff #42522)

The test_categories.validate will catch the case where you forget to put the braces and throw errors at you, so it's not possible to get unexpected behavior here.

I may be old-fashioned, but I like when things have a single type, which is obvious from the code. Look at all the cases where we do callable(bugnumber) and tell me if it's obvious why a "bug number" should be "callable". Plus, none of our other decorators taking lists have this kind of magic in them (imagine how would this complicate the logic of expectedFailureAll).

If we want to provide a simpler syntax for adding a single category, i would go for adding a separate @add_test_category decorator for that.

[tberghammer]

packages/Python/lldbsuite/test/lldbtest.py

518 ↗

(On Diff #42522)

I think the pythonic way is to accept (almost) any type of argument including a single element string and any iterable (list, set, generator expression, etc...). In long term I would like to see all of our decorator taking single and multi argument at the same time but we can make that change later with refactoring all decorator to support it (it can be handled easily by a utility function).

lldb/trunk/packages/Python/lldbsuite/test/lldbtest.py

def check_expected_version(comparison, expected, actual):
     actual_str = '.'.join([str(x) for x in actual])
 
     return op_lookup[comparison](LooseVersion(actual_str), LooseVersion(expected_str))
 
 #
 # Decorators for categorizing test cases.
 #
 from functools import wraps
+
 def add_test_categories(cat):
-    """Decorate an item with test categories"""
+    """Add test categories to a TestCase method"""
     cat = test_categories.validate(cat, True)
     def impl(func):
-        func.getCategories = lambda test: cat
+        if isinstance(func, type) and issubclass(func, unittest2.TestCase):
+            raise Exception("@add_test_categories can only be used to decorate a test method")
+        if hasattr(func, "categories"):
+            cat.extend(func.categories)
+        func.categories = cat
         return func
+
     return impl
 
 def benchmarks_test(func):
     """Decorate the item as a benchmarks test."""
     if isinstance(func, type) and issubclass(func, unittest2.TestCase):
         raise Exception("@benchmarks_test can only be used to decorate a test method")
     @wraps(func)
     def wrapper(self, *args, **kwargs):

lldb/trunk/packages/Python/lldbsuite/test/test_categories.py

 """
 Provides definitions for various lldb test categories
 """
 
 from __future__ import absolute_import
+from __future__ import print_function
 
 # System modules
 import sys
 
 # Third-party modules
 
 # LLDB modules
 

lldb/trunk/packages/Python/lldbsuite/test/test_result.py

class LLDBTestResult(unittest2.TextTestResult):
 
     def getDescription(self, test):
         doc_first_line = test.shortDescription()
         if self.descriptions and doc_first_line:
             return '\n'.join((str(test), self.indentation + doc_first_line))
         else:
             return str(test)
 
-    def getCategoriesForTest(self,test):
-        if hasattr(test,"_testMethodName"):
-            test_method = getattr(test,"_testMethodName")
-            test_method = getattr(test,test_method)
-        else:
-            test_method = None
-        if test_method != None and hasattr(test_method,"getCategories"):
-            test_categories = test_method.getCategories(test)
-        elif hasattr(test,"getCategories"):
-            test_categories = test.getCategories()
-        elif inspect.ismethod(test) and test.__self__ != None and hasattr(test.__self__,"getCategories"):
-            test_categories = test.__self__.getCategories()
-        else:
-            test_categories = []
-        if test_categories == None:
-            test_categories = []
+    def getCategoriesForTest(self, test):
+        """
+        Gets all the categories for the currently running test method in test case
+        """
+        test_method = getattr(test, test._testMethodName)
+        if test_method != None and hasattr(test_method, "categories"):
+            test_categories.extend(test_method.categories)
+
+        test_categories.extend(test.getCategories())
+
         return test_categories
 
     def hardMarkAsSkipped(self,test):
         getattr(test, test._testMethodName).__func__.__unittest_skip__ = True
         getattr(test, test._testMethodName).__func__.__unittest_skip_why__ = "test case does not fall in any category of interest for this run"
         test.__class__.__unittest_skip__ = True
         test.__class__.__unittest_skip_why__ = "test case does not fall in any category of interest for this run"