Deprecate the `java_tests` alias in favor of `junit_tests`.

Review Request #4322 — Created Oct. 18, 2016 and submitted

benjyw
pants
pants-reviews
gmalmquist, mateor, stuhood
Deprecate the `java_tests` alias in favor of `junit_tests`.

CI passed: https://travis-ci.org/pantsbuild/pants/builds/168784588

  1. Benjy, can you share the motivation for this? Even though the current runner is currently hard-coupled to Junit, you can imagine a future with an alternative JavaTest framework.

    And because it was the base class for so long, I have plugins that defaulted to the lowest base and will have to be updated. For instance, we have consumers of buildgen that will now have to either maintain a buildgen fork or stay on the same side of this commit as we are.

    1. one compromise could be deprecating the alias but leaving the base class

    2. Ah, part of the problem is that I was going to add a deprecated alias to the old name, but forgot. Added it now.

      The motivation is that

      A) these really are tightly bound to JUnit. If we supported an alternative test framework it would probably require different targets (or in the new engine world, different configs).

      B) These aren't just for Java tests. Scala tests work just as well.

      But with the alias I added, nothing should break, right?

    3. I am running out the door, so forgive me if this quick read is off.

      I think this is closer, but will still break. Specifically, I have checks for isinstance(JavaTests) that intend to catch JUnit that will no longer trigger, since it is now an instance of JUnitTest instead of the reverse.

      In the best case, it would start spitting out deprecations for every check right? Considering that this runs in Buildgen (which follows the SimpleCodegen style of subclasses) I fear this would dump deprecations at a high rate.

    4. and just to motivate why it is harder than just converting BUILD files in our repo: the outside consumers are orgs that use disparate versions of Pants, and consume buildgen as contrib plugin sdists from pypi. So we would have to re-release the sdists, and vendor them around this commit.

    5. I can rework the inheritance structure, and make the deprecation module-level, so it only spits out one deprecation warning if you import the module.

    6. OK, pushed another attempt to address the problem (if I understand it correctly).

      Note that I had to merge master to fix a conflict so travis would run, so you may want to look at just the most recent diff.

  2. 
      
  1. Thanks benjy, this will work for me. I had a chance here to take a closer look at the implementation and got a better sense of the "wny" as well. I appreciate the wiggle room on the deprecation. That seems like the right thing given its long life as a base class but I realize you could have balked.

    1. Submitted, thanks for the thoughtful comments.

  2. 
      
  1. Ship It!
  2. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

3a022b967a5c9023c4f746e595dccb7351033573

Loading...