[Lldb-commits] [PATCH] D19998: Add a "-gmodules" category to the test suite.

[Pavel Labath via lldb-commits]

labath added a comment.

I think we should make one more iteration on this, as there is still some room for improvement.

In http://reviews.llvm.org/D19998#438756, @zturner wrote:

> Should this be disabled by default unless explicitly requested? Seems like
>  "run the entire test suite N times" should be opt in, not opt out.


I think it makes sense to run multiple variants of some tests, as "gmodules" is a regular debugger feature and we want to make sure we support that feature. What I think is a problem here is the "whole test suite" part, as I think a lot of tests (think, all process control and thread tests) don't (or shouldn't) depend on debug info (see discussion early on in this thread). My feeling is that we should start restricting the duplication more aggressively, and then it won't matter that **some** tests get run multiple times.

That said, if you as the windows maintainer say that you don't want to support gmodules debugging on windows at the moment, then I think it's fine to skip those tests on this platform.


================
Comment at: packages/Python/lldbsuite/support/gmodules.py:19
@@ -19,2 +19,2 @@
         compiler = os.path.basename(compiler_path)
         if "clang" not in compiler:
----------------
This diff looks broken. This appears to be a new file (I certainly don't have it in my tree), and it is being shown as a diff. Could you reupload a correct diff against the current top of tree?

================
Comment at: packages/Python/lldbsuite/support/gmodules.py:22
@@ -21,3 +21,3 @@
             return False
-        clang_help = os.popen("%s --help" % compiler_path).read()
-        return GMODULES_HELP_REGEX.search(clang_help, re.DOTALL) is not None
+        elif os.name == "nt":
+            # gmodules support is broken on Windows
----------------
This check should be folded into `test_categories.is_supported_on_platform`. (Also, it is incorrect as it should presumably be checking against the target platform and not the host.)

================
Comment at: packages/Python/lldbsuite/test/decorators.py:527
@@ -525,3 +526,3 @@
 
-def skipUnlessClangModules():
+def skipUnlessClangModules(func):
     """Decorate the item to skip test unless Clang -gmodules flag is supported."""
----------------
This whole decorator can be removed. It's invocations can be replaced with `add_test_categories(["gmodules"])` (cf. `lldbtest.py:1453`, it will avoid duplicating the test if it is already annotated with categories).

================
Comment at: packages/Python/lldbsuite/test/lang/cpp/gmodules/TestWithModuleDebugging.py:13
@@ -12,1 +12,3 @@
+    @skipUnlessClangModules
     def test_specialized_typedef_from_pch(self):
+        self.buildGModules()
----------------
replace these two decorators with `add_test_categories(["gmodules"])`

================
Comment at: packages/Python/lldbsuite/test/lldbinline.py:148
@@ +147,3 @@
+        self.BuildMakefile()
+        self.buildDwarf()
+        self.do_test()
----------------
`buildGModules()` ?


http://reviews.llvm.org/D19998