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

Scoverage work - add scoverage to OSS pants #8064

Merged
merged 37 commits into from Jul 27, 2019

Conversation

@sammy-1234
Copy link
Contributor

commented Jul 17, 2019

Problem

This PR is in response to issue #8026.

Solution

It attempts to add scoverge by creating a new subsystem scoverage_platform.py and modifies the scala_library target according to the subsystem in case scoverage is enabled.
Furthermore, it adds a scoverage.py runtime support required for report generation.

Result

Users can run scoverage by running the following command:
./pants --scoverage-enable-scoverage=True test path/to/<scala-target>

Tests

I have added tests for the new subsystem in test_scoverage_platform.py. Run them as follows:
./pants test tests/python/pants_test/backend/jvm/subsystems:test_scoverage_platform

Files changed

Please take a look at my comment below for a list of all the modified files.

@Eric-Arellano
Copy link
Contributor

left a comment

Only looked at where you can use Python 3. Also did this from my phone so please message me on Slack if any of this is confusing.

# Copyright 2015 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

from __future__ import absolute_import, division, print_function, unicode_literals

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jul 17, 2019

Contributor

Don’t need this anymore.

@@ -0,0 +1,94 @@
# coding=utf-8

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jul 17, 2019

Contributor

Don’t need this because we don’t care about Python 2 support.

Show resolved Hide resolved src/python/pants/backend/jvm/subsystems/scala_coverage_platform.py Outdated
@property
def injectables_spec_mapping(self):
return {
'scoverage': ['{}'.format(self.get_options().scoverage_target_path)],

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jul 17, 2019

Contributor

Can use an f string

`f”{self.get_options...}”

"""
Adds 'scoverage' to scalac_plugins in case scoverage is enabled for that [target].
:return: modified scalac_plugins
:rtype: list of strings

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jul 17, 2019

Contributor

Rather than putting this in docstring, can use a type hint. On line 40 put -> List[str] between the param list and colon. See strutil.py for an example.

You’ll also need to add from typing import List

"""
Adds 'scoverage' to scalac_plugins_args in case scoverage is enabled for that [target].
:return: modified scalac_plugins_args
:rtype: map from string to list of strings.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jul 17, 2019

Contributor

Return type is Dict[str, List[str]]

scalac_plugin_args = target.payload.scalac_plugin_args
if scalac_plugin_args:
scalac_plugin_args.update(
{"scoverage": ["writeToClasspath:true", "dataDir:{}".format(target.identifier)]})

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Jul 17, 2019

Contributor

F strings here and a few lines below

@sammy-1234

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

Thanks @Eric-Arellano 💯 for python3 help.

The latest commit removes scoverage target from BUILD.tools and creates an "injectable" for the same in ScalaCoveragePlatform subsystem.

@sammy-1234 sammy-1234 force-pushed the sammy-1234:scoverageWork branch from d11fd6c to fc48b64 Jul 18, 2019

sammy-1234 and others added some commits Jul 18, 2019

Update src/python/pants/backend/jvm/subsystems/scala_coverage_platfor…
…m.py

Co-Authored-By: Eric Arellano <ericarellano@me.com>
Sameer Arora
@stuhood
Copy link
Member

left a comment

Thanks! Looks like a great start.

I'd like to see (at least) a draft of the second half of the change before we land this though.

@@ -0,0 +1,3 @@
target(

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 19, 2019

Member

I believe that this target can be removed now.

pants.ini Outdated
@@ -220,6 +220,13 @@ no_warning_args: [
'-S-nowarn',
]

compiler_option_sets_enabled_args: {
# Needed to make scoverage CodeGrid highlighting work
'scoverage': [

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 19, 2019

Member

Rather than having to manually include this, we should automatically include it when scoverage is enabled. Probably somewhere around here?

compiler_option_sets_args = self.get_merged_args_for_compiler_option_sets(compiler_option_sets)
zinc_args.extend(compiler_option_sets_args)

blacklist_file = 'new_blacklist_scoverage'


class ScalaCoveragePlatform(InjectablesMixin, Subsystem):

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 19, 2019

Member

Naming nit: rather than calling this ScalaCoverage, we should probably call it Scoverage, as that seems to be the project's actual name.

class ScalaCoveragePlatform(InjectablesMixin, Subsystem):
"""The scala coverage platform."""

options_scope = 'scala-coverage'

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 19, 2019

Member

ditto naming.

register('--enable-scoverage',
default=False,
type=bool,
help='Specifies whether to generate scoverage reports for scala test targets.'

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 19, 2019

Member

This should be consumed from this subsystem instead:

register('--coverage-open', type=bool, fingerprint=True,
help='Open the generated HTML coverage report in a browser. Implies --coverage.')

This comment has been minimized.

Copy link
@sammy-1234

sammy-1234 Jul 19, 2019

Author Contributor

If I understand correctly, manager.py is initialized by junit_run.py at runtime only. However, I also need this option (enable-scoverage) at compile time to determine whether to instrument the targets or not. That is why I kept it here. Am I thinking about this right?

This comment has been minimized.

Copy link
@sammy-1234

sammy-1234 Jul 22, 2019

Author Contributor

I think I see what is happening here: Even though the CodeCoverage subsystem gets initialized, none of its options are registered. This is because the subsystem is using def register_junit_options(register, register_jvm_tool): instead of def register_options(cls, register):

while the options under the latter (register_options) gets automatically registered, the former (register_junit_options) requires to be called by some other code, e.g, this is how it is called in junit_run.py:

# TODO(jtrobec): Remove direct register when coverage steps are moved to their own subsystem.
CodeCoverage.register_junit_options(register, cls.register_jvm_tool)

And when it is called by some other class,( junit in this example), the scope of that class gets prepended to the scope of coverage, thus registering the options as --test-junit-coverage

Consequently, setting --scoverage-coverage=True (scope at compile time) does not automatically set --test-junit-coverage=True(scope at runtime) (and vice-versa)

:return: See constructor.
:rtype: list of strings.
"""
if self._scoverage:

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 19, 2019

Member

Rather than fully overriding the set of plugins, this should merge both lists. Ditto the plugin args and options.

This comment has been minimized.

Copy link
@sammy-1234

sammy-1234 Jul 19, 2019

Author Contributor

the function get_scalac_plugins in scoverage_platform.py does that. It merges the list if initially non empty (otherwise creates a new one) Similarly, for plugin args and options, the merging happens inside the scoverage subsystem.

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 22, 2019

Member

Ok. That's a bit confusing, because if someone were to change line 102 here, they would need to remember to change the subsystem code as well. IMO, the merging code should live in here to avoid that issue.

@sammy-1234

This comment has been minimized.

Copy link
Contributor Author

commented Jul 20, 2019

Scoverage (2/2) - Adding report generator
This 2nd PR is a draft of how report generator would look in pants.

Files Changes

backend/jvm/tasks/coverage/manager.py: Adding support for scoverage

New Files Added

backend/jvm/tasks/coverage/scoverage.py: scoverage runtime subsystem
src/scala/org/pantsbuild/scoverage/report/ScoverageRep.scala: scoverage report generator
src/scala/org/pantsbuild/scoverage/report/Settings.scala: scoverage report generator settings

Sameer Arora
@stuhood
Copy link
Member

left a comment

Thanks! This looks really good.

As mentioned below: please break out a separate PR for the scoverage report generation JVM code. We'll have to land that first.

:return: See constructor.
:rtype: list of strings.
"""
if self._scoverage:

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 22, 2019

Member

Ok. That's a bit confusing, because if someone were to change line 102 here, they would need to remember to change the subsystem code as well. IMO, the merging code should live in here to avoid that issue.

Show resolved Hide resolved src/python/pants/backend/jvm/subsystems/scoverage_platform.py Outdated
if self._settings.coverage_open:
return os.path.join(base_report_dir, 'html', 'index.html')

def _execute_scoverage_report_gen(self, measurements_dir, report_dir, target_filter):

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 22, 2019

Member

Another PR will need to be broken out to land the report generation JVM code so that we can publish it to maven central and then consume it here as a register_jvm_tool.

@sammy-1234 sammy-1234 changed the title Scoverage work (1/2) - add scoverage to OSS pants Scoverage work - add scoverage to OSS pants Jul 22, 2019

Show resolved Hide resolved BUILD.tools Outdated
dependencies = [
'src/python/pants/option',
'src/python/pants/subsystem',
'src/python/pants/java/jar',

This comment has been minimized.

Copy link
@nsaechao

nsaechao Jul 22, 2019

Collaborator

nit: extra space?

Show resolved Hide resolved src/python/pants/backend/jvm/subsystems/scoverage_platform.py Outdated
Show resolved Hide resolved src/python/pants/backend/jvm/subsystems/scoverage_platform.py Outdated
compiler_option_sets = [SCOVERAGE]
return tuple(compiler_option_sets)

def is_blacklisted(self, target) -> bool:

This comment has been minimized.

Copy link
@nsaechao

nsaechao Jul 22, 2019

Collaborator

The blacklist should be cached in-memory, currently the file is read per target.

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 25, 2019

Member

If you switch this to a list option (as above) that will be handled automatically.

Show resolved Hide resolved src/python/pants/backend/jvm/tasks/coverage/scoverage.py
Show resolved Hide resolved src/python/pants/backend/jvm/tasks/coverage/scoverage.py
Show resolved Hide resolved src/python/pants/backend/jvm/tasks/coverage/scoverage.py Outdated
f'"{report_dir}"]',
]

# if target_filter:

This comment has been minimized.

Copy link
@nsaechao

nsaechao Jul 22, 2019

Collaborator

dead code? can be removed

Show resolved Hide resolved src/scala/org/pantsbuild/scoverage/report/ScoverageReport.scala Outdated
@sammy-1234

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

Scoverage Outline

Major files added

scoverage_platform.py: Scoverage subsystem
scoverage.py: Scoverage runtime environment (similar to Jacoco and Cobertura)
test_scoverage_platform.py: tests for scoverage

Major files changed

scala_library.py: modifications to scala library in case scoverage is enabled
manager.py: modifying the coverage manager to choose scoverage in case it is enabled
register.py: registering the subsystem
zinc.py: Adding compiler_options_sets_args for scoverage to work.

Other changes

  • Adding required build files
  • Changing existsing tests to add Scoverage dependency in order to pass unit tests,

Sameer Arora added some commits Jul 24, 2019

Sameer Arora
Sameer Arora
Sameer Arora
@stuhood
Copy link
Member

left a comment

Thanks!

default=False,
type=bool,
help='Specifies whether to generate scoverage reports for scala test targets.'
'Default value is False. If True,'

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 25, 2019

Member

These lines need spaces in them in order to avoid running together.

'Default value is False. If True,'
'implies --test-junit-coverage-processor=scoverage.')

register('--blacklist-file',

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 25, 2019

Member

Pants supports @file options natively, so you should make this option a list option:

register('--blacklist-targets',
  type=list,
  member_type=target_option,
  ..
)

And then someone who wants to put the list of targets in a file (in python list syntax) can pass --blacklist-targets=@some-blacklist-file.

You could also pass in a (string) list of regexes to match against target names.

compiler_option_sets = [SCOVERAGE]
return tuple(compiler_option_sets)

def is_blacklisted(self, target) -> bool:

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 25, 2019

Member

If you switch this to a list option (as above) that will be handled automatically.

return False

if target.address.spec in self._blacklist_file_contents:
logger.warning(f"{target.address.spec} found in blacklist, not instrumented.")

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 25, 2019

Member

This should probably be at debug or info rather than warn.

@@ -47,6 +51,8 @@ def __init__(self, java_sources=None, payload=None, **kwargs):
})
super().__init__(payload=payload, **kwargs)

self._scoverage_instance = ScoveragePlatform.global_instance()

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 25, 2019

Member

The target itself doesn't need to hold a reference to _scoverage_instance: can get it lazily from the singleton.

]
)

register('--target-filters', type=list, default=[],

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 25, 2019

Member

This feels redundant with the ScoveragePlatform.blacklist option. Do we need both?

This comment has been minimized.

Copy link
@sammy-1234

sammy-1234 Jul 25, 2019

Author Contributor

This target filtering is for report generation, i.e helps in generating reports only for the specified targets. The blacklist option is more for overcoming the issue that if some targets cannot be instrumented at all (due to exceeding JVM code size limits), they should be passed into the blacklist.

This comment has been minimized.

Copy link
@sammy-1234

sammy-1234 Jul 26, 2019

Author Contributor

this option helps in preventing the need to compile again if the report is needed only for a handful of targets, i.e, say I compiled 500 targets and got the report generated. But now I need a report for all targets containing the class "merlin"(~ 10 targets), or all targets in package "report", then I can get those reports without needing to compile everything again.

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 26, 2019

Member

Hm. But in reality, are people going to know that? From a user's perspective, having just one option would likely be easier to understand.

This comment has been minimized.

Copy link
@sammy-1234

sammy-1234 Jul 26, 2019

Author Contributor

Agreed. People just need to use this target-filter option most of the time. In case they encounter this error of JVM code size limits, they can overcome that with the help of blacklist option as well. More like a Troubleshooting mechanism. I'll make sure to document this.

def slf4j_jar(name):
return JarDependency(org='org.slf4j', name=name, rev='1.7.5')

def scopt_jar():

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 25, 2019

Member

This is only used one place, so maybe just inline it.

def _iter_datafiles(self, output_dir):
for root, _, files in safe_walk(output_dir):
for f in files:
if f.startswith("scoverage"):

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 25, 2019

Member

Should move these comments into python method docs:

def _iter_datafiles(self, output_dir):
   """This is a docstring for a method.

   With some other stuff in it.
   """
   ..
Show resolved Hide resolved src/python/pants/backend/jvm/tasks/coverage/scoverage.py
@@ -322,6 +322,11 @@ def relative_to_exec_root(path):
zinc_args.append('-transactional')

compiler_option_sets_args = self.get_merged_args_for_compiler_option_sets(compiler_option_sets)

# Needed to make scoverage CodeGrid highlighting work

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 25, 2019

Member

This should be incorporated in ScalaLibrary rather than here.

This comment has been minimized.

Copy link
@sammy-1234

sammy-1234 Jul 25, 2019

Author Contributor

How can I set compiler_option_set_enabled_args in ScalaLibrary. I looked at the compiler_option_sets_mixin.py here : https://github.com/pantsbuild/pants/blob/14e0bd6b27910310e225ff9586c2410459153fd9/src/python/pants/option/compiler_option_sets_mixin.py But there is no API for adding extra arguments. That is I why added it manually here.

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 26, 2019

Member

Ok, fine with this as is I guess.

help='Path to files containing targets not to be instrumented.')

register('--scoverage-target-path',
default='//:scoverage',

This comment has been minimized.

Copy link
@nsaechao

nsaechao Jul 25, 2019

Collaborator

It looks like this target is bootstrapped by default. If a user overrides this, the default //:scoverage target is still injected into the build graph.

@nsaechao
Copy link
Collaborator

left a comment

Thanks Sameer for doing this awesome work, based off my previous review and assuming you address Stu's concerns/changes. Everything looks good to me.

@stuhood
Copy link
Member

left a comment

Looks great! Thanks.

else:
compiler_option_sets = [SCOVERAGE]

super().__init__(payload=payload,

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 26, 2019

Member

Having two super calls is pretty weird, and in general this has resulted in a lot of nesting.

An alternative might be to do something like:

scalac_plugins = ... or []
scalac_plugin_args = ... or []
compiler_option_sets = ... or []
if ScoveragePlatform.global_instance().get_options().enable_scoverage:
  scalac_plugins.append(..)
  scalac_plugin_args.update(..)
  compiler_option_sets.update(..)
super(...)
]
)

register('--target-filters', type=list, default=[],

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 26, 2019

Member

Hm. But in reality, are people going to know that? From a user's perspective, having just one option would likely be easier to understand.

Show resolved Hide resolved src/python/pants/backend/jvm/tasks/coverage/scoverage.py
@@ -322,6 +322,11 @@ def relative_to_exec_root(path):
zinc_args.append('-transactional')

compiler_option_sets_args = self.get_merged_args_for_compiler_option_sets(compiler_option_sets)

# Needed to make scoverage CodeGrid highlighting work

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 26, 2019

Member

Ok, fine with this as is I guess.

Sameer Arora added some commits Jul 26, 2019

Sameer Arora
@stuhood

This comment has been minimized.

Copy link
Member

commented Jul 27, 2019

@sammy-1234: re: the lint errors: are you using the commit hooks? https://www.pantsbuild.org/howto_contribute.html#getting-pants-source-code

@sammy-1234

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2019

Thanks for that. But even then, it is still giving me this weird error:
[INFO] pexrc interpreters requested and found, but other paths were also specified, so interpreters may not be restricted to the pexrc ones. Full search path is: /opt/ee/python/2.7/bin/python2.7:/opt/ee/python/3.6/bin/python3.6:/usr/local/bin/python3.7:/Users/sameera/Desktop/forked_pants/pants/build-support/pants_dev_deps.py37.venv/bin:/opt/twitter_mde/bin:/opt/twitter_mde/homebrew_minimal/mde_bin:/usr/local/mysql/bin:/Users/sameera/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/munki:/opt/twitter_mde/data/node/bin:/Users/sameera/.npm-global/bin

and then failing with:

Failed to execute PEX file, missing macosx_10_14_x86_64-cp-27-cp27m compatible dependencies for: mypy

Any clue how I can resolve this? Thanks :)

@stuhood

This comment has been minimized.

Copy link
Member

commented Jul 27, 2019

@sammy-1234 : If you have an /etc/pexrc file, you should: sudo mv /etc/pexrc /etc/pexrc.old, run clean-all, and then the hooks will work. This is an unfortunate sideeffect of some of the setup on our laptops.

Sameer Arora
@sammy-1234

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2019

Thanks @stuhood . The hooks pass now 💯

@stuhood stuhood merged commit 23a8a2b into pantsbuild:master Jul 27, 2019

1 check passed

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

@sammy-1234 sammy-1234 deleted the sammy-1234:scoverageWork branch Jul 27, 2019

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