The testcase for objective-c data formatters is very big as it checks a bunch of stuff. This is annoying when using the lit test driver, because it prevents us from running the different cases in parallel. As a result, it's always one of the last few tests that complete. This patch splits the test into multiple files that share a common base class. This way lit can run the different tests in parallel.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
I think this is good regardless for readability, but I would really appreciate if we can collect some numbers to see how effective this change actually is.
Looks fine to me, with some inline ideas you can implement if you think they're worthwhile.
Also, since you're looking at reducing test time, the question you can ask yourself is: is it really worth it running separate dwarf+dsym flavours of these tests? I'm of the view that this should not be necessary, as the data formatters operate on high level abstractions, which should hide any differences in the debug info format (and the fact that the debug info parsers generate correct and consistent abstractions should be tested elsewhere). However, I don't have a horse in this race, so the decision is entirely up to you...
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCCF.py | ||
---|---|---|
21–23 ↗ | (On Diff #193822) | I'm pretty sure these are unneeded, as all you do is call the overridden method. |
29–32 ↗ | (On Diff #193822) | I think this pattern is a relict from the days when we didn't have the magic test multiplicator. In those days, tests were written as: def test_foo_dsym(self): self.buildDsym() self.foo() def test_foo_dwarf(self): self.buildDwarf() self.foo() def foo(self): # do the real stuff here However, now that we can generate test flavours automatically, I don't think it makes much sense. So, unless you find it useful for some reason, I think we can just remove the helper function (by deleting these four lines). |
45–53 ↗ | (On Diff #193822) | It doesn't look like this test is doing anything to the type summaries, so I don't think it's necessary to clean up anything here. |
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjNSException.py | ||
---|---|---|
30 ↗ | (On Diff #193822) | It would be cleaner to replace with lldbutil.check_variable() but that's less important. |
Nice! Might be nice to have some automated info that gets dumped when one file with multiple tests takes a long time and suggest breaking them up in the python test infrastructure modules. Might be other nice gains to be had?
What's the motivation behind all the @no_debug_info_test decorators? Are those codepaths guaranteed to be tested elsewhere?
What's the motivation behind all the @no_debug_info_test decorators? Are those codepaths guaranteed to be tested elsewhere?
As a rule of thumb I'd say, let's only stick @no_debug_info_test on tests where a (future) coverage bot proves that it is safe. How does that sound?
lit has this feature. It's called --time-tests. That's how we discovered the problem. There are other tests that we can fix.
--Davide
And, as you ask, these are the slowest tests for now.
68.25s: lldb-Suite :: types/TestIntegerTypes.py 86.52s: lldb-Suite :: types/TestIntegerTypesExpr.py 95.66s: lldb-Suite :: lang/objc/foundation/TestObjCMethods2.py 131.24s: lldb-Suite :: lang/objc/objc-new-syntax/TestObjCNewSyntax.py