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 report generator #8098

Merged
merged 11 commits into from Jul 24, 2019

Conversation

@sammy-1234
Copy link
Contributor

commented Jul 23, 2019

Problem

Scoverage needs a report generator for generating html and xml reports of code coverage of scala test targets.

Solution

Creating a report generator for scoverage.

Result

We can publish this artifact and then consume it in a subsystem to generate scoverage reports for scala test targets.

@stuhood
Copy link
Member

left a comment

Thanks!


jar_library(name='scalac-scoverage-plugin',
jars=[
scala_jar(org='com.twitter.scoverage', name='scalac-scoverage-plugin', rev='1.0.1-twitter'),

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 23, 2019

Member

Please include a link to the github repository of the fork, the PR that the fork contains, and a bit of explanation of what we should do to resolve the fork (ie, periodically ping upstream on that PR).

Also, since this is named com.twitter.scoverage, should put it in a subdirectory probably: 3rdparty/jvm/com/twitter/scoverage/BUILD

scala_library(
provides=scala_artifact(
org='org.pantsbuild',
name='scoverageReport',

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 23, 2019

Member

JVM artifacts tend to have dashed names. So maybe scoverage-report-generator.

package org.pantsbuild.scoverage.report

import java.io.File
import sbt.internal.util.ConsoleLogger

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 23, 2019

Member

This should probably not depend on SBT at all. Instead, should probably swap this to slf4j-api with slf4j-jdk14 as the logging implementation. See https://www.slf4j.org/manual.html


// Copying the logger from org.pantsbuild.zinc.bootstrap
// As per https://github.com/pantsbuild/pants/issues/6160, this is a workaround
// so we can run zinc without $PATH (as needed in remoting).

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 23, 2019

Member

(as above: no sbt dep plz)

val writeReports: Boolean = {
val write = writeHtml || writeXml
if (!write) {
// we could end here, but we will still attempt to load the coverage and dump stats as info

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 23, 2019

Member

If it simplifies things, I would suggest just failing here.

def getAllCoverageDirs(dataDir: File, acc: Seq[File]): Seq[File] = {
if (dataDir.listFiles.filter(_.isFile).toSeq.exists(_.getName contains "scoverage.coverage")) {
dataDir.listFiles.filter(_.isDirectory).toSeq
.foldRight(acc :+ dataDir) { (e, a) => getAllCoverageDirs(e, a) }

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 23, 2019

Member

Appending to a Seq is not always cheap. If you want to ensure that you have cheap append, your argument should probably be a Vector[File].

This comment has been minimized.

Copy link
@sammy-1234

sammy-1234 Jul 23, 2019

Author Contributor

I assume prepending is cheap?

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 23, 2019

Member

Prepending is only cheap if you are using List or Vector... but not Array, for example. So it's good to be specific in recursive code like this.

This comment has been minimized.

Copy link
@sammy-1234

sammy-1234 Jul 23, 2019

Author Contributor

Alrighty, made it to a vector. Thanks

val dataDirs: Seq[File] = filterFiles(dataDir, options)
aggregate(dataDirs)
} else {
logger.error("Coverage directory does not exists.")

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 23, 2019

Member

does not exist

if (reportOptions.writeReports) {
writeReports(cov, reportOptions)
}
case _ => logger.error("Failed to load coverage")

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 23, 2019

Member

All of the failure cases in this matchshould cause the process to exit unsuccessfully.

To exit unsuccessfully, the process should either:

  1. raise an exception
  2. call System.exit(1)

I think that rather than having the loadCoverage method return an Option, you might consider having it just directly raise an exception. But if you don't feel comfortable with that and would prefer a functional style, a better thing to return would probably be Either[Coverage, String] (where the right hand side would be an error message to exit with). https://www.scala-lang.org/api/2.12.8/scala/util/Either.html

This comment has been minimized.

Copy link
@sammy-1234

sammy-1234 Jul 23, 2019

Author Contributor

loadCoverage now raises an exception


opt[String]("sourceDirPath")
.action((dir: String, s: Settings) => s.copy(sourceDirPath = dir))
.text("Directory containing the project sources.")

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 23, 2019

Member

"Project sources" is pretty ambiguous... is this tool used per-target?

This comment has been minimized.

Copy link
@sammy-1234

sammy-1234 Jul 23, 2019

Author Contributor

No, its just there for backwards compatibility. I don't think there is any use for this tool for latest ReportWriters (or in my report generator):
https://github.com/scoverage/scalac-scoverage-plugin/blob/9bb6198cb6c4dc6498d034ba4c1810ea2ac8d62c/scalac-scoverage-plugin/src/main/scala/scoverage/report/ScoverageHtmlWriter.scala#L13-L22

Since its still necessary to pass it in as a parameter, I just pass in the current directory by default.

.action((dir: String, s: Settings) => s.copy(measurementsDirPath = dir))
.text("Directory where all scoverage measurements data is stored.")

opt[String]("reportDirPath")

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 23, 2019

Member

Rather than taking a reportDirPath and boolean writeHtmlReport and writeXmlReport settings, consider maybe taking explicit paths for each of those? Otherwise the caller has to guess the name of the output inside the destination directory.

This comment has been minimized.

Copy link
@sammy-1234

sammy-1234 Jul 23, 2019

Author Contributor

Currently, if both options are on, it creates < dest >/html and < dest >/xml inside the destination directory. If only one option is on (say html) it creates just < dest >/html. Would this not work?

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 23, 2019

Member

That's more confusing than just letting the caller choose where to put each thing.

* @return [Coverage] object for the [dataDirs]
*/
private def aggregatedCoverage(dataDirs: Seq[File]): Coverage = {
var id = 0

This comment has been minimized.

Copy link
@nsaechao

nsaechao Jul 23, 2019

Collaborator

Possibly could use AtomicInteger here if trying to increment ids e.g., though this class is more for thread-safe op.

import java.util.concurrent.atomic.AtomicInteger
val id = new AtomicInteger()
...
coverage add stmt.copy(id = id.incrementAndGet())
* @return Coverage object wrapped in Option
*/
private def aggregate(dataDirs: Seq[File]): Option[Coverage] = {
logger.info(s"Found ${dataDirs.size} subproject scoverage data directories [${dataDirs.mkString(",")}]")

This comment has been minimized.

Copy link
@nsaechao

nsaechao Jul 23, 2019

Collaborator

If dataDirs is null, this will throw an NullPointerException

This comment has been minimized.

Copy link
@nsaechao

nsaechao Jul 23, 2019

Collaborator

Feels like this method could just be consolidated with aggregatedCoverage

This comment has been minimized.

Copy link
@sammy-1234

sammy-1234 Jul 23, 2019

Author Contributor

aggregate is called only by loadAggregatedCoverage and I am checking there if the dataDirs is null or not. If it doesn't exist, it raises a runtime exception.

def filterFiles(dataDir: File, options: ReportOptions): Seq[File] = {
val targetFiles = options.targetFilters

val coverareDirs = getAllCoverageDirs(dataDir, Seq())

This comment has been minimized.

Copy link
@nsaechao

nsaechao Jul 23, 2019

Collaborator

typo coverareDirs?

.action((dir: String, s: Settings) => s.copy(sourceDirPath = dir))
.text("Directory containing the project sources.")

opt[String]("dataDirPath")

This comment has been minimized.

Copy link
@nsaechao

nsaechao Jul 23, 2019

Collaborator

Looks like this option and loadDataDir are tightly bounded, does it make sense to just consolidate to one?

.text("Scoverage data file directory to be used in case report needed for single measurements " +
"directory. Must set `loadDataDir` to use this options.")

opt[Unit]("writeHtmlReport")

This comment has been minimized.

Copy link
@nsaechao

nsaechao Jul 23, 2019

Collaborator

nit: I think the general usage of output over write is more dominant.


opt[Seq[String]]("targetFilters")
.action((f: Seq[String], s: Settings) => s.copy(targetFilters = f))
.text("Directory names for which report has to be generated.")

This comment has been minimized.

Copy link
@nsaechao

nsaechao Jul 23, 2019

Collaborator

I'm unclear here, does this mean that only targets listed here will be generated? What happens if this is blank?

This comment has been minimized.

Copy link
@sammy-1234

sammy-1234 Jul 23, 2019

Author Contributor

If this is blank, the reports are generated for all targets where scoverage.coverage files are found. But yes, If this is not null, we generate the reports for only the specified targets

Sameer Arora added some commits Jul 23, 2019

Sameer Arora
Sameer Arora
@stuhood
Copy link
Member

left a comment

Thanks, looks good!

@stuhood

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

One flake in CI. Will merge on green.

@stuhood stuhood merged commit b0877f8 into pantsbuild:master Jul 24, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.