Page MenuHomePhabricator

[testsuite] Split Objective-C data formatter
ClosedPublic

Authored by JDevlieghere on Apr 4 2019, 6:43 PM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

JDevlieghere created this revision.Apr 4 2019, 6:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2019, 6:43 PM

Fix CF test

davide added a comment.Apr 4 2019, 6:45 PM

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.

aprantl added inline comments.Apr 5 2019, 8:40 AM
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/ObjCDataFormatterTestCase.py
21 ↗(On Diff #193822)

All uses of this variable should also be replaced by lldbutil.run_to_source_breakpoint()

33 ↗(On Diff #193822)

i.e.: this

aprantl added inline comments.Apr 5 2019, 8:41 AM
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjNSException.py
30 ↗(On Diff #193822)

It would be cleaner to replace
self.expect('frame var'

with lldbutil.check_variable()

but that's less important.

JDevlieghere marked 5 inline comments as done.

Code review feedback

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.

I see 120 -> 12 seconds on my iMac Pro.

davide accepted this revision.Apr 5 2019, 10:54 AM

great!

This revision is now accepted and ready to land.Apr 5 2019, 10:54 AM

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.

I see 120 -> 12 seconds on my iMac Pro.

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?

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2019, 10:58 AM

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?

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?

Sounds good, I'll remove the decorators and we can do this incrementally.

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.

I see 120 -> 12 seconds on my iMac Pro.

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?

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

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.

I see 120 -> 12 seconds on my iMac Pro.

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?

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