Fixing cobertura coverage so that it actually works.
Review Request #1704 - Created Feb. 3, 2015 and submitted
|benjyw, ity, nhoward_tw|
Changes in this commit:
- The three Cobertura phases are now invoked with different classpaths. This fixes problems where
jars in the full cobertura dependencies were conflicting all the time with jars in the run phase,
and publishing a shaded jar was too much of a hack for my tastes. This also allows for a different
reporting tool to be substituted for the reporting phase of cobertura.
- Three separate tools for the three stages of cobertura, as they need different classpaths and
different dependencies, to accomodate the previous point.
- Various fixes in how cobertura was actually invoked, in particular, the --auxclasspath option in
the instrumentation phase was crucial in getting reliable results.
- Miscellaneous code cleanups (see diffs)
travis-ci passed for https://github.com/pantsbuild/pants/pull/993: https://travis-ci.org/pantsbuild/pants/builds/48599375
TODO(x)is a common way to search for real todos. Please stick to TODO, XXX, or NOTE depending on the context.
.formatis always preferred now.
What's with the
This shouldn't be wrapped in
file, unless the interface of
temporary_fileis really weird.
instrumented_classes_fileshould be a file object that is writable.
Also, this would need to be
'wb'and explicitly encoded as
utf-8: this would fail on classes that contain non-ascii characters. And they, unfortunately, do exist.
well, it was a reply to benjy's todo.
preferred for everything, or just for logging (where we don't want to worry about a seldom-exercised line suddenly failing because of an incorrect format conversion?).
it's there to see if you were paying attention :)
The original (incorrect) reason for using temporary_file_path() was that I needed the filename after the file was closed (I incorrectly assumed that the file would disappear when closed, as opposed to when the context manager would exit.
however, if you want it opened 'rb', (and to open it as utf8 it would have to go through codecs.open, as the file ctor does not take an encoding), it has to go this route again; temporary_file does not take any additional keyword arguments, nor does it have provisions for opening with a codec.
Moreover, I tried creating a java class with a greek name, put it in an identically-named file, and kept getting errors like this:
CacheValidationError: Problem validating target sandbox.users.ji.cover.src.main.java.com.twitter.j5.fufutos in sandbox/users/ji/cover/src/main/java/com/twitter/j5: 'ascii' codec can't encode characters in position 54-63: ordinal not in range(128)
it's not that the BUILD file is not somehow saved as utf8, I worked around that by using an ascii character as the first name of the file and using glob() in the sources parameter, so there is no utf8 processing when the BUILD file is read.
Is this a deeper bug? DO you have an example with non-ascii filenames and classnames? the cucumber example in the tests doesn't count!
so this should be something like: with temporary_file_path() as icf: codecs.open(icf, mode='w', encoding='utf-8').write(u'\n'.join(classes) + u'\n') is @classes a list of unicode strings? (wb is unnecessary with codecs, b is implied)
One last nit: could you change the title of this CR to reflect the overall nature of the commit and leave the historical stuff in the details section? I'm not sure if you can change RBC titles once they're out, but double check the commit title generated by
./rbt patchbefore upstreaming please.
This should be good to go.