Page MenuHomePhabricator

[lldb] Add a SubsystemRAII that takes care of calling Initialize and Terminate in the unit tests
ClosedPublic

Authored by teemperor on Dec 17 2019, 2:08 PM.

Details

Summary

Many of our tests need to initialize certain subsystems/plugins of LLDB such as
FileSystem or HostInfo by calling their static Initialize functions before the
test starts and then calling ::Terminate after the test is done (in reverse order).
This adds a lot of error-prone boilerplate code to our testing code.

This patch adds a RAII called SubsystemRAII that ensures that we always call
::Initialize and then call ::Terminate after the test is done (and that the Terminate
calls are always in the reverse order of the ::Initialize calls). It also gets rid of
all of the boilerplate that we had for these calls.

Per-fixture initialization is still not very nice with this approach as it would
require some kind of static unique_ptr that gets manually assigned/reseted
from the gtest SetUpTestCase/TearDownTestCase functions. Because of that
I changed all per-fixture setup to now do per-test setup which can be done
by just having the SubsystemRAII as a member of the test fixture. This change doesn't
influence our normal test runtime as LIT anyway runs each test case separately
(and the Initialize/Terminate calls are anyway not very expensive). It will however
make running all tests in a single executable slightly slower.

Diff Detail

Event Timeline

teemperor created this revision.Dec 17 2019, 2:08 PM
Herald added a reviewer: shafik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript

I only converted like 40% of our tests to the new system as I first wanted to get some feedback before I do all the refactoring. Also I'm open to ideas how to name the member (currently I just use subsystems without the m_ prefix as it's not really a real member with any contents).

We have at least one Initialize function (Socket::Initialize) which can return an llvm::Error (and it is used in some unit tests). What's your plan to handle that?

Assuming we can come up with something there, I think this would be a great utility. I wouldn't be opposed to using this for the main initialization code too...

martong resigned from this revision.Dec 19 2019, 2:23 AM
teemperor updated this revision to Diff 234711.Dec 19 2019, 6:26 AM
  • Added SFINAE spaghetti to handle llvm::Error return types.
labath accepted this revision.Dec 19 2019, 7:45 AM

Yep, looks good.

lldb/unittests/TestingSupport/SubsystemRAII.h
60

This doesn't have to be a template since it's already parameterized by the enclosing T. Or, you could actually move it outside the SubsystemRAII class (and possibly into a detail namespace) and save the compiler some work then Using the same class in different subsystem sequences.

This revision is now accepted and ready to land.Dec 19 2019, 7:45 AM
shafik added inline comments.Dec 19 2019, 9:49 AM
lldb/unittests/TestingSupport/SubsystemRAII.h
58

Should we write a test for this?

teemperor updated this revision to Diff 234839.Dec 20 2019, 1:54 AM
  • Add test
  • Move SubsystemRAIICase to ::detail

I'll wait with landing this until HostInfo's Initialize/Terminate are fixed (calling Terminate and Initialize is currently breaking the whole thing)

teemperor marked 2 inline comments as done.Dec 20 2019, 1:54 AM
teemperor updated this revision to Diff 234860.Dec 20 2019, 4:50 AM
  • Migrated more tests to SubsystemRAII
This revision was automatically updated to reflect the committed changes.