[Lldb-commits] [PATCH] D15428: Make debug info specification use categories system

Pavel Labath via lldb-commits lldb-commits at lists.llvm.org
Fri Dec 11 02:32:57 PST 2015

labath requested changes to this revision.
labath added a comment.
This revision now requires changes to proceed.

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?

Comment at: packages/Python/lldbsuite/test/functionalities/unwind/standard/TestStandardUnwind.py:120
@@ -119,3 +119,3 @@
     if f.endswith(".cpp") or f.endswith(".c"):
-        @dwarf_test
+        @add_test_categories("dwarf")
         @unittest2.skipIf(TestBase.skipLongRunningTest(), "Skip this long running test")
You need `[]` around the `"dwarf"`. Otherwise, you will get errors here, when the skipIf below does not actually skip.

Comment at: packages/Python/lldbsuite/test/lang/cpp/limit-debug-info/TestWithLimitDebugInfo.py:10
@@ +9,3 @@
+    @add_test_categories("dwarf")
+    @skipIf(debug_info=not_in(["dwarf"]))
+    def test_limit_debug_info(self):
It should not be necessary to annotate and skip at the same time.

Comment at: packages/Python/lldbsuite/test/lldbtest.py:2226
@@ -2267,2 +2225,3 @@
         for attrname, attrvalue in attrs.items():
             if attrname.startswith("test") and not getattr(attrvalue, "__no_debug_info_test__", False):
+                target_platform = lldb.DBG.GetSelectedPlatform().GetTriple().split('-')[2]
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...

Comment at: packages/Python/lldbsuite/test/lldbtest.py:2230
@@ +2229,3 @@
+                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"])
might as well use `getDarwinOSTriples()` here

Comment at: packages/Python/lldbsuite/test/lldbtest.py:2233
@@ +2232,3 @@
+                if not dont_do_dsym_test:
+                    @add_test_categories(["dsym"])
+                    @wraps(attrvalue)
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.


More information about the lldb-commits mailing list