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 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

  • 0
  • 0
  • 5
  • 0
  • 5
Description From Last Updated
  1. Thanks for the review! One blocker issue, and then I think we can ship this.

  2. src/java/org/pantsbuild/tools/junit/impl/ (Diff revision 1)

    It seems like this introduces a dependency on scalatest to the runner, because if the org.scalatest.Suite class 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.

  3. xx: As mentioned above: this won't fly. Should skip the scalatest codepath cleanly if scalatest is not present.

  2. src/java/org/pantsbuild/tools/junit/impl/ (Diff revision 1)

    Good catch. Fixing up.

    I changed only the isScalaTestTest method since the getJUnitRunner is only called if the isScalaTestTest returns true

  3. We don't need it since I moved out the scala files to another location

  1. Thanks!

    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=())]
    1. A quick update: this weekend is crazy with Scala by the Bay and another event, and I want to give this a real-world test in our repo, so I might hold off on merging until Monday unless a second reviewer comes along to confirm that this is ready.

  1. 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.

  2. 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.

  3. this is so JUnit 3!

  1. Did some cursory testing internally, and this seems sane. There is one issue to clear up which seems to have killed CI: please update when CI is green and I'll land this immediately.

  2. This line fails in checkstyle:

    1. Your most recent run succeeded. Will merge when you post it here.

Review request changed

Status: Closed (submitted)

Change Summary:

Merged as d828787a1c67afc581b723a74ffe83c1ba315cf7