A base class for tests that test a single task type.

Review Request #1168 — Created Oct. 15, 2014 and submitted

benjyw
pants
f8fdfbe...
pants-reviews
jsirois, patricklaw

Subclasses advertise the task type they care about in setupClass().

Deals with option registration and scoping for the task under test.

Also addresses some small code review comments from a previous commit.

ci.sh passed.

PA
  1. Ship It!

  2. 
      
JS
  1. 
      
    1. Thanks for the review. Because merging the two TaskTest concepts is a little involved, I will do it in several separate commits, all in this review. That way you can review them one at a time, or all together, however you prefer to use RB.

      Will notify when ready for further review.

    2. Update: merging those two TaskTest concepts turns out to be a major yak shave: different tests use each style to create and pass around Context and Task instances in different ways. So I will hold off on that for now. It will become a lot easier after we're entirely on new options, because many of the differences involve how config and args are plumbed in.

      For now I've added copious comments about what needs to be done, and I've completed the first part of the yak shave, moving prepare_task() into TaskTest, which you can review here. I've also used this as the idiom for advertising the task type under test, instead of an @abstractproperty:

      @classmethod
      def task_type(cls):
      return MyTask

      Because this is what TaskType does, and this will make merging those concepts a little bit easier.

  2. tests/python/pants_test/base_test.py (Diff revision 1)
     
     

    alpha

  3. Already confusing, getting a bit more confusing: https://github.com/pantsbuild/pants/blob/master/tests/python/pants_test/tasks/test_base.py#L85

    Strongly consider tacking a TODO here to aggregate or else move this stuff there now - it has slightly more obvious packaging although really the Task test baseclass should probably live in pants_test.backend.core.tasks.

    1. Ooops, I completely blanked on the existence of this class. Will merge my stuff into this one, It looks like almost nothing currently uses it?

      I won't move it to pants_test.backend.core.tasks yet because I'm not fully convinced that's where it should go. We probably want to distinguish helpers from tests of the tasks in pants.backends.core.tasks, although I'm not sure what the right structure. I'll let you make that call separately.

  4. Consider an @abstractproperty instead. The usage is from the instance, not the class and its nice to avoid forcing super(...) gymnastics whenever possible imo.

    1. That is nicer. But if I merge this into TaskTest, that already has a classmethod for task_type, so I will use that instead. [Or rather its ConsoleTaskTest subclass does, but I will move that into TaskTest).

  5. 
      
BE
JS
  1. Ship It!

  2. 
      
IT
  1. Ship It!

  2. 
      
BE
Review request changed

Status: Closed (submitted)

Loading...