-
Notifications
You must be signed in to change notification settings - Fork 275
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
Fix source pathing for code coverage #1006
Changes from 3 commits
555bd09
4eae668
77f2181
9e2e2a2
acaa155
6d51720
576b00f
4bf9408
9a8eaa9
8d0c3da
0293fad
d7f4927
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,8 +36,11 @@ def _phase_coverage(ctx, p, srcjars): | |
output_jar = ctx.actions.declare_file( | ||
"{}-offline.jar".format(input_jar.basename.split(".")[0]), | ||
) | ||
srcs_paths = [] | ||
for src in ctx.files.srcs: | ||
srcs_paths = srcs_paths + [src.path] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not append here? |
||
in_out_pairs = [ | ||
(input_jar, output_jar), | ||
(input_jar, output_jar, srcs_paths), | ||
] | ||
|
||
args = ctx.actions.args() | ||
|
@@ -54,7 +57,7 @@ def _phase_coverage(ctx, p, srcjars): | |
arguments = [args], | ||
) | ||
|
||
replacements = {i: o for (i, o) in in_out_pairs} | ||
replacements = {i: o for (i, o, _) in in_out_pairs} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in_out_pairs seems like a bad name now that they are triples. Maybe rename to |
||
provider = _coverage_replacements_provider.create( | ||
replacements = replacements, | ||
) | ||
|
@@ -73,4 +76,4 @@ def _phase_coverage(ctx, p, srcjars): | |
) | ||
|
||
def _jacoco_offline_instrument_format_each(in_out_pair): | ||
return (["%s=%s" % (in_out_pair[0].path, in_out_pair[1].path)]) | ||
return (["%s=%s=%s" % (in_out_pair[0].path, in_out_pair[1].path, ",".join(in_out_pair[2]))]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment that pair seems like a bad name for something with three elements. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,5 @@ | ||
package coverage; | ||
|
||
object A1 { | ||
def a1(flag: Boolean): B1.type = | ||
if (flag) B1 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,8 @@ | ||
package coverage; | ||
|
||
object A2 { | ||
def a2(): Unit = { | ||
println("a2: " + | ||
"" // B2.b2_a() | ||
println("a2: " + B2.b2_a() | ||
) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,5 @@ | ||
package coverage; | ||
|
||
object B1 { | ||
|
||
def not_called(): Unit = { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,5 @@ | ||
package coverage; | ||
|
||
class B2 { | ||
public static String b2_a() { | ||
return C2.c2("hello from b2_a"); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -62,20 +62,10 @@ scala_library( | |
"A2.scala", | ||
], | ||
deps = [ | ||
# TODO :: Understand why referencing a local java library breaks coverage | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice to fix this! |
||
# ":b2", | ||
":b2", | ||
], | ||
) | ||
|
||
# | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very happy to see this comment go away |
||
# As it stands I can't seem to generate coverage for Java libraries pulled into | ||
# a scala_test target. | ||
# | ||
# The java_library is instrumented, but doesn't have the .uninstrumented files | ||
# that Bazel seems to expect. There are a few code paths for code coverage, so | ||
# down the road we can explore how to fix this... | ||
# | ||
|
||
java_library( | ||
name = "b2", | ||
srcs = [ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,5 @@ | ||
package coverage; | ||
|
||
object C2 { | ||
def c2(input: String): String = | ||
input.reverse | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
package coverage; | ||
import org.scalatest._ | ||
|
||
class TestAll extends FlatSpec { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,5 @@ | ||
package coverage; | ||
|
||
import org.junit.Test; | ||
import org.junit.Assert.*; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,78 +1,110 @@ | ||
SF:/A1.scala | ||
FN:-1,A1$::<clinit> ()V | ||
FN:5,A1$::<init> ()V | ||
FN:3,A1$::a1 (Z)LB1$; | ||
FN:-1,A1::a1 (Z)LB1$; | ||
FNDA:1,A1$::<clinit> ()V | ||
FNDA:1,A1$::<init> ()V | ||
FNDA:1,A1$::a1 (Z)LB1$; | ||
FNDA:0,A1::a1 (Z)LB1$; | ||
SF:test/coverage/A1.scala | ||
FN:-1,coverage/A1$::<clinit> ()V | ||
FN:7,coverage/A1$::<init> ()V | ||
FN:5,coverage/A1$::a1 (Z)Lcoverage/B1$; | ||
FN:-1,coverage/A1::a1 (Z)Lcoverage/B1$; | ||
FNDA:1,coverage/A1$::<clinit> ()V | ||
FNDA:1,coverage/A1$::<init> ()V | ||
FNDA:1,coverage/A1$::a1 (Z)Lcoverage/B1$; | ||
FNDA:0,coverage/A1::a1 (Z)Lcoverage/B1$; | ||
FNF:4 | ||
FNH:3 | ||
BA:3,2 | ||
BA:5,2 | ||
BRF:1 | ||
BRH:1 | ||
DA:3,4 | ||
DA:4,0 | ||
DA:5,5 | ||
DA:5,4 | ||
DA:6,0 | ||
DA:7,5 | ||
LH:2 | ||
LF:3 | ||
end_of_record | ||
SF:/A2.scala | ||
FN:-1,A2$::<clinit> ()V | ||
FN:7,A2$::<init> ()V | ||
FN:3,A2$::a2 ()V | ||
FN:-1,A2::a2 ()V | ||
FNDA:1,A2$::<clinit> ()V | ||
FNDA:1,A2$::<init> ()V | ||
FNDA:1,A2$::a2 ()V | ||
FNDA:0,A2::a2 ()V | ||
SF:test/coverage/A2.scala | ||
FN:-1,coverage/A2$::<clinit> ()V | ||
FN:8,coverage/A2$::<init> ()V | ||
FN:5,coverage/A2$::a2 ()V | ||
FN:-1,coverage/A2::a2 ()V | ||
FNDA:1,coverage/A2$::<clinit> ()V | ||
FNDA:1,coverage/A2$::<init> ()V | ||
FNDA:1,coverage/A2$::a2 ()V | ||
FNDA:0,coverage/A2::a2 ()V | ||
FNF:4 | ||
FNH:3 | ||
DA:3,4 | ||
DA:7,5 | ||
DA:5,11 | ||
DA:8,5 | ||
LH:2 | ||
LF:2 | ||
end_of_record | ||
SF:/B1.scala | ||
FN:-1,B1$::<clinit> ()V | ||
FN:7,B1$::<init> ()V | ||
FN:4,B1$::not_called ()V | ||
FN:-1,B1::not_called ()V | ||
FNDA:1,B1$::<clinit> ()V | ||
FNDA:1,B1$::<init> ()V | ||
FNDA:0,B1$::not_called ()V | ||
FNDA:0,B1::not_called ()V | ||
SF:test/coverage/B1.scala | ||
FN:-1,coverage/B1$::<clinit> ()V | ||
FN:9,coverage/B1$::<init> ()V | ||
FN:6,coverage/B1$::not_called ()V | ||
FN:-1,coverage/B1::not_called ()V | ||
FNDA:1,coverage/B1$::<clinit> ()V | ||
FNDA:1,coverage/B1$::<init> ()V | ||
FNDA:0,coverage/B1$::not_called ()V | ||
FNDA:0,coverage/B1::not_called ()V | ||
FNF:4 | ||
FNH:2 | ||
DA:4,0 | ||
DA:7,5 | ||
DA:6,0 | ||
DA:9,5 | ||
LH:1 | ||
LF:2 | ||
end_of_record | ||
SF:test/coverage/B2.java | ||
FN:3,coverage/B2::<init> ()V | ||
FN:5,coverage/B2::b2_a ()Ljava/lang/String; | ||
FN:9,coverage/B2::b2_b ()V | ||
FNDA:0,coverage/B2::<init> ()V | ||
FNDA:1,coverage/B2::b2_a ()Ljava/lang/String; | ||
FNDA:0,coverage/B2::b2_b ()V | ||
FNF:3 | ||
FNH:1 | ||
DA:3,0 | ||
DA:5,3 | ||
DA:9,0 | ||
DA:10,0 | ||
LH:1 | ||
LF:4 | ||
end_of_record | ||
SF:test/coverage/C2.scala | ||
FN:-1,coverage/C2$::<clinit> ()V | ||
FN:7,coverage/C2$::<init> ()V | ||
FN:5,coverage/C2$::c2 (Ljava/lang/String;)Ljava/lang/String; | ||
FN:-1,coverage/C2::c2 (Ljava/lang/String;)Ljava/lang/String; | ||
FNDA:1,coverage/C2$::<clinit> ()V | ||
FNDA:1,coverage/C2$::<init> ()V | ||
FNDA:1,coverage/C2$::c2 (Ljava/lang/String;)Ljava/lang/String; | ||
FNDA:1,coverage/C2::c2 (Ljava/lang/String;)Ljava/lang/String; | ||
FNF:4 | ||
FNH:4 | ||
DA:5,9 | ||
DA:7,5 | ||
LH:2 | ||
LF:2 | ||
end_of_record | ||
SF:/TestAll.scala | ||
FN:10,TestAll$$anonfun$1::<init> (LTestAll;)V | ||
FN:10,TestAll$$anonfun$1::apply ()V | ||
FN:10,TestAll$$anonfun$1::apply$mcV$sp ()V | ||
FN:6,TestAll$$anonfun$2::<init> (LTestAll;)V | ||
FN:6,TestAll$$anonfun$2::apply ()Lorg/scalatest/compatible/Assertion; | ||
FN:3,TestAll::<init> ()V | ||
FNDA:1,TestAll$$anonfun$1::<init> (LTestAll;)V | ||
FNDA:1,TestAll$$anonfun$1::apply ()V | ||
FNDA:1,TestAll$$anonfun$1::apply$mcV$sp ()V | ||
FNDA:1,TestAll$$anonfun$2::<init> (LTestAll;)V | ||
FNDA:1,TestAll$$anonfun$2::apply ()Lorg/scalatest/compatible/Assertion; | ||
FNDA:1,TestAll::<init> ()V | ||
SF:test/coverage/TestAll.scala | ||
FN:11,coverage/TestAll$$anonfun$1::<init> (Lcoverage/TestAll;)V | ||
FN:11,coverage/TestAll$$anonfun$1::apply ()V | ||
FN:11,coverage/TestAll$$anonfun$1::apply$mcV$sp ()V | ||
FN:7,coverage/TestAll$$anonfun$2::<init> (Lcoverage/TestAll;)V | ||
FN:7,coverage/TestAll$$anonfun$2::apply ()Lorg/scalatest/compatible/Assertion; | ||
FN:4,coverage/TestAll::<init> ()V | ||
FNDA:1,coverage/TestAll$$anonfun$1::<init> (Lcoverage/TestAll;)V | ||
FNDA:1,coverage/TestAll$$anonfun$1::apply ()V | ||
FNDA:1,coverage/TestAll$$anonfun$1::apply$mcV$sp ()V | ||
FNDA:1,coverage/TestAll$$anonfun$2::<init> (Lcoverage/TestAll;)V | ||
FNDA:1,coverage/TestAll$$anonfun$2::apply ()Lorg/scalatest/compatible/Assertion; | ||
FNDA:1,coverage/TestAll::<init> ()V | ||
FNF:6 | ||
FNH:6 | ||
BA:6,2 | ||
BA:7,2 | ||
BRF:1 | ||
BRH:1 | ||
DA:3,2 | ||
DA:5,22 | ||
DA:6,51 | ||
DA:9,23 | ||
DA:10,13 | ||
DA:4,2 | ||
DA:6,22 | ||
DA:7,51 | ||
DA:10,23 | ||
DA:11,13 | ||
LH:5 | ||
LF:5 | ||
end_of_record |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do
src_paths = [src.path for src in ctx.files.srcs]
? IMHO it's a bit cleaner and more readable to do a list comprehension than a loop with mutation.