From e5b3d4d48915c4e94a3293ab6ef7bd71b4bc55dc Mon Sep 17 00:00:00 2001 From: Matthew Rooney Date: Fri, 18 Aug 2023 11:07:15 +0100 Subject: [PATCH] Fix the issue with coverage that was caused by information being lost with a TypeApply Add the ability to verify measured coverage invocations in CoverageTests --- .../dotc/transform/InstrumentCoverage.scala | 4 +- .../tools/dotc/coverage/CoverageTests.scala | 30 ++++ tests/coverage/run/type-apply/test.check | 1 + .../run/type-apply/test.measurement.check | 7 + tests/coverage/run/type-apply/test.scala | 5 + .../run/type-apply/test.scoverage.check | 139 ++++++++++++++++++ 6 files changed, 184 insertions(+), 2 deletions(-) create mode 100644 tests/coverage/run/type-apply/test.check create mode 100644 tests/coverage/run/type-apply/test.measurement.check create mode 100644 tests/coverage/run/type-apply/test.scala create mode 100644 tests/coverage/run/type-apply/test.scoverage.check diff --git a/compiler/src/dotty/tools/dotc/transform/InstrumentCoverage.scala b/compiler/src/dotty/tools/dotc/transform/InstrumentCoverage.scala index 3e5925899848..7da9ae5df27c 100644 --- a/compiler/src/dotty/tools/dotc/transform/InstrumentCoverage.scala +++ b/compiler/src/dotty/tools/dotc/transform/InstrumentCoverage.scala @@ -237,8 +237,8 @@ class InstrumentCoverage extends MacroTransform with IdentityDenotTransformer: val InstrumentedParts(pre, coverageCall, expr) = tryInstrument(fun) if coverageCall.isEmpty then - // `fun` cannot be instrumented, and `args` is a type so we keep this tree as it is - tree + // `fun` cannot be instrumented and `args` is a type, but `expr` may have been transformed + cpy.TypeApply(tree)(expr, args) else // expr[T] shouldn't be transformed to: // {invoked(...), expr}[T] diff --git a/compiler/test/dotty/tools/dotc/coverage/CoverageTests.scala b/compiler/test/dotty/tools/dotc/coverage/CoverageTests.scala index 77e172f61167..a14d40776358 100644 --- a/compiler/test/dotty/tools/dotc/coverage/CoverageTests.scala +++ b/compiler/test/dotty/tools/dotc/coverage/CoverageTests.scala @@ -17,6 +17,8 @@ import scala.language.unsafeNulls import scala.collection.mutable.Buffer import dotty.tools.dotc.util.DiffUtil +import java.util.stream.Collectors + @Category(Array(classOf[BootstrappedOnlyTests])) class CoverageTests: import CoverageTests.{*, given} @@ -61,6 +63,26 @@ class CoverageTests: val instructions = FileDiff.diffMessage(expectFile.toString, targetFile.toString) fail(s"Coverage report differs from expected data.\n$instructions") + // measurement files only exist in the "run" category + // as these are generated at runtime by the scala.runtime.coverage.Invoker + val expectMeasurementFile = path.resolveSibling(s"$fileName.measurement.check") + if run && Files.exists(expectMeasurementFile) then + + // Note that this assumes that the test invoked was single threaded, + // if that is not the case then this will have to be adjusted + val targetMeasurementFile = findMeasurementFile(targetDir) + + if updateCheckFiles then + Files.copy(targetMeasurementFile, expectMeasurementFile, StandardCopyOption.REPLACE_EXISTING) + + else + val targetMeasurementFile = findMeasurementFile(targetDir) + val expectedMeasurements = fixWindowsPaths(Files.readAllLines(expectMeasurementFile).asScala) + val obtainedMeasurements = fixWindowsPaths(Files.readAllLines(targetMeasurementFile).asScala) + if expectedMeasurements != obtainedMeasurements then + val instructions = FileDiff.diffMessage(expectMeasurementFile.toString, targetMeasurementFile.toString) + fail(s"Measurement report differs from expected data.\n$instructions") + () }) /** Generates the coverage report for the given input file, in a temporary directory. */ @@ -75,6 +97,14 @@ class CoverageTests: test.checkCompile() target + private def findMeasurementFile(targetDir: Path): Path = { + val allFilesInTarget = Files.list(targetDir).collect(Collectors.toList).asScala + allFilesInTarget.filter(_.getFileName.toString.startsWith("scoverage.measurements.")).headOption.getOrElse( + throw new AssertionError(s"Expected to find measurement file in targetDir [${targetDir}] but none were found.") + ) + } + + object CoverageTests extends ParallelTesting: import scala.concurrent.duration.* diff --git a/tests/coverage/run/type-apply/test.check b/tests/coverage/run/type-apply/test.check new file mode 100644 index 000000000000..d4686cafe5a4 --- /dev/null +++ b/tests/coverage/run/type-apply/test.check @@ -0,0 +1 @@ +List(List(1), List(2), List(3)) diff --git a/tests/coverage/run/type-apply/test.measurement.check b/tests/coverage/run/type-apply/test.measurement.check new file mode 100644 index 000000000000..f0382d7405be --- /dev/null +++ b/tests/coverage/run/type-apply/test.measurement.check @@ -0,0 +1,7 @@ +6 +1 +3 +2 +5 +4 +0 diff --git a/tests/coverage/run/type-apply/test.scala b/tests/coverage/run/type-apply/test.scala new file mode 100644 index 000000000000..704a5953fae2 --- /dev/null +++ b/tests/coverage/run/type-apply/test.scala @@ -0,0 +1,5 @@ +@main +def Test: Unit = + // verifies a problematic case where the TypeApply instrumentation was added to the coverage file, + // but was never marked as invoked + println(List(1,2,3).map(a => List(a))) diff --git a/tests/coverage/run/type-apply/test.scoverage.check b/tests/coverage/run/type-apply/test.scoverage.check new file mode 100644 index 000000000000..74dc7fde6bb0 --- /dev/null +++ b/tests/coverage/run/type-apply/test.scoverage.check @@ -0,0 +1,139 @@ +# Coverage data, format version: 3.0 +# Statement data: +# - id +# - source path +# - package name +# - class name +# - class type (Class, Object or Trait) +# - full class name +# - method name +# - start offset +# - end offset +# - line number +# - symbol name +# - tree name +# - is branch +# - invocations count +# - is ignored +# - description (can be multi-line) +# ' ' sign +# ------------------------------------------ +0 +type-apply/test.scala + +test$package$ +Object +.test$package$ +Test +163 +201 +4 +println +Apply +false +0 +false +println(List(1,2,3).map(a => List(a))) + +1 +type-apply/test.scala + +test$package$ +Object +.test$package$ +Test +171 +200 +4 +map +Apply +false +0 +false +List(1,2,3).map(a => List(a)) + +2 +type-apply/test.scala + +test$package$ +Object +.test$package$ +Test +171 +182 +4 +apply +Apply +false +0 +false +List(1,2,3) + +3 +type-apply/test.scala + +test$package$ +Object +.test$package$ +Test +171 +175 +4 +List +Ident +false +0 +false +List + +4 +type-apply/test.scala + +test$package$ +Object +.test$package$ +$anonfun +192 +199 +4 +apply +Apply +false +0 +false +List(a) + +5 +type-apply/test.scala + +test$package$ +Object +.test$package$ +$anonfun +192 +196 +4 +List +Ident +false +0 +false +List + +6 +type-apply/test.scala + +test$package$ +Object +.test$package$ +Test +0 +14 +1 +Test +DefDef +false +0 +false +@main\ndef Test +