make sure junit_run task only runs on targets that are junit compaible

Review Request #924 — Created Aug. 20, 2014 and submitted

ity, ji, jsirois
make sure junit_run task only runs on targets that are junit compaible
Two questions:

1. any suggestions on how to write a unit test for the code change? I didn't find unittest for this class.

2. how do I test the changes? Should I just create some dummy BUILD files and only put resources targets into it to make sure they're all filtered out and junit_run does an no-opt?

Thanks for bearing with me.
  1. OK - I dug more looking for an easy integration test in pantsbuild examples/ or testprojects/ and I actually came to understand the problem better.
    This works!:
    jsirois@gill ~/dev/3rdparty/pants (master) $ find . -name BUILD | xargs grep "resources("
    jsirois@gill ~/dev/3rdparty/pants (master) $ PANTS_DEV=1 ./pants goal test examples/src/resources/com/pants/example/names
    11:41:29 00:00   [resources]
    11:41:29 00:00     [prepare]
    11:41:29 00:00   [test]
    11:41:29 00:00     [pytest]
    11:41:29 00:00     [junit]
    11:41:29 00:00     [specs]
    But the bug actually has a narrowly different case:
    18:14:05 18:14:05 00:01   [check-exclusives]
    18:14:05 18:14:05 00:01     [check-exclusives]
    18:14:05 18:14:05 00:01   [resolve]
    18:14:05 18:14:05 00:01     [ivy]
    18:14:05 18:14:05 00:01   [compile]
    18:14:27 18:14:27 00:23   [resources]
    18:14:27 18:14:27 00:23     [prepare]
    18:14:27                    Invalidated 2 targets containing 2 payload files.
    18:14:27 18:14:27 00:23     [buildprops]
    18:14:27                    Injecting generated SyntheticAddress(/data/jenkins/workspace/birdcage_3/.pants.d/resources/buildprops/emoji.images.images:build-props) for binary BuildFileAddress(/data/jenkins/workspace/birdcage_3/emoji/images/BUILD, images)
    18:14:27 18:14:27 00:23   [test]
    18:14:27 18:14:27 00:23     [junit]
    18:14:27                Waiting for background workers to finish.
    18:14:28                FAILURE
    18:14:28   File "/data/jenkins/workspace/birdcage_3/pants-support/jenkins/scripts/.pex/install/pantsbuild.pants-0.0.21-py2-none-any.whl.66b51cbabee8a87b74bbcb792397160afa122709/pantsbuild.pants-0.0.21-py2-none-any.whl/pants/backend/core/tasks/", line 165, in get_group_key_for_target
    18:14:28     return self.target_to_key[target]
    18:14:28 KeyError: Resources(SyntheticAddress(/data/jenkins/workspace/birdcage_3/.pants.d/resources/buildprops/emoji.images.images:build-props))
    Note well the failing twitter case fails to find a _synthetic_ resources target: "Resources(SyntheticAddress(/data..." - ie: an injected target.  Also not the target is injected after check-exclusives and resolve run - so the classpath is unaware of this added-later-target!  Thats the fundamental issue.
    So to test the actual failure case you'd:
    1.) hand-create and exclusives mapping
    2.) create a synthetic resources target
    3.) run junit_test against that single target in 2
    Expected: noop, Actual before fix - blowup.
    An example of this type of setup in a unit test is here (I just added this yesterday or so): 
    1. This also suggests an internal ~bug.  The problem of JunitRUn selected too broad a swath of target to operate on would have never come up if the Twitter-internal [buildprops] task were installed in the [gen] goal (gen runs before resolve for a good reason).  It is - after-all a code-gen task.
    2. I have no idea what a synthetic target is, still need your helps/handhold on how to do the proper testing.
      I tried your first example without my fix: PANTS_DEV=1 ./pants goal test examples/src/resources/com/pants/example/names
      It worked because 
      class _JUnitRunner(object):
        def execute(self, targets):
          tests = list(self._get_tests_to_run() if self._tests_to_run
                       else self._calculate_tests_from_targets(targets))     <=== this returns empty.
          if tests:
            bootstrapped_cp = self._task_exports.tool_classpath(self._junit_bootstrap_key)
            junit_classpath = self._task_exports.classpath(
              confs=self._context.config.getlist('junit-run', 'confs', default=['default']),
      thus it never even reached 'targets[0]' statement. note self._calculate_tests_from_targets does rely on checking if the targets are instance of JavaTests. So seems to me my fix should be a lot tighter, only allow targets that are or derive from JavaTests.
    3. A synthetic target is this:
      Thats a new target created programatically and added to the build graph at runtime a long time after the targets defined on disk in BUILD files have been read and parsed when pants starts up.
      You can look at the Twitter-internal [buildprops] task code and see a similar thing.
      And agreed on JavaTests being the only check needed.
  2. Definitely not.  And:
    $ java -help 2>&1 | grep agent
                      load native agent library <libname>, e.g. -agentlib:hprof
                      see also, -agentlib:jdwp=help and -agentlib:hprof=help
                      load native agent library by full pathname
                      load Java programming language agent, see java.lang.instrument
  3. No - just Targets that can own @Test annoted sources or subclasses of org.junit.TestCase.  I led you astray thinking out loud yesterday and suggested is_jvm, but - in fact, just the JunitTests targets are appropriate for JunitRun.  Their dependencies may be is_jvm, but that's upstream tasks concern (resolve, compile).
    Sorry for the mis-direction.
    1. are you saying i should only check:
           return (target.is_java or target.is_test)
      ? Because junit_test (JavaTests) has only 'java' and 'test' labels.
      on a second thought, shouldn't I just do this?
           return isinstance(target, JavaTests)
    2. Either works - both should only select JavaTests targets or subclasses.  As I said offline, labels - which were intended as a replacement for type-tests, are in a state of flux.  I agree a type test is most clear here and this was mostly agreed on as the right direction at the summit [1].
  4. exactly the opposite!
  1. LGTM mod the missing unit test.
  1. this will be followed with the unit test?
    1. Yes, I thought that was what we discussed in the standup. Let the fix rollin so we can catch next Monday's integration, and will write unittest (still need to follow up with you and John to figure out how to write it) next week.
    2. sounds good, thanks!
  1. Finished all unittests run
    PANTS_DEV=1 ./pants goal test tests::
                          478 passed, 5 skipped, 5 xfailed in 418.00 seconds 
    14:13:05 08:13     [junit]
    14:13:05 08:13       [run]
                         Time: 0.032
                         OK (1 test)
    14:13:05 08:13     [specs]
                   Waiting for background workers to finish.
    Committer, please patch it in. Thanks a lot!
    1. ping, any committer to help? thx!
    2. Hi Jen, I would be happy to do it, but what I would like to see is a passing travis-ci run.  I didn't see that this issue has it.
      I know you asked about this before.  I'm not sure if you got a good response.  What I do is from my local workspace:
        zundel/branch-name $ git push -f origin 
      then go to github and create a PR from this branch in the pantsbuild/pants repo.
      Travis-CI should pick it up.  Make sure that new PR issue number is linked to the RBCommons code review in the "Bug" field.
      Then I can be pretty sure that your code isn't going to break the CI build after submitting it.
    3. S/Jen/Jin/ sorry.
    4. Because this branch isn't on a forked repo, instead directly branched off from master on the main github pants repo, I don't have permission to push to origin:
      [jinfeng@tw-mbp13retina-jfeng-257 ~/workspace/github-pants (pants_666)]$ git push -f origin 
      warning: push.default is unset; its implicit value is changing in
      Git 2.0 from 'matching' to 'simple'. To squelch this message
      and maintain the current behavior after the default changes, use:
        git config --global push.default matching
      To squelch this message and adopt the new behavior now, use:
        git config --global push.default simple
      See 'git help config' and search for 'push.default' for further information.
      (the 'simple' mode was introduced in Git 1.7.11. Use the similar mode
      'current' instead of 'simple' if you sometimes use older versions of Git)
      Username for '': jinfeng
      Password for '': 
      error: The requested URL returned error: 403 Forbidden while accessing
      fatal: HTTP request failed
    5. Here is how you fix that to end up with the configuration describe in

      First, do

      git remote show

      and you can see the remote repos you have setup. You might want to save this output in case you want to change things later.

      If you haven't already forked a repo in git hub, do it by going to the pantsbuild/pants project and hitting the 'fork' button on the upper right.

      You don't need to clone it, just do this in your existing repo.

      git remote add upstream
      git remote remove origin # sounds scary, but it is just removing the URL setting, you can easily add it back.
      gir remote add # or whatever URL your forked repo shows you as the cloned URL

      Now, you should be able to do 'git push -f origin' from your branch.

      I am not sure if this will show up in travis-ci or not, but at least your github setup will be correct and you can share branches from this workspace.

    6. It's all right, I thought there might be some ways to trigger a travis run for a branch off master. Instead, I've just moved my patch to my own forked repo and triggered a CI:

    7. sigh....I remember John told me that due to some permission reason only a comitter can re-run a failed travis CI run (I can't see the re-run button on the page). My run ( just failed for non-fix related reasons:

      Exception message: Package EggPackage(u'') is not translateable.
      Exception message: Cannot satisfy requirements: [PythonRequirement(pytest==2.5.2)]
      Failed to bootstrap pants.
      The command "java -version; ./build-support/bin/ -d
      " exited with 1.

      Wondering if anyone can help to re-trigger the failed travis run? Otherwise, then I would introduce some trivial like comment change so I can push to origin, create another pull request and trigger a new travis run.

    8. I lied. the job is restarted, my restart right is still there :)

    9. Ugh, I hate that error.

      git commit --allow-empty
      git push -f 

      should kick it off again.

  1. it's passed.

    pants_666 - make sure _JUnitRun filters out non-JavaTests targets

    Jin Feng authored and committed Commit ed28206 Compare ed282066505a
    Build Matrix
    Job Duration Finished Python
    10.1 20 min 5 sec about 6 hours ago 2.6
    10.2 20 min 17 sec about 5 hours ago 2.7

    1. oh and I ran this yest too -

  1. Commited at c5d922c Please close the review and mark as submitted.

    1. done. and thx, Eric!

Review request changed

Status: Closed (submitted)