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

[Pavel Labath via lldb-commits]

labath added inline comments.

================
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."""
----------------
tfiala wrote:
> labath wrote:
> > 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).
> Yes, I see now.  I did not understand add_test_categories(["gmodules"]) was a valid way to mark a test as gmodules-only.
> 
> It does look a little weird that we need to do:
> @no_debug_info_test
> @add_test_categories(["gmodules"])
> 
> I wonder if we want a combined decorator that is effectively:
> @valid_debug_info([...])
> 
> that specifies that this test is only valid for those debug info entries specified.  Then the entry becomes a single decorator:
> @valid_debug_info(["gmodules"])
> 
> What do you think?
You shouldn't need both decorators. The test multiplicator will first check whether whether the test is already explicitly annotated with some debug info category (lldbtest.py:1446) and multiply only into the categories that were not annotated.

Now that I look at it closer, I see that the code there is not entirely correct in case you annotate a test with two categories (it will create two tests, but both of them will be annotated with both categories). However, this is a topic for another change, and should not prevent you from using it now.

So, yes, you should only need one decorator, and things should just work. As for the current syntax, I am open to making any changes to it, but I want to avoid having two ways of doing this in parallel.


http://reviews.llvm.org/D19998