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

[Pavel Labath via lldb-commits]

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.


http://reviews.llvm.org/D15428