Make debug info specification use categories system

[zturner]

Summary

I'm not sure if I did this right, but the end result of this CL is removal of the -N command line option, and making people go through the categories system instead.

[zturner]

Actually remove the -N command line option as well.

[labath]

The interaction between the add_test_categories and the "magic test multiplier" is causing problems, which is evident in the fact how you needed to add the @skip decorator to make it work.

It sounds like we need to decide what will be the "correct" interaction of the "test multiplier" and tests manually marked with a specific debug info.

I think the most sensible behavior would be to treat a manual debuginfo mark as a signal that you don't want your test to be auto-multiplied, the test multiplier could then check whether the test is already marked with one of the debuginfo annotations, and if it is, then avoid touching it.

What do you think?

packages/Python/lldbsuite/test/functionalities/unwind/standard/TestStandardUnwind.py

120

You need [] around the "dwarf". Otherwise, you will get errors here, when the skipIf below does not actually skip.

packages/Python/lldbsuite/test/lang/cpp/limit-debug-info/TestWithLimitDebugInfo.py

10

It should not be necessary to annotate and skip at the same time.

packages/Python/lldbsuite/test/lldbtest.py

2226–2227

Here, we would then also check getCategoriesForTest(attrvalue) for presence of debug info markers. It looks like you will need to move that function from test_result.py to somewhere else though...

2230

might as well use getDarwinOSTriples() here

2233

This annotation will overwrite any previous category annotations that the user has previously added. I wasn't sure whether we wanted this behavior when I was writing the decorator, but now that we have these automatic categorizations, it definitely sounds like a bad idea. add_test_categories should really "add" categories to the test.

Since this is quite orthogonal to your patch, I'll whip up a separate patch to do that.

[labath]

packages/Python/lldbsuite/test/lldbtest.py

2226–2227

Actually, getCategoriesForTest(attrvalue) will not work here, as the test is not running and the class is not constructed yet. After D15451 you should be able to do something like this here:

categories = getattr(attrvalue, "categories", [])
if not set(categories).intersection(["dwarf", "dsym", "dwo"]):
  categories = ["dwarf", "dsym", "dwo"]
dont_do_dsym_test = any(platform in target_platform for platform in ["linux", "freebsd", "windows"]) or ("dsym" not in categories)
dont_do_dwo_test = any(platform in target_platform for platform in getDarwinOSTriples()) or ("dwo" not in categories)
dont_do_dwarf_test = "dwarf" not in categories

This way, if the test is categorized with a debug info marker, we use that info to choose which tests to generate. If the test is not categorized, we just use the default platform rules.

Zach, Tamas, how does that sound?

2233

I have this up it D15451.

[zturner]

In D15428#308131, @labath wrote:

The interaction between the add_test_categories and the "magic test multiplier" is causing problems, which is evident in the fact how you needed to add the @skip decorator to make it work.

It sounds like we need to decide what will be the "correct" interaction of the "test multiplier" and tests manually marked with a specific debug info.

I think the most sensible behavior would be to treat a manual debuginfo mark as a signal that you don't want your test to be auto-multiplied, the test multiplier could then check whether the test is already marked with one of the debuginfo annotations, and if it is, then avoid touching it.

In hindsight I wonder if I even needed to manually mark the test with a debug info. Maybe we can say that tests should *never* be manually marked with a debug info category, and only use the skip decorators. That should still work, because it will be auto-multiplied to every debug format, and then some of them will be skipped.

[zturner]

Updated with comments from previous review. Also change the multiplication logic to use a loop instead of writing each one separately.

[tberghammer]

packages/Python/lldbsuite/test/lang/cpp/limit-debug-info/TestWithLimitDebugInfo.py

9–12

Considering your change in lldbtest.py:2227 I think you should annotate the function with add_test_categories(["dwarf"]) instead

packages/Python/lldbsuite/test/lldbtest.py

2227–2228

I think this won't work if the list of debug info formats are specified on class level instead of at function level.

[labath]

In D15428#308401, @zturner wrote:

In hindsight I wonder if I even needed to manually mark the test with a debug info. Maybe we can say that tests should *never* be manually marked with a debug info category, and only use the skip decorators. That should still work, because it will be auto-multiplied to every debug format, and then some of them will be skipped.

I quite like that. @dwarf_test essentially means "skip if debug info is not dwarf", so why not state that explicitly... I'm not going to ask you to rewrite it again, but if you feel like it, then by all means go for it.

packages/Python/lldbsuite/test/lldbtest.py

2227–2228

That's true, but given that it's difficult to access the class annotations from this context, I'm fine with living with this "defect", at least for now. Although, it sounds like that if we go with the option Zachary proposes, this problem would go away...

packages/Python/lldbsuite/test/configuration.py

 # The test suite.
 suite = unittest2.TestSuite()
 
-dont_do_dsym_test = False
-dont_do_dwarf_test = False
-dont_do_dwo_test = False
-
 # The list of categories we said we care about
 categoriesList = None
 # set to true if we are going to use categories for cherry-picking test cases

packages/Python/lldbsuite/test/dotest.py

         cflags_extras = args.E
         os.environ['CFLAGS_EXTRAS'] = cflags_extras
 
-    # argparse makes sure we have correct options
-    if args.N == 'dwarf':
-        configuration.dont_do_dwarf_test = True
-    elif args.N == 'dwo':
-        configuration.dont_do_dwo_test = True
-    elif args.N == 'dsym':
-        configuration.dont_do_dsym_test = True
-
     if args.d:
         sys.stdout.write("Suspending the process %d to wait for debugger to attach...\n" % os.getpid())
         sys.stdout.flush()

 
     target_platform = lldb.DBG.GetSelectedPlatform().GetTriple().split('-')[2]
 
-    # By default, both dsym, dwarf and dwo tests are performed.
-    # Use @dsym_test, @dwarf_test or @dwo_test decorators, defined in lldbtest.py, to mark a test as
-    # a dsym, dwarf or dwo test.  Use '-N dsym', '-N dwarf' or '-N dwo' to exclude dsym, dwarf or
-    # dwo tests from running.
-    configuration.dont_do_dsym_test = configuration.dont_do_dsym_test \
-        or any(platform in target_platform for platform in ["linux", "freebsd", "windows"])
-    configuration.dont_do_dwo_test = configuration.dont_do_dwo_test \
-        or any(platform in target_platform for platform in ["darwin", "macosx", "ios"])
-
     # Don't do debugserver tests on everything except OS X.
     configuration.dont_do_debugserver_test = "linux" in target_platform or "freebsd" in target_platform or "windows" in target_platform
 

packages/Python/lldbsuite/test/functionalities/unwind/standard/TestStandardUnwind.py

 # Generate test cases based on the collected source files
 for f in test_source_files:
     if f.endswith(".cpp") or f.endswith(".c"):
-        @dwarf_test
+        @add_test_categories("dwarf")
         @unittest2.skipIf(TestBase.skipLongRunningTest(), "Skip this long running test")
         def test_function_dwarf(self, f=f):
             if f.endswith(".cpp"):

packages/Python/lldbsuite/test/lang/c/typedef/Testtypedef.py

 
     mydir = TestBase.compute_mydir(__file__)
 
-    @skipUnlessDarwin
-    @dsym_test
-    @expectedFailureClang("llvm.org/pr19238")
-    def test_with_dsym(self):
+    @expectedFailureAll(bugnumber="llvm.org/pr19238", compiler="clang")
+    @expectedFailureAll(bugnumber="llvm.org/pr25626 expectedFailureClang fails on FreeBSD", oslist=["freebsd"])
+    def test_typedef(self):
         """Test 'image lookup -t a' and check for correct display at different scopes."""
-        self.buildDsym()
-        self.image_lookup_for_multiple_typedefs()
-
-    @dwarf_test
-    @expectedFailureClang("llvm.org/pr19238")
-    @expectedFailureFreeBSD("llvm.org/pr25626 expectedFailureClang fails on FreeBSD")
-    def test_with_dwarf(self):
-        """Test 'image lookup -t a' and check for correct display at different scopes."""
-        self.buildDwarf()
+        self.build()
         self.image_lookup_for_multiple_typedefs()
 
     def image_lookup_for_multiple_typedefs(self):

packages/Python/lldbsuite/test/lang/cpp/limit-debug-info/TestWithLimitDebugInfo.py

 
     mydir = TestBase.compute_mydir(__file__)
 
-    @dwarf_test
-    def test_with_dwarf(self):
-        self.buildDwarf()
+    @add_test_categories("dwarf")
+    @skipIf(debug_info=not_in(["dwarf"]))
+    def test_limit_debug_info(self):
+        self.build()
 
         cwd = os.getcwd()
 

packages/Python/lldbsuite/test/lldbtest.py

     wrapper.__no_debug_info_test__ = True
     return wrapper
 
-def dsym_test(func):
-    """Decorate the item as a dsym test."""
-    if isinstance(func, type) and issubclass(func, unittest2.TestCase):
-        raise Exception("@dsym_test can only be used to decorate a test method")
-    @wraps(func)
-    def wrapper(self, *args, **kwargs):
-        if configuration.dont_do_dsym_test:
-            self.skipTest("dsym tests")
-        return func(self, *args, **kwargs)
-
-    # Mark this function as such to separate them from the regular tests.
-    wrapper.__dsym_test__ = True
-    return wrapper
-
-def dwarf_test(func):
-    """Decorate the item as a dwarf test."""
-    if isinstance(func, type) and issubclass(func, unittest2.TestCase):
-        raise Exception("@dwarf_test can only be used to decorate a test method")
-    @wraps(func)
-    def wrapper(self, *args, **kwargs):
-        if configuration.dont_do_dwarf_test:
-            self.skipTest("dwarf tests")
-        return func(self, *args, **kwargs)
-
-    # Mark this function as such to separate them from the regular tests.
-    wrapper.__dwarf_test__ = True
-    return wrapper
-
-def dwo_test(func):
-    """Decorate the item as a dwo test."""
-    if isinstance(func, type) and issubclass(func, unittest2.TestCase):
-        raise Exception("@dwo_test can only be used to decorate a test method")
-    @wraps(func)
-    def wrapper(self, *args, **kwargs):
-        if configuration.dont_do_dwo_test:
-            self.skipTest("dwo tests")
-        return func(self, *args, **kwargs)
-
-    # Mark this function as such to separate them from the regular tests.
-    wrapper.__dwo_test__ = True
-    return wrapper
-
 def debugserver_test(func):
     """Decorate the item as a debugserver test."""
     if isinstance(func, type) and issubclass(func, unittest2.TestCase):

         newattrs = {}
         for attrname, attrvalue in attrs.items():
             if attrname.startswith("test") and not getattr(attrvalue, "__no_debug_info_test__", False):
-                @dsym_test
-                @wraps(attrvalue)
-                def dsym_test_method(self, attrvalue=attrvalue):
-                    self.debug_info = "dsym"
-                    return attrvalue(self)
-                dsym_method_name = attrname + "_dsym"
-                dsym_test_method.__name__ = dsym_method_name
-                newattrs[dsym_method_name] = dsym_test_method
-
-                @dwarf_test
+                target_platform = lldb.DBG.GetSelectedPlatform().GetTriple().split('-')[2]
+
+                dont_do_dsym_test = any(platform in target_platform for platform in ["linux", "freebsd", "windows"])
+                dont_do_dwo_test = any(platform in target_platform for platform in ["darwin", "macosx", "ios"])
+
+                if not dont_do_dsym_test:
+                    @add_test_categories(["dsym"])
+                    @wraps(attrvalue)
+                    def dsym_test_method(self, attrvalue=attrvalue):
+                        self.debug_info = "dsym"
+                        return attrvalue(self)
+                    dsym_method_name = attrname + "_dsym"
+                    dsym_test_method.__name__ = dsym_method_name
+                    newattrs[dsym_method_name] = dsym_test_method
+
+                @add_test_categories(["dwarf"])
                 @wraps(attrvalue)
                 def dwarf_test_method(self, attrvalue=attrvalue):
                     self.debug_info = "dwarf"

                 dwarf_test_method.__name__ = dwarf_method_name
                 newattrs[dwarf_method_name] = dwarf_test_method
                 
-                @dwo_test
-                @wraps(attrvalue)
-                def dwo_test_method(self, attrvalue=attrvalue):
-                    self.debug_info = "dwo"
-                    return attrvalue(self)
-                dwo_method_name = attrname + "_dwo"
-                dwo_test_method.__name__ = dwo_method_name
-                newattrs[dwo_method_name] = dwo_test_method
+                if not dont_do_dwo_test:
+                    @add_test_categories(["dwo"])
+                    @wraps(attrvalue)
+                    def dwo_test_method(self, attrvalue=attrvalue):
+                        self.debug_info = "dwo"
+                        return attrvalue(self)
+                    dwo_method_name = attrname + "_dwo"
+                    dwo_test_method.__name__ = dwo_method_name
+                    newattrs[dwo_method_name] = dwo_test_method
             else:
                 newattrs[attrname] = attrvalue
         return super(LLDBTestCaseFactory, cls).__new__(cls, name, bases, newattrs)

packages/Python/lldbsuite/test/test_categories.py

 
 all_categories = {
     'dataformatters': 'Tests related to the type command and the data formatters subsystem',
+    'dwarf'         : 'Tests that can be run with DWARF debug information',
+    'dwo'           : 'Tests that can be run with DWO debug information',
+    'dsym'          : 'Tests that can be run with DSYM debug information',
     'expression'    : 'Tests related to the expression parser',
     'objc'          : 'Tests related to the Objective-C programming language support',
     'pyapi'         : 'Tests related to the Python API',