Deprecate the `subsystem_instance` utility function.

Review Request #4220 — Created Sept. 11, 2016 and submitted

benjyw
pants
pants-reviews
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:

44be4da8bda29e21061e550b6529a346934b9b0f

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

  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
MA
  1. 
      
  2. Just FYI that this could potentially raise a confusing looking TypeError if any of the subsystem_types aren't a class member, like a string or something.

    You could consider moving the type check up to the Subsystem class,

    1. Ha - Just now reworking the Docker plugin to get it ready for OSS and I hit where you already have both of the above points considered.

      @staticmethod
      def _is_subsystem_type(obj):
      return inspect.isclass(obj) and issubclass(obj, Subsystem)

    2. Fixed (moved that function into Subsystem, and both call sites use that now)

  3. Could just use a set comprehension here.

  4. 
      
BE
BE
BE
ST
  1. 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).

  2. It's unclear why this helper doesn't need to call Subsystem.reset, but the helper in JarDependencyManagementTest does.

    1. The latter is called multiple times in a single test, so it needs to manually reset its state. I added a comment to that effect there.

  3. Should indicate that this always has the sideeffect of reconfiguring the subsystem with the given options. Or does it?

    1. This is actually tricky, and I'll try to clarify in the comments. But if the subsystrem was already initialized then it won't re-initialize, but it will return the global instance from the previous initialization. This is unavoidable since we don't have a way to reset only specfic subsystems.

      But I'll see if I can detect this case and yell about it.

    2. Added a TODO and an issue to clean this corner up. I have a plan for how to do so.

  4. 
      
JS
  1. 
      
  2. Importing a setup utility reads as a red flag, where is the paired teardown? How about introducing a helper method in BaseTest that 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.TestCase subclasses in this review that are using global_subsystem_instance/init_subsystems without even attempting a teardown, so maybe the BaseTest.tearDown call to Subsystem.reset() is not even needed; though, if so, I suspect that would be a bit of luck.

    1. It is a bit of a red flag, but it's flagging a real problem: namely that while we can ask to set up subsystems one at a time, we can (currently) only tear them down all at once, in Subsystem.reset(). So we have no choice but to rely on a reset in setUp and/or tearDown (we don't strictly need both).

      The contextmanager approach ignored this: so you'd think you were setting up a single subsystem and then tearing it down, but in fact you were tearing down all subsytem state. This is what my changes are trying to get rid of.

      So this red flag does at least reflect reality. And this was true in general even before we had subsystems: once we set up options (say in self.context(...)) they stay set up throughout the test. The only thing we can rely on is that at the start of each test we're in a clean state (due to the Subsystem.reset() in setUp()).

      And note that it's not clear that selectively tearing down subsystems is generally possible. For example, what happens to the subsystems that subsystem depends on? Do we clear them up too? Do we clear up subsystems that depend on the one we're tearing down?

      We've ignored all these issues by effectively saying "once you set up an optionable in a test, it's set up unless you nuke the entire state", and I'm not sure how easy it would be to get away from that.

    2. K - thanks for the reality check.
  3. 
      
BE
Review request changed

Status: Closed (submitted)

Change Summary:

3b77f6b6c38fea6616cae10ca8e0c3a20782934a

Loading...