When a python test fails outside of a function, the resultslog message is just [EF] file.py, without the double-colons

Review Request #3397 — Created Jan. 30, 2016 and submitted

sameerbrenn
pants
2868
pants-reviews
benjyw, stuhood

We have some tests in our repo that don't have test classes, they don't even have test functions. They just have raw python that runs at the top level. When they fail pants doesn't parse the resultslog properly because the resultslog doesn't list the function using the double-colon notation.

./pants test tests/python/pants_test/backend/python/tasks:pytest_run
build-support/bin/unit-test.sh

Travis: https://travis-ci.org/pantsbuild/pants/builds/106268102

  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
BE
  1. 
      
  2. Just noticed that this RE has (.+?) when I think (.*) is a more succinct way of saying the same thing.

    1. They're different, iirc. .* is greedy and is capable of matching 0 characters. .+? is not-greedy, and will only match 1 or more characters.

    2. Ah indeed! .*? is not ? applied to .*, but *? (non-greedy *) applied to ..

      But I guess the effect is still the same, assuming there's only one double-colon in the line, and that there's at least one non-space character preceding it.

  3. Does this combined RE work?

    r'^[EF] +(?P<file>.+)(::.+)?$'
    

    Basically, "(E or F) followed by one or more spaces, followed by a filename, optionally followed by (a double-colon and some stuff we don't care about)".

    Then you can access the filename with m.group('file'). Or if you want a shorter re but with less explanatory indexes:

    r'^[EF] +(.+)(::.+)?$'
    

    and the file is at m.group(1)

    [because group(0) is the entire match].

    Either of these is preferable to groups()[0], as you're fetching just what you care about.

    1. Awesome thanks. I did have to change it to be non-greedy: r'^[EF] +(?P<file>.+?)(::.+)?$'

  4. 
      
SA
SA
SA
BE
  1. LGTM, but as I mentioned in the other RB, you should still make the owners of those pathological tests fix them :)

    1. Thanks! Yeah, we're working on that, it's a pretty long process. One issue is that we don't know who owns some of this code.

  2. 
      
ST
  1. 
      
  2. 
      
SA
Review request changed

Status: Closed (submitted)

Change Summary:

Merged as be3ded19378e4699de5135af0516495463a728d2

Loading...