Deprecate the `subsystem_instance` utility function.
Review Request #4220 — Created Sept. 11, 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).
It's unclear why this helper doesn't need to call
Subsystem.reset, but the helper in
Should indicate that this always has the sideeffect of reconfiguring the subsystem with the given options. Or does it?
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.