Inject an automatic dep on junit for all junit_tests targets.

Review Request #4228 — Created Sept. 13, 2016 and submitted

gmalmquist, kwlzn, mateor

It's silly to require an explicit dep.

This required creating a new "JUnit" subsystem. While there,
I moved the junit runner tool to that subsystem, so the runner
and the library are in the same place.

It's still clunkier than it needs to be to inject deps onto targets,
but that's a problem for another time.

This change removes the explicit junit dep from one of our examples,
to prove that this all works. A subsequent change will remove any
other unnecessary explicit deps on junit.

CI passes:

  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
  1. This may inadverdent implication on intellij users, as we use the exported jars including the junit jar for intellij to distinguish sources vs tests as well as to luanch junit test. I think the bottom line is intellij needs to know e.g. examples/tests/java/org/pantsbuild/example/hello/greet depends on junit.

    1. All targets of that type will still depend in junit, you just won't have to specify that dependency explicitly.

    2. oh sounds good. thanks

  1. What would be the implication in terms of unused deps? Does this change check for junit as transitive dependency?

    Say we have junit test target A and B (-> denotes depends on)
    B -> A -> 3rdparty:junit, would 3rdparty:junit be a direct dep of B as well even if B may not require so?

    1. No, why would it be a direct dep of B?

    2. In the test, 3rdparty:junit(removed in BUILD file) will be injected into examples/tests/java/org/pantsbuild/example/hello/greet, so if some junit test target X depends on examples/tests/java/org/pantsbuild/example/hello/greet, will 3rdparty:junit be injected into X as a direct dep?

      If not, how do you decide whether to inject 3rdparty:junit into a junit test target?

    3. All targets of type junit_tests() will have that dep injected. We don't typically have junit_tests() targets depend on other junit_tests() targets, so I'm not sure I understand your example.

    4. ah ok. if junit_tests() cannot depend on another junit_tests(), that should be no problem. thanks!

    5. They can, they just typically don't. But even if they do, why is this a problem?

    6. It may be an issue for strict dep policy, because 3rdparty:junit may not be directly used by a junit_tests(). e.g. B extends A, A extends TestCase, so B is not directly using TestCase

    7. So, we actually have this case in our repo.

      mateo:1 mateo$ git grep -Hl 'junit_tests(' | xargs grep -L 3rdparty:junit | wc -l

      There is a BaseClass that we use to force specific classloader, since there we witnessed different classloaders being used in specs2 tests depending on the pants context.

      But it isn't clear to me why this is an issue - junit itself is on the classpath either way, no?

  1. I'd like to see a couple tests.
    1. Specifying the behavior if junit is an explicit dependency. From poking around, it won't cause an error, but duplicate target dependencies are errors if they are defined in BUILD files. It would be good to be sure that that behavior stays consistent.
    2. Less important, but a test that explicitly covers this new behavior would be good.

    1. Added a test that addresses those points.

  1. Thanks Benjy! Nice cleanup.

  2. Although this is used in a test, probably best to still mark private in order to send the right signal to consumers.

    1. I'd rather not, I don't generally love having outside code reference private things, even tests.  That's what :API: Public is for. I'd rather the reader of this receive the signal that this is in fact used outside this file. 
      In fact, I just noticed that references JUnit._RUNNER_MAIN, so I made that public too. 
  3. Due to the super-awkward nature of traversable(_dependency)?_specs, this will run once per test target. Might be worthwhile to memoize the parsing of the junit_addr.

    1. Done. I think we can memoize this entire method, not just the parsing, no?

  4. This task should declare a subsystem dependency on the JUnit subsystem.

    1. Good catch. Fixed.

  1. Thanks for the reviews, Nick and Stu. Pushed change to address your comments. PTAL.

  2. Do you think it would be good to indicate //:junit_library is injected?

    [tw-mbp-yic pants (fid)]$ ./pants dependencies examples/tests/java/org/pantsbuild/example/hello/greet/::

    as ./pants list //:junit_library would give an error.

  1. So - this could silently upgrade the junit version for folks, right? I don't see anything surfaced to the user, so they could start using a new junit without noticing, unless I misunderstand.

    This will remove the need for 3rdparty:junit for junit_tests but not for other targets. We have some legacy helper scala_libraries which will still need the 3rdparty:junit as a dependency. I worry about introducing special cases where users are expected to skip dependencies depending on the target type. Maybe this exists in some other tasks that bootstrap their tools and I have internalized it :)

    I can imagine a Buildgen config that would handle these cases for Foursquare - we would SKIP junit globally and manually force the dep for any non-junit_tests.

    1. Hmm, yes, that is an issue. The obvious fix is to either set ://junit_library to point to your preferred junit version, or to set the --junit_library option on the JUnit subsystem to point to whatever you were using (presumably 3rdparty:junit or something like that).

      This should definitely go in the release notes. I'll see if I can find a way to be smarter about this in the code.

      I see this as similar to how we inject the scala runtime onto all scala_library targets. Nothing stops you from depending on it explicitly, and version collisions can happen if you do so carelessly, but since ~100% of targets of this type will require this dep - indeed the dep is implied by the name of the target type - it makes sense to not require it to be explicit.

    2. Hmm, it doesn't appear to be possible to detect a version mismatch, because we inject when we only have specs, we don't yet know what they resolve to. Maybe we could resolve them later or something, but that's a bigger change. I don't think we detect "single target depends directly on two different versions of the same jar" today?

    3. I know that we do not - unless it happens within ivy.resolve which I don't know too well. The classpath handling will happily add them both and just the first one on the classpath will take priority. We could consider putting all injected deps at the end of the classpath - that would give user BUILD file priority.

      Or just dedupe in ClasspathUtils or similar.

    4. We would have to know at that point that the deps are injected, which I don't think is a thing we can currently do.

      We do yield the injected deps after the real ones, but I'm not sure that affects the classpath order.

    5. I created an issue to track this:

    6. So where do we stand on this change from your POV? How concerned are you about a possible conflict during upgrade? It is easy to resolve that conflict by explicitly setting

      junit_library: 3rdparty:junit

      But I'd be surprised if anyone was using a JUnit other than 4.12, no? It was released two years ago, and 4.11 (which should still be API compatible) was released two years before that.

    7. I think that this is probably fine - the problem case is pretty remote and would hopefully be obvious. #ShipIt

  2. I have wanted finer control over the runner version, it would be nice to have these be configurable.

    I could be satisfied with a TODO - using a subsystem is a step closer to that.

    1. It is configurable. That's just the default value for the '--junit' option. You can set ://junit to whatever you want. I would have liked this to be called --junit-runner, but I think I went with --junit for backwards compatibility. Once this is all in we can do a deprecation cycle to rename it.

  1. Ship It!
  1. Submitted. Thanks for the thorough reviews!

Review request changed

Status: Closed (submitted)

Change Summary: