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

Refactor code coverage in preparation for adding a new coverage engine (jacoco) #4881

Merged
merged 4 commits into from Sep 29, 2017

Conversation

Projects
None yet
3 participants
@jtrobec
Copy link
Contributor

jtrobec commented Sep 21, 2017

Problem

There is tight coupling between the Cobertura coverage class and junit_run that makes it difficult to add a new coverage system. The use of an abstract BaseClass makes sharing common coverage parameters difficult.

Solution

Refactoring to eliminate abstract BaseCoverage class, in favor of a utility class and a class to manage coverage. The manager can be turned into a subsystem, and the coverage classes into dependent subsystems down the road. Not doing that now because it would impact the parameter scoping, so requires a deprecation cycle.

Result

After the refactor, added a new, no-op coverage system for jacoco. In a subsequent branch, will implement jacoco coverage inside of this class.

jtrobec added some commits Sep 21, 2017

Refactoring to eliminate abstract BaseCoverage class, in favor of a u…
…tility class and a class to manage coverage. The manager can be turned into a subsystem, and the coverage classes dependent subsystems down the road.
# register options for coverage engines
# TODO(jtrobec): get rid of these calls when engines are dependent subsystems
Cobertura.register_options(register, register_jvm_tool)
Jacoco.register_options(register, register_jvm_tool)

This comment has been minimized.

@stuhood

stuhood Sep 21, 2017

Member

As mentioned elsewhere, Jacoco should be a subsystem from the start, to avoid needing to move these options later.

This comment has been minimized.

@jtrobec

jtrobec Sep 21, 2017

Contributor

done, although as discussed subsystem is a factory within jacoco

log=task.context.log)


# TODO(jtrobec): make this a subsystem after deprecating options.

This comment has been minimized.

@stuhood

stuhood Sep 21, 2017

Member

IMO, make it a Subsystem now, and rename the staticmethod register_options to avoid a collision... then later (via the deprecation), the staticmethod will become a classmethod.

This comment has been minimized.

@jtrobec

jtrobec Sep 21, 2017

Contributor

converted to subsystems

@@ -0,0 +1,53 @@
# coding=utf-8
# Copyright 2014 Pants project contributors (see CONTRIBUTORS.md).

This comment has been minimized.

@stuhood

This comment has been minimized.

@jtrobec

jtrobec Sep 21, 2017

Contributor

got it, thx!

"""
:API: public
"""

def settings_from_syscalls(self, options, workdir, log, syscalls):

This comment has been minimized.

@stuhood

stuhood Sep 21, 2017

Member

settings_from_syscalls makes the 'Syscalls' rename sound like a bad idea.

This comment has been minimized.

@jtrobec

jtrobec Sep 21, 2017

Contributor

just changed it to get_settings...this was an odd name, not sure why i went that way. also changed Syscalls to MockSyscalls to be less alarming

return (tgt.is_java or tgt.is_scala) and not tgt.is_test and not tgt.is_synthetic


def initialize_instrument_classpath(settings, targets, instrumentation_classpath):

This comment has been minimized.

@stuhood

stuhood Sep 21, 2017

Member

This seems terribly specific to CoverageSettings and/or BaseCoverage, which wouldn't seem to make it a great candidate for a util package. ... And in particular, since Jacoco doesn't use an instrumentation classpath, isn't this just Cobertura specific at this point?

This comment has been minimized.

@jtrobec

jtrobec Sep 21, 2017

Contributor

moved these to static methods in cobertura, and updated test accordingly

@stuhood stuhood requested a review from jsirois Sep 21, 2017

@stuhood
Copy link
Member

stuhood left a comment

Thanks Justin... looks good to me! One substantive comment.

I'd like @jsirois to take a look before landing this, just to make sure it aligns with what he was thinking about "coverage engines as tasks".

class Factory(Subsystem):
options_scope = 'jacoco'

@classmethod

This comment has been minimized.

@stuhood

stuhood Sep 25, 2017

Member

I think that this should be an instance method: the idea is that you get an instance of the subsystem/Factory with, e.g. global_instance(), and then call the instance method (which can consume the options for that instance using self.get_options(), iirc) to create the CoverageEngine.

log=task.context.log)


class CodeCoverage(Subsystem, SubsystemClientMixin):

This comment has been minimized.

@stuhood

stuhood Sep 25, 2017

Member

Not sure the SubsystemClientMixin inheritance is necessary, but could be wrong.

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Sep 25, 2017

I'll get this a look by EOD.

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Sep 26, 2017

Erm, by tomorrow am 1st thing, getting late and not quite done a release and a hotfix.

@stuhood stuhood merged commit d5ea7db into pantsbuild:master Sep 29, 2017

1 check passed

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

This comment has been minimized.

Copy link
Member

stuhood commented Sep 29, 2017

John's out for a little while, so we'll merge and loop back around if need be.

@wisechengyi wisechengyi referenced this pull request Oct 13, 2017

Open

Run with coverage #317

illicitonion added a commit to twitter/pants that referenced this pull request Oct 18, 2017

stuhood added a commit that referenced this pull request Oct 18, 2017

illicitonion added a commit to twitter/pants that referenced this pull request Oct 23, 2017

Coverage isn't enabled by default
pantsbuild#4881 accidentally enabled it by
default, because --coverage-process being specified implicitly enables
coverage, and it specified a default value for that flag.

stuhood added a commit that referenced this pull request Oct 23, 2017

Coverage isn't enabled by default (#5009)
#4881 accidentally enabled it by
default, because --coverage-process being specified implicitly enables
coverage, and it specified a default value for that flag.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment