Deprecate the `subsystem_instance` utility function.
Review Request #4220 - Created Sept. 10, 2016 and submitted
|jsirois, mateor, stuhood|
It was implemented as a contextmanager that resets subsystem
state in __exit__. However this interacts badly with tests that
use that state (sometimes without even realizing it) outside the
subsystem_instance context. Several tests have to do weird
balancing acts to get around this.
Instead we introduce a global_subsystem_instance() that simply
inits and returns the global instance of a subsystem. Test code
can rely on our standard test base classes for cleanup between tests,
or it can reset subsystem state itself if it needs to do so mid-test.
This change allows us to remove various wrapper contextmanagers
sprinkled around our tests, and generally supports simplification
of several tests.
This is part 2 of my effort to simplify and standardize how we
create subsystems in tests. See for part 1:
Note to reviewers: The diff is a little hairy, but the focus is on
the change in tests/python/pants_test/subsystem/subsystem_util.py.
The rest is just modifying all relevant tests to use the new method,
and the proof of the validity of those is mostly that the tests pass...
CI passes: https://travis-ci.org/pantsbuild/pants/builds/159169092
Revision 3 (+835 -797)
This seems like it cleans up a bunch of usage: thanks.
It would be awesome to try and set a clear distinction in this API between tests that instantiate a Subsystem for sideeffects (because they know that something they're about to call will need it to have been initialized) and for those that simply want to pass an instance of a Subsystem to some code (ie, the DI usecase).
Importing a setup utility reads as a red flag, where is the paired teardown? How about introducing a helper method in
BaseTestthat in turn uses
global_subsystem_instance. At least then reading
self.subsystem_instance(...)gives a clue that self may take care of teardown.
I guess there are several bare
unittest.TestCasesubclasses in this review that are using
init_subsystemswithout even attempting a teardown, so maybe the
Subsystem.reset()is not even needed; though, if so, I suspect that would be a bit of luck.