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: https://travis-ci.org/pantsbuild/pants/builds/159432189
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.
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?
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.
Thanks Benjy! Nice cleanup.
Although this is used in a test, probably best to still mark private in order to send the right signal to consumers.
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.
This task should declare a subsystem dependency on the JUnit subsystem.
Address code review comments.
Do you think it would be good to indicate
[tw-mbp-yic pants (fid)]$ ./pants dependencies examples/tests/java/org/pantsbuild/example/hello/greet/:: examples/tests/java/org/pantsbuild/example/hello/greet:greet examples/src/java/org/pantsbuild/example/hello/greet:greet examples/src/resources/org/pantsbuild/example/hello:hello //:junit_library junit:junit:4.12
./pants list //:junit_librarywould give an error.
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.