[Lldb-commits] [PATCH] D134344: [WIP][lldb][test] 1 - Remove gmodules debug_info variant: add decorator for API tests that explicitly test gmodules

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D134344#3805953 <https://reviews.llvm.org/D134344#3805953>, @Michael137 wrote:

> that would require an audit of each API test right?

Not really. I think this could be a purely mechanical change which replaces `NO_DEBUG_INFO_TESTCASE = False` with something different. Ideally I'd make that something a "inheriting from a different class". So we could have something like `APITestCase` and a `DebugInfoTestCase` (inheriting from the first), and the tests which want debug info replication (one can also imagine different kinds of replication for some other tests) would inherit from the latter.

In D134344#3806509 <https://reviews.llvm.org/D134344#3806509>, @aprantl wrote:

> But I'm also missing the context as to why this would be desirable, so if there's a good reason, let me know!

I have two reasons for that:
The first is that from a simply engineering perspective, doing it the other way around seems cleaner, as now we kind of have two ways to avoid replication. Either you don't inherit from the replicated test base clase (all lldb-server and lldb-vscode tests do that), or you do, but then mark yourself as NO_DEBUG_INFO_TESTCASE.
Secondly, it seems to be that no-replication is a better default. We have a lot of features that don't (or shouldn't) depend on the kind of debug info we're using, and we're probably forgetting to add this attribute to some of them. It's possible those tests are adding some marginal debug info coverage, but it's hard to rely on that, because noone know what that is. So I'd say that an opt-in is a better default (particularly for the tests we're adding nowadays), and the replication should be done when you know you're doing something debug-heavy.
I also think that having a more opt-in mechanism could enable us to do *more* replication. For example, I think that running the some tests in both DWARF v4 and v5 modes would be interesting, but I definitely wouldn't want to run all of them, all the time.

In D134344#3811091 <https://reviews.llvm.org/D134344#3811091>, @Michael137 wrote:

>> (B) Keep the `gmodules` category in the debug_info categories but add an indicator (e.g., by making the `debug_info_categories` a dictionary) that will skip replication if set. That would solve (1). And (2) would work as it does today without changes.
>
> Uploaded alternative diff that implements this option. Seems simpler since tests in the `gmodules` category Just Work and we don't need to special-case `gmodules` in several places
>
> https://reviews.llvm.org/D134524

It's not exactly how I was imagining this, but I like it. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134344