Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support omitting the test sources themselves from coverage. #8208

Merged
merged 3 commits into from Sep 5, 2019

Conversation

@benjyw
Copy link
Contributor

commented Aug 25, 2019

If tests are in the same packages as the libs they test then today we
will trace and report them, which is almost certainly not what the user wants.

This change optionally omits test files from tracing.

This change also:

  • Excludes synthetic __init__.py files created by pex in the chroot
    from reporting (there is no good way to exclude them from tracing, but
    since they're empty, that doesn't really matter).

  • Drops the console output, which was just long noise cluttering up output,
    especially in CI.

  • Allows the user to choose which of the xml and html reports they want.

These last two points are performance gains: Generating coverage reports
takes non-trivial time, especially on a large set of traced files, and we were
previously always generating three reports. Now we generate two by default,
and the user can choose to generate just one (or none, but that seems pretty
pointless).

This code also updates our coverage plugin to reflect the fact that plugins
cannot on their own prevent .py files from being traced or reported. If a plugin
has no interest in a file, it will still be traced and reported using default mechanisms.

Finally, we replace a reference to the deprecated name SafeConfigParser
with the current name ConfigParser.

Support omitting test and synthetic files from coverage.
Also remove the coverage console output, and allow the user to
choose which of the xml and html reports they want. This
improves performance - generating reports takes non-trivial time,
especially on a large set of traced files, and we currently
always generate three.

@benjyw benjyw requested review from jsirois, stuhood, Eric-Arellano and asherf Aug 25, 2019

@Eric-Arellano
Copy link
Contributor

left a comment

Yay! Thank you!

@@ -112,6 +112,13 @@ def register_options(cls, register):
help='Emit coverage information for specified packages or directories (absolute or '
'relative to the build root). The special value "auto" indicates that Pants '
'should attempt to deduce which packages to emit coverage for.')
# TODO: The default is False for backwards compatiblity. We almost certainly want to change
# that after a deprecation cycle.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Aug 25, 2019

Contributor

I recommend that we start the deprecation now in this PR. How would we do this? If the default is used but not explicitly specified, log a DeprecationWarning?

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Aug 25, 2019

Contributor

Hm, after rereading this PR, in an ideal world would this even be an option? Can you imagine use cases where someone would want this to be False?

I know we have to consider backwards compatibility, but want to make sure in two release cycles that this option still would make sense to have.

This comment has been minimized.

Copy link
@benjyw

benjyw Aug 25, 2019

Author Contributor

We could just treat this as a bug and change it without deprecation, if people are on board with that.

This comment has been minimized.

Copy link
@benjyw

benjyw Aug 25, 2019

Author Contributor

We could just treat this as a bug and change it without deprecation, if people are on board with that.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Aug 25, 2019

Contributor

I think this is a bug - we didn’t seem to intentionally implement this behavior and I see no case where someone would want it. I support changing without deprecation and without adding an option.

This comment has been minimized.

Copy link
@benjyw

benjyw Sep 3, 2019

Author Contributor

ping @jsirois @stuhood @asherf (this comment in particular, and the PR in general)

This comment has been minimized.

Copy link
@asherf

asherf Sep 3, 2019

Contributor

I don't have enough context on this so I don't have an informed option if this is a bug or a deprecation.

This comment has been minimized.

Copy link
@stuhood

stuhood Sep 4, 2019

Member

Yep, fine with that.

Will defer to @Eric-Arellano for the rest.

@@ -551,13 +551,16 @@ def load_coverage_data_for(self, context, covered_path, expect_coverage=True):
covered_src_root_relpath = os.path.relpath(covered_path, src_root_abspath)
chroot_path = os.path.join(src_chroot_path, covered_src_root_relpath)

cp = configparser.SafeConfigParser()
cp = configparser.ConfigParser()

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Aug 25, 2019

Contributor

Yay! Thanks.

self.assertEqual([1, 3, 5, 6, 7], not_run_statements)
# TODO: Switch this test to read coverage data from an XML report instead of
# directly from the analysis. That way we see what the users sees, instead of the
# slightly confusing raw analysis.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Aug 25, 2019

Contributor

How likely are we to ever do this? This current test seems fine and I suspect it's unlikely we'll spend more time to make this improvement, so I would advocate removing the TODO.

@jsirois
jsirois approved these changes Sep 3, 2019
Copy link
Member

left a comment

I'm definitely OK with treating prior behavior as a bug and flipping the default or killing the option outright. Someone may reasonably want coverage numbers over test code though to prove tests are run; ie: detect bad CI setups or too high a % of conditional test skips firing. As such, just flipping the default seems right to me.

@benjyw

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

I'm definitely OK with treating prior behavior as a bug and flipping the default or killing the option outright. Someone may reasonably want coverage numbers over test code though to prove tests are run; ie: detect bad CI setups or too high a % of conditional test skips firing. As such, just flipping the default seems right to me.

Done - flipped the sense of the flag, so that people can do so if they really want to.

benjyw added 2 commits Sep 3, 2019

@benjyw benjyw merged commit ce56ed2 into pantsbuild:master Sep 5, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@benjyw benjyw deleted the benjyw:coverage_fixes branch Sep 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.