Re: Review Request 4361: Patch to make scala tests work
Stu Hood
| This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4361/ |
Thanks for the review! One blocker issue, and then I think we can ship this.
| src/java/org/pantsbuild/tools/junit/impl/ScalaTestUtil.java (Diff revision 1) | |||
|---|---|---|---|
| 24 | /** |
||
| 25 | * Checks if the passed in test clazz has an ancestor that is the scala test suite |
||
| 26 | * object (looked up in the test classes class loader). |
||
| 27 | * @param clazz the test class |
||
| 28 | * |
||
| 29 | * @return true if the test class is a scalatest test, false if not. |
||
| 30 | */ |
||
| 31 | public static boolean isScalaTestTest(Class<?> clazz) { |
||
| 32 | try { |
||
| 33 | Class suiteClass = Class.forName("org.scalatest.Suite", true, clazz.getClassLoader()); |
||
| 34 | return suiteClass.isAssignableFrom(clazz); |
||
| 35 | } catch (ClassNotFoundException e) { |
||
| 36 | throw new RuntimeException(e); |
||
| 37 | } |
||
| 38 | } |
||
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.
| tests/java/org/pantsbuild/tools/junit/lib/BUILD (Diff revision 1) | |||
|---|---|---|---|
| 10 | '3rdparty:scalatest', |
||
xx: As mentioned above: this won't fly. Should skip the scalatest codepath cleanly if scalatest is not present.
- Stu Hood
On November 8th, 2016, 12:40 a.m. UTC, Dave Brewster wrote:
|
Review request for pants-reviews, Chris Heisterkamp, John Sirois, Stu Hood, and Eric Ayers.
By Dave Brewster.
Updated Nov. 8, 2016, 12:40 a.m.
Bugs:
4013
Repository:
pants
Description
Diffs
|
Stu Hood
| This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4361/ |
Ship it!
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=())]
- Stu Hood
On November 8th, 2016, 10:15 p.m. UTC, Dave Brewster wrote:
|
Review request for pants-reviews, Chris Heisterkamp, John Sirois, Stu Hood, and Eric Ayers.
By Dave Brewster.
|
Updated Nov. 8, 2016, 10:15 p.m. |
|
Bugs:
4013
Repository:
pants
Description
Diffs |
|
|
Stu Hood
| This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4361/ |
On November 8th, 2016, 11:03 p.m. UTC, Stu Hood wrote:
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=())]
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.
- Stu
Stu Hood
| This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4361/ |
Fix it, then Ship it!
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.
| src/java/org/pantsbuild/tools/junit/impl/ScalaTestUtil.java (Diff revision 2) | |||
|---|---|---|---|
| 20 | // debatable what to do here. isScalaTest should fail if scala test isn't available so this is probably ok. |
||
This line fails in checkstyle:
- Stu Hood
Stu Hood
| This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4361/ |
On November 15th, 2016, 6:57 a.m. UTC, Stu Hood wrote:
src/java/org/pantsbuild/tools/junit/impl/ScalaTestUtil.java (Diff revision 2) 20 // debatable what to do here. isScalaTest should fail if scala test isn't available so this is probably ok.This line fails in checkstyle:
Your most recent run succeeded. Will merge when you post it here.
- Stu