Simplify failed test reporting.

Review Request #4240 — Created Sept. 15, 2016 and submitted

jsirois
pants
jsirois/issues/3837/simplify_junit_parse_interface
3837, 3874, 3879
5035894...
pants-reviews
benjyw, cheister, kwlzn
Simplify failed test discovery by scanning a flat dir for junit xml
report files. This structure re-working will enable re-use of test
failure reporting code in large part by the pytest task in the python
backend.

Along the way tighten some ad-hoc parsing and data structures by
introducing new _Test and _TestRegistry types. Also fixup 100 col
violations and some non-idomatic quotes and string formats.

 src/python/pants/backend/jvm/tasks/BUILD                                  |   5 +-
 src/python/pants/backend/jvm/tasks/coverage/base.py                       |  19 ++-
 src/python/pants/backend/jvm/tasks/coverage/cobertura.py                  |   8 +-
 src/python/pants/backend/jvm/tasks/junit_run.py                           | 424 +++++++++++++++++++++++++++++---------------------
 src/python/pants/task/testrunner_task_mixin.py                            |  46 +++---
 src/python/pants/util/BUILD                                               |   5 -
 src/python/pants/util/iterators.py                                        |  31 ----
 tests/python/pants_test/backend/jvm/tasks/coverage/test_base.py           |  31 ++--
 tests/python/pants_test/backend/jvm/tasks/test_junit_run_integration.py   |  48 ++++--
 tests/python/pants_test/backend/jvm/tasks/test_junit_tests_integration.py | 110 +++++++------
 tests/python/pants_test/util/BUILD                                        |   8 -
 tests/python/pants_test/util/test_iterators.py                            |  25 ---
 12 files changed, 389 insertions(+), 371 deletions(-)

Locally green:

./pants test-changed
./pants test $(./pants list tests/python/pants_test/backend/jvm/tasks/: 2>&1 | grep junit)

CI went green here:
https://travis-ci.org/pantsbuild/pants/builds/160892990

  • 0
  • 0
  • 3
  • 0
  • 3
Description From Last Updated
JS
  1. 
      
  2. NB: I'm doing this dance because there has been pants-devel/#pantsbuild correspondence indicating folks rely on the location of the private .pants.d/test/junit top-level dir for finding junit xml :/.
    1. We do rely on that for now, but could just as easily rely on a specific directory for junit XML output being listed. I don't think the global directory where all previous junit run output get stacked together is all that wonderful. Our process is

      1) rm -rf .pants.d/test/junit/*
      2) run our pants commands
      3) scrape this directory to make reports

    2. Thanks for the background / confirmation. I'll add a note to the code that explains the current need for the dance and file an issue to add either a flag for an output dir or else a stable homing under dist/ where the final products for the most recent run can be copied.
  3. 
      
JS
JS
BE
  1. Thanks for this! junit_run has been in dire need of some work for a long time. My comments are mostly around matching up to xunit terminology more precisely (or explaining to me my misunderstandings of the terminology...).

  2. Since you're cleaning this up anyway, is there a less janky way to indicate whether the returned value is a sourcefile or a classname?

    1. See what you think.
  3. With my limited understanding of JUnit I'm not sure what this means. How does a test enclose another test? AFAIK in xunit terminology a test is a method annotated with @Test, so what does it mean for one of these to enclose another?

    1. Yeah - this gets awkward.  The junit xml output from py.test for python code for example can have the encosling module (file) as the "classname" and the top-level test functions as the tests ("name"s).
      Perhaps the easiest thing to do is stick with junit/java terminology since the target is a junit (not xunit) specific report format.  See what you think of the re-wordings.
  4. I notice you're been careful not to shadow local variables in the outer scope, thanks for that. My IDE doesn't like it when that happens.

    1. You're welcome and same here. I've come around to my IDEs way of thinking.
  5. While you're here can you fix up this docstring to use more precise language? AFAIK a "test case" is a container ("case" as in "suitcase") for multiple tests, and what we're returning here is a list of tests, not test cases.

  6. Change this (malformed anyway...) param doc to reflect the new params. Or just get rid of it.

  7. 
      
JS
JS
BE
  1. Changes LGTM, thanks!

  2. Yep, this works for me.

  3. typo: methodnme

  4. 
      
JS
BE
  1. Ship It!
  2. 
      
JS
JS
Review request changed

Status: Closed (submitted)

Change Summary:

Now on master:

git log -1 origin/master
commit 67925858cd5cdb1ec770c4e9aeeff288c80456fc
Author: John Sirois <john.sirois@gmail.com>
Date:   Sun Sep 18 16:15:21 2016 -0600

    Simplify failed test reporting.
    
    Simplify failed test discovery by scanning a flat dir for junit xml
    report files. This structure re-working will enable re-use of test
    failure reporting code in large part by the pytest task in the python
    backend.
    
    Along the way tighten some ad-hoc parsing and data structures by
    introducing new _Test and _TestRegistry types. Also fixup 100 col
    violations and some non-idomatic quotes and string formats.
    
    Testing Done:
    Locally green:
    ```
    ./pants test-changed
    ./pants test $(./pants list tests/python/pants_test/backend/jvm/tasks/: 2>&1 | grep junit)
    ```
    
    CI went green here:
      https://travis-ci.org/pantsbuild/pants/builds/160892990
    
    Bugs closed: 3837, 3874, 3879
    
    Reviewed at https://rbcommons.com/s/twitter/r/4240/
Loading...