Thanks for the review! One blocker issue, and then I think we can ship this.
It seems like this introduces a dependency on scalatest to the runner, because if the
org.scalatest.Suiteclass isn't present, this will raise a RuntimeException?
It seems like the dependency would otherwise not be necessary, so perhaps this should fail silently if that class does not exist? That way non-scalatest tests are not affected.
xx: As mentioned above: this won't fly. Should skip the scalatest codepath cleanly if scalatest is not present.
Patch to make scala tests work
Review Request #4361 — Created Nov. 7, 2016 and submitted
|cheister, jsirois, stuhood, zundel|
Change to make scalatest test work using the JUnitRunner builtin to scala test. Basically I check if the test extends org.scalatest.Suite, if it does I include it in the specs. Later on, we do the same thing when converting the test class to a junit Runner. We check to see if it is a Suite, and if so then fake like we included the @RunWith annotation.
A few notes:
1) I didn't support scala test "methods"
2) I used reflection instead of modifying junit.py and friends to make scalatest be a non-shaded jar (in fact we don't need to include it at all)
3) I made sure I used the test class's class loader to load Suite and JUnitRunner
4) I bumped the runner_jar version to 1.0.16, I hope this is correct.
I would love to see this as a 1.2.1 release.
NOTE: This is a recreate of #4344
Will merge this after a few hours to give other folks a chance to provide feedback.
One note about backporting this in
1.2.1: we can certainly do that, but note that it will not be necessary for it to land in a release before it is usable. It's possible to override any JVM tool version in pants by creating a target in a well-known/configurable location:
$ ./pants help-advanced junit | grep -A4 'junit-junit' --junit-junit=<target_option> (default: //:junit) Target address spec for overriding the classpath of the junit jvm tool which is, by default: [JarDependency(org=u'org.pantsbuild', base_name=u'junit- runner', rev=u'1.0.15', force=False, ext=None, url=None, apidocs=None, classifier=None, mutable=None, intransitive=False, excludes=())]
Looking this over, I don't think it will break anything - I can't say that I know what all the issues are with Scala that you are trying to solve.
There is a lot of functionality in the runner that you aren't testing, like the different forms of parallelism or specifying a single test method on the command line. There is a lot of infrastructure to perform those kinds of tests.
Keep in mind that this legacy path is always used for non-parallel tests currently, so this is the behavior you're going to get most of the time with --default-concurrency=serial turned on.
Revision 3 (+174 -43)