[Lldb-commits] [PATCH] D131539: [lldb] Make sure all Python API tests are marked as NO_DEBUG_INFO_TESTCASE

Wed Aug 10 08:02:03 PDT 2022

JDevlieghere added a comment.

In D131539#3711835 <https://reviews.llvm.org/D131539#3711835>, @labath wrote:

> In D131539#3711518 <https://reviews.llvm.org/D131539#3711518>, @JDevlieghere wrote:
>
>> In D131539#3711488 <https://reviews.llvm.org/D131539#3711488>, @kastiglione wrote:
>>
>>> this diff has made me wonder: should we have a `NoDebugInfoTestCase` that can be used by any test, and would replace assigning to `NO_DEBUG_INFO_TESTCASE`?
>>
>> I was wondering the same thing. I decided against it because we already have `NO_DEBUG_INFO_TESTCASE` (test level) and `@no_debug_info_test` (function level) and I didn't want to add yet another option. Additionally, there's a few other patters that are common for the Python API tests (e.g. the `self.source` and `self.line`) that could be moved up into the base class in a follow up patch.
>
> Note that we already kind of have that. The `Base` test class is the base of all our tests and does not support automatic test replication. Tests which inherit directly from that (lldb-server and lldb-vscode tests) for instance, don't get the replication even though they don't use `NO_DEBUG_INFO_TESTCASE`. The `TestBase` class (which we use for the "normal" SB API tests) adds a bunch of SB utility functions *and* it adds the ability to replicate tests. I think that separating the two parts would be completely reasonable, and would remove the need for most/all of the uses of `NO_DEBUG_INFO_TESTCASE` and `@no_debug_info_test`.

The ability to replicate tests is not even part of `TestBase` I think. Except for the attribute, it's all handled by the `LLDBTestCaseFactory`. If you think it's better to go the test case route (in favor of NO_DEBUG_INFO_TESTCASE) we can pull up everything but the factory and property from `TestBase` into `Base` (as you outlined) or keep them separate and just move the factory and property down to a (third) child class. Either way we should rename those 2 (3) classes to something more meaningful.

@labath: was your comment meant as a suggestion (i.e. we should make this change) or just informative in reply to Dave's question (i.e. we could make this change if we needed it)?

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131539/new/

https://reviews.llvm.org/D131539