Patch to make scala tests work

Review Request #4344 — Created Oct. 31, 2016 and discarded

dbrewster
pants
feature/scalatest
4013
pants-reviews
cheister, jsirois, stuhood, zundel
Patch to make scala tests work

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.

  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
DB
ST
  1. Thanks for the patch!

    Please add some tests for this functionality in tests/java/org/pantsbuild/tools/junit/impl... you should be able to run them before this code is published.

  2. This will need to be bumped after the java code lands, and a committer publishes it... 1.0.16 doesn't exist yet on maven central.

    1. Dave - you should click either 'Fixed' or 'Drop' when you have addressed an issue.  In this case it looks like it should be 'Fixed'.
  3. 
      
DB
JS
  1. Thanks Dave.
    1. Neither the 1-2 interdiff nor the full diff match with your comments (what you said was done does not show as done).
      The algorithm is to always upload the full diff against the branch point. I never merge origin/master and always rebase, so for me the algorithm is ./rbt post -u --parent=HEAD~N where N is the number of commits I've made on my branch.
      If you do merges then I'm not positive what algorithm works best but others in slack may work this way and have advice.

    2. I used ./rbt post -o -r 4344 --parent=master. The diff doesn't contain my latest changes so I'll try that command. I haven't merged so that will work for me.

      Done -- try that.

    3. If --parent=master is showing an empty diff, then there is something pretty wrong. That would imply that git diff master ${yourbranch} is empty.

      The diff as it stands is not starting from the correct base: as an example, the change in src/python/pants/backend/jvm/subsystems/junit.py should be a noop, because you've changed it back to what it was in master.

      Sorry for the trouble here... we're hoping to switch away from rbcommons and toward github PRs before the end of the year.

  2. This is appropriate as a // style comment below - maybe - though it just restates what the if (ScalaTestUtil.isScalaTestTest(clazz)) code says. It's definitely not appropriate as a doc comment here though.

  3. At the very least the class name really should change to indicate this is our generic custom builder that may do lots of custom things - the current name says the code you added does not belong here (retries has nothing to do with scala tests).
    1. Renamed to CustomAnnotationBuilder and updated comment. I don't think it is necessary to have two versions of the builder -- seems overkill.

      I also moved this and the ScalaTestUtilClass into the impl section as that seems more like the code that is already there wrt the other test util classes.

  4. withretry is an odd package for scala-anything. You could re-name the package if you rename AllDefaultPossibilitiesBuilderWithRetry as suggested above or maybe just create a new package for the scala test support code that contains the util and maybe the ScalaTestAnnotatedBuilder extracted top-level in that new package.

  5. Please make this class final and with a private constructor. The org.pantsbuild.tools.junit.impl.Util class is a great example of the style.

  6. 1 blank line between the javadoc body and the @params block.  Here and below.
  7. Please catch the explicit list - the code can use java 7 | style to do this. Here and below.

  8. Its not abstract, its just not a test - comment should change to reflect reality.
    1. I didn't do this change. All of the Build files say 2015 so I didn't think it appropriate to change one of them in this pull request

    2. New files should always have current copyright dates. If its a git mv, that's different. Please change to 2016.
  9. The sources can be removed - globbing *.scala is the default for a scala source root (tests/scala here).

    1. Done

    2. Actually I didn't do this. Removing caused a build failure so I'm guess that scala_library doesn't default sources to globs('*.scala')

    3. It does; however, it also excludes *Test.scala by default. Sorry for the misdirect on that one - forgot about the exclude feature.
      If the target were instead junit_tests, then the default glob would grab globs('*Test.java', '*Test.scala', '*Spec.scala').

  10. The scaladoc comment is not needed here nor in the file below.
  11. 
      
DB
DB
DB
Review request changed

Status: Discarded

Change Summary:

Redid with a new diff. Now at https://rbcommons.com/s/twitter/r/4361/

Loading...