Create test task mixin

Review Request #2902 — Created Sept. 29, 2015 and submitted

sameerbrenn
pants
b34075d...
pants-reviews
benjyw, jsirois, stuhood

In order to minimize duplicated code between test tasks for different languages, here is a TestTaskMixin that will keep functionality shared among tests for all languages.

- Filters target list according to _get_targets method in language-specific class
- Validates targets according to _validate_targets method in language-specific class
- --skip option will skip tests

Added test:
./pants test tests/python/pants_test/backend/core/task:test_task_mixin

All tests pass:
./pants test tests/python/pants_test:all

Pull request: https://github.com/pantsbuild/pants/pull/2288

  • 0
  • 0
  • 4
  • 3
  • 7
Description From Last Updated
ST
  1. One important rule of multiple inheritance is that you should have only one constructor at each level. So in this case, precisely one of JUnitRuns parents should provide a constructor. In this case, both your new class and JvmTask provide a constructor, which is a nono.

    Instead, this should probably be called and used as a "mixin"... meaning that it assumes that it will be mixed into a particular type (in this case, TaskBase) but it does not inherit from that class on its own. You can see that JUnitRun already extends one such mixin.

    1. Thanks Stu, that makes sense, I'll make it a mixin and won't inherit TaskBase-- however, JvmToolTaskMixin (which JUnitRun extends) does inherit from TaskBase. Is that an error? For testing I removed that inheritance and all the tests still pass, but IntelliJ tells me that super(JvmToolTaskMixin, cls).prepare is unresolved.

  2. 
      
JT
  1. I'm new to working on pants so please take this with a grain of salt, but I do want to encourage reuse via composition rather than inheritance. While there's very little functionality in the base test class at this point, the more that goes in there the more difficult it is for me to test classes down the inheritance chain in isolation (i.e. without executing superclass code). The reuse via inheritance pattern is pretty common in the code I've looked at, and maybe it's the idiomatic way to do things in python, but I think we'll find it easier to write good UTs if we restrict inheritance to interface definitions.

  2. 
      
SA
BE
  1. This still says "WIP". Please update, or create a new RB, when this is ready for full review.

  2. 
      
SA
SA
ST
  1. 
      
  2. Can you expand this section to indicate that the ambition is that over time more logic migrates out of JUnitRun and PytestRun and into this mixin?

  3. This could probably be further extended into a filter on target types?

    1. I've changed it to _test_target_filter which returns a filter fn, which is then used in the mixin class to filter the targets. Is that what you meant?

  4. src/python/pants/backend/core/tasks/test_task_mixin.py (Diff revision 4)
     
     
     
     
     
     
     

    Should this be a singular form, and have the mixin execute the loop?

    1. Yes, that makes sense. Done.

  5. AFAIK, this is now unnecessary because going to the superclasses is the default behaviour.

  6. ditto

  7. 
      
SA
ST
  1. 
      
  2. src/python/pants/backend/core/tasks/test_task_mixin.py (Diff revision 5)
     
     
     
     
     
     
     

    It's a bit weird for this to be a property that returns a function, rather than just being a method.

  3. 
      
SA
SA
BE
  1. 
      
  2. Short docstrings should be all one line:

    """Run the task."""

    And all comments and docstrings should consist of complete sentences that start with a capital letter and end with a period. Please fix throughout.

  3. What does "valid" mean here? This isn't a concept we use anywhere else.

    1. The only "validation" check that currently exists is in JUnitRunner:

      if not target.payload.sources.source_paths:
        msg = 'JavaTests target must include a non-empty set of sources.'
        raise TargetDefinitionException(target, msg)
      

      Python doesn't have any checks other than the filter to ensure that the target is a PythonTests, so _validate_target is empty.

      I meant it pretty generically-- a custom fn that can be defined in the language-specific test runner which ensures that the target meets the requirements set for that language-specific runner.

      Perhaps a word other than "validate" is appropriate?

  4. 
      
SA
BE
  1. 
      
  2. Remove superfluous empty line.

    ```
    """Short docstrings look like this."""

    """Long docstrings look like this.

    With extra text here.
    """"

  3. I'm just not sure why you need this, after _get_relevant_targets()? What is an example of a target that's "relevant" but not "valid"?

    1. The get_relevant targets just filters the targets to ensure that they are of the right type. Pytest_run only runs python test targets, not java test targets. The next step is to ensure that the test target isn't configured badly. This step doesn't just filter out bad test targets, it throws if it sees a bad test target. The Python task has no such checks, but the JUnit task checks to ensure that sources= is not empty.

      I've added a comment to make this more clear.

  4. 
      
SA
ST
  1. Ship It!
  2. 
      
BE
  1. Ship It!
  2. 
      
SA
SA
SA
SA
SA
JT
  1. 
      
  2. it would seem more clear to me to call all targets "targets" and test targets only "test_targets"...maybe just me. Looking at 'all_targets' and 'targets' I don't have context to see the difference, but between 'targets' and 'test_targets' i feel like i have a better idea.

  3. i guess i feel like passing around both these sets seems clunky...can you create a util or something that does the filtering as needed? or add them to the task_exports and I'll move it out when i move instrumentation out?

    1. I like the task exports, particularly because you note that it will work with your plans to move instrumentation out.

  4. 
      
SA
JT
  1. Ship It!
  2. 
      
SA
Review request changed

Status: Closed (submitted)

Change Summary:

Merged as b5da65d5262ac6036377928d67208c995f2b80bc

Loading...