From 1c970658246a0089024840c48ed4f2fc8103c4c4 Mon Sep 17 00:00:00 2001 From: Ryan Ferguson Date: Fri, 22 Nov 2024 09:35:27 -0700 Subject: [PATCH 1/3] actual test detection --- .../metals/BuildServerConnection.scala | 2 +- .../internal/metals/MetalsLspService.scala | 6 ++- .../metals/debug/BuildTargetClasses.scala | 26 ++++++++++ .../testProvider/TestFrameworkProvider.scala | 47 ++++++++++++++----- .../metals/testProvider/TestSuitesIndex.scala | 17 ++++--- .../testProvider/TestSuitesProvider.scala | 45 +++++++++++++----- .../frameworks/SpecsTestFinder.scala | 32 ++++++++++--- 7 files changed, 132 insertions(+), 43 deletions(-) diff --git a/metals/src/main/scala/scala/meta/internal/metals/BuildServerConnection.scala b/metals/src/main/scala/scala/meta/internal/metals/BuildServerConnection.scala index ce1c87bae62..aa4edd4bbe3 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/BuildServerConnection.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/BuildServerConnection.scala @@ -147,7 +147,7 @@ class BuildServerConnection private ( /* Currently only Bloop and sbt support running single test cases * and ScalaCLI uses Bloop underneath. */ - def supportsTestSelection: Boolean = isBloop || isSbt || isScalaCLI + def supportsTestSelection: Boolean = isBloop || isSbt || isScalaCLI || isBazel /* Some users may still use an old version of Bloop that relies on scala-debug-adapter 1.x. * Metals does not support scala-debug-adapter 1.x anymore. diff --git a/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala b/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala index cb94068300d..ec516cf4e69 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala @@ -317,7 +317,11 @@ abstract class MetalsLspService( ) protected val testFrameworkProvider: TestFrameworkProvider = - new TestFrameworkProvider(semanticdbs, trees) + new TestFrameworkProvider( + semanticdbs, + definitionIndex, + trees, + ) protected val testProvider: TestSuitesProvider = new TestSuitesProvider( buildTargets, diff --git a/metals/src/main/scala/scala/meta/internal/metals/debug/BuildTargetClasses.scala b/metals/src/main/scala/scala/meta/internal/metals/debug/BuildTargetClasses.scala index 79ff01a706a..7f00fe87015 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/debug/BuildTargetClasses.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/debug/BuildTargetClasses.scala @@ -9,6 +9,7 @@ import scala.meta.internal.metals.BuildTargets import scala.meta.internal.metals.MetalsEnrichments._ import scala.meta.internal.metals.debug.BuildTargetClasses.Classes import scala.meta.internal.metals.debug.BuildTargetClasses.TestSymbolInfo +import scala.meta.internal.metals.testProvider.TestEntry import scala.meta.internal.semanticdb.Scala.Descriptor import scala.meta.internal.semanticdb.Scala.Symbols @@ -171,6 +172,31 @@ final class BuildTargetClasses(val buildTargets: BuildTargets)(implicit } } + /* + * Caches test entries found by metals when the build server fails to provide them + */ + def cacheTestClasses( + target: b.BuildTarget, + testEntries: List[TestEntry], + ): Unit = { + val targetId = target.getId() + val classes = new Classes + testEntries.foreach { testEntry => + val details = testEntry.suiteDetails + val testInfo = BuildTargetClasses.TestSymbolInfo( + details.className + .asInstanceOf[BuildTargetClasses.FullyQualifiedClassName], + details.framework, + ) + classes.testClasses.put( + details.symbol.value.asInstanceOf[BuildTargetClasses.Symbol], + testInfo, + ) + } + index.put(targetId, classes) + + } + private def cacheTestClasses( classes: Map[b.BuildTargetIdentifier, Classes], result: b.ScalaTestClassesResult, diff --git a/metals/src/main/scala/scala/meta/internal/metals/testProvider/TestFrameworkProvider.scala b/metals/src/main/scala/scala/meta/internal/metals/testProvider/TestFrameworkProvider.scala index b30d2f2a247..b681b6a7ec5 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/testProvider/TestFrameworkProvider.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/testProvider/TestFrameworkProvider.scala @@ -3,6 +3,8 @@ package scala.meta.internal.metals.testProvider import ch.epfl.scala.bsp4j.BuildTarget import scala.meta.internal.metals.MetalsEnrichments._ import scala.meta.internal.metals.debug.Specs2 +import scala.meta.internal.mtags +import scala.meta.internal.mtags.GlobalSymbolIndex import scala.meta.internal.mtags.Semanticdbs import scala.meta.internal.mtags.Symbol import scala.meta.internal.parsing.Trees @@ -12,6 +14,7 @@ import scala.meta.transversers._ final class TestFrameworkProvider( semanticdbs: () => Semanticdbs, + symbolIndex: GlobalSymbolIndex, trees: Trees, ) { def getTestSuiteDetailsForPath( @@ -19,22 +22,12 @@ final class TestFrameworkProvider( textDocument: TextDocument, ): List[TestSuiteDetails] = { val location = Range.defaultInstance.toLocation(path.toURI.toString()) - def isASpecsTest(symbol: SymbolInformation) = { - symbol.signature match { - case klass @ ClassSignature(_, parents: Iterable[Type], _, _) => { - parents.exists { case TypeRef(_, name: String, _) => - name == "sbt/testing/Framework#" - } - } - case _ => false - } - } textDocument.symbols.collect { case symbol if isASpecsTest(symbol) => { - val className = ClassName(symbol.displayName.split('.').last) + val className = ClassName(symbol.symbol.split('.').last) TestSuiteDetails( fullyQualifiedName = FullyQualifiedName( - symbol.displayName + symbol.symbol ), // fix maybe? not sure this is the right fully qualified name framework = Specs2, className = className, @@ -44,4 +37,34 @@ final class TestFrameworkProvider( } }.toList } + + private val specsNames = + Set("org/specs2/Specification#", "org/specs2/mutable/Specification#") + + private def isASpecsTest( + symbol: SymbolInformation, + depth: Int = 0, + ): Boolean = { + if (specsNames.contains(symbol.symbol)) { + return true + } + symbol.signature match { + case ClassSignature(_, parents: Iterable[Type], _, _) => { + parents.exists { case TypeRef(_, name: String, _) => + (for { + definition <- symbolIndex.definition(mtags.Symbol(name)) + document <- semanticdbs() + .textDocument(definition.path) + .documentIncludingStale + parentSymbol <- document.symbols.find { s => s.symbol == name } + } yield { + isASpecsTest(parentSymbol, depth + 1) + }) + .getOrElse(false) + } + } + case _ => false + } + } + } diff --git a/metals/src/main/scala/scala/meta/internal/metals/testProvider/TestSuitesIndex.scala b/metals/src/main/scala/scala/meta/internal/metals/testProvider/TestSuitesIndex.scala index 8c018863a3c..63d411c099f 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/testProvider/TestSuitesIndex.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/testProvider/TestSuitesIndex.scala @@ -28,7 +28,7 @@ private[testProvider] final case class TestFileMetadata( hasTestCasesGranularity: Boolean, ) -private[testProvider] final case class TestEntry( +private[metals] final case class TestEntry( buildTarget: BuildTarget, path: AbsolutePath, suiteDetails: TestSuiteDetails, @@ -60,12 +60,9 @@ private[testProvider] final class TestSuitesIndex { /** * Cached, already discovered test suites per build target. * - * For every test suite a.TestSuiteName Metals returns 2 symbols, - * one for object and one for class. - * However, only one of them has location in source code and it's useful. - * That's why FullyQualifiedClassName is used as a key. - * "a/TestSuiteName., a.TestSuiteName" - * "a/TestSuiteName#, a.TestSuiteName" + * For every test suite a.TestSuiteName Metals returns 2 symbols, one for object and one for class. However, only one + * of them has location in source code and it's useful. That's why FullyQualifiedClassName is used as a key. + * "a/TestSuiteName., a.TestSuiteName" "a/TestSuiteName#, a.TestSuiteName" */ private val cachedTestSuites = TrieMap[ @@ -109,8 +106,10 @@ private[testProvider] final class TestSuitesIndex { /** * Determine if test cases should be updated for a given file after compilation - * @param path - file path - * @param md5 - md5 of updated file + * @param path + * \- file path + * @param md5 + * \- md5 of updated file */ def shouldBeUpdated(path: AbsolutePath, md5: String): Boolean = fileToMetadata diff --git a/metals/src/main/scala/scala/meta/internal/metals/testProvider/TestSuitesProvider.scala b/metals/src/main/scala/scala/meta/internal/metals/testProvider/TestSuitesProvider.scala index 5b876405ddc..815e74fdbcf 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/testProvider/TestSuitesProvider.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/testProvider/TestSuitesProvider.scala @@ -22,17 +22,23 @@ import scala.meta.internal.metals.UserConfiguration import scala.meta.internal.metals.clients.language.MetalsLanguageClient import scala.meta.internal.metals.codelenses.CodeLens import scala.meta.internal.metals.debug.BuildTargetClasses -import scala.meta.internal.metals.debug.JUnit4 -import scala.meta.internal.metals.debug.MUnit -import scala.meta.internal.metals.debug.Scalatest -import scala.meta.internal.metals.debug.TestFramework -import scala.meta.internal.metals.debug.Unknown -import scala.meta.internal.metals.debug.WeaverCatsEffect +import scala.meta.internal.metals.debug.{ + JUnit4, + MUnit, + Scalatest, + Specs2, + TestFramework, + Unknown, + WeaverCatsEffect, +} import scala.meta.internal.metals.testProvider.TestExplorerEvent._ -import scala.meta.internal.metals.testProvider.frameworks.JunitTestFinder -import scala.meta.internal.metals.testProvider.frameworks.MunitTestFinder -import scala.meta.internal.metals.testProvider.frameworks.ScalatestTestFinder -import scala.meta.internal.metals.testProvider.frameworks.WeaverCatsEffectTestFinder +import scala.meta.internal.metals.testProvider.frameworks.{ + JunitTestFinder, + MunitTestFinder, + ScalatestTestFinder, + SpecsTestFinder, + WeaverCatsEffectTestFinder, +} import scala.meta.internal.mtags import scala.meta.internal.mtags.GlobalSymbolIndex import scala.meta.internal.mtags.Semanticdbs @@ -72,6 +78,8 @@ final class TestSuitesProvider( new ScalatestTestFinder(trees, symbolIndex, semanticdbs) private val weaverCatsEffect = new WeaverCatsEffectTestFinder(trees, symbolIndex, semanticdbs) + private val specsTestFinder = + new SpecsTestFinder(trees, symbolIndex, semanticdbs) private def isExplorerEnabled = clientConfig.isTestExplorerProvider() && userConfig().testUserInterface == TestUserInterfaceKind.TestExplorer @@ -336,6 +344,13 @@ final class TestSuitesProvider( suiteName = suite.fullyQualifiedName, symbol = suite.symbol, ) + case Specs2 => + specsTestFinder.findTests( + doc = semanticdb, + path = path, + suiteName = suite.fullyQualifiedName, + symbol = suite.symbol, + ) case Unknown => Vector.empty } @@ -424,7 +439,7 @@ final class TestSuitesProvider( ) // when test suite is deleted it has to be removed from cache symbolsPerTargetsWithEmpty.map { - case SymbolsPerTarget(buildTarget, testSymbols) => + case SymbolsPerTarget(buildTarget, testSymbols) => { val fromBSP = testSymbols.values .map(info => FullyQualifiedName(info.fullyQualifiedName)) @@ -439,6 +454,7 @@ final class TestSuitesProvider( } } (buildTarget, removed) + } }.toMap } @@ -463,7 +479,9 @@ final class TestSuitesProvider( .getCapabilities() .getCanTest() ) { - computeTestEntries(currentTarget.target) + val testEntries = computeTestEntries(currentTarget.target) + // buildTargetClasses.cacheTestClasses(currentTarget.target, testEntries) + testEntries } else { currentTarget.testSymbols .readOnlySnapshot() @@ -526,7 +544,7 @@ final class TestSuitesProvider( private def computeTestEntries(buildTarget: BuildTarget): List[TestEntry] = { val sources = buildTargets.buildTargetSources(buildTarget.getId()) - sources.flatMap { path => + val ret = sources.flatMap { path => val docOpt: Option[TextDocument] = semanticdbs().textDocument(path).documentIncludingStale val detailsPerClass = docOpt @@ -540,6 +558,7 @@ final class TestSuitesProvider( ) } }.toList + ret } /** diff --git a/metals/src/main/scala/scala/meta/internal/metals/testProvider/frameworks/SpecsTestFinder.scala b/metals/src/main/scala/scala/meta/internal/metals/testProvider/frameworks/SpecsTestFinder.scala index f9322326243..690bb2596dc 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/testProvider/frameworks/SpecsTestFinder.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/testProvider/frameworks/SpecsTestFinder.scala @@ -2,6 +2,7 @@ package scala.meta.internal.metals.testProvider.frameworks import scala.meta.internal.metals.MetalsEnrichments._ import scala.meta.internal.metals.testProvider.TestCaseEntry +import scala.meta.internal.metals.testProvider.FullyQualifiedName import scala.meta.internal.mtags import scala.meta.internal.parsing.Trees import scala.meta.internal.semanticdb.SymbolInformation @@ -11,16 +12,33 @@ import scala.meta.io.AbsolutePath import scala.meta.transversers._ import scala.reflect.NameTransformer -object SpecsTestFinder { +class SpecsTestFinder( + trees: Trees, + symbolIndex: mtags.GlobalSymbolIndex, + semanticdbs: () => mtags.Semanticdbs, +) { - def collectTestCases(path: AbsolutePath, trees: Trees) = { + def findTests( + doc: TextDocument, + path: AbsolutePath, + suiteName: FullyQualifiedName, + symbol: mtags.Symbol, + ): Seq[TestCaseEntry] = { val treeOpt = trees.get(path) - treeOpt.map { tree => - tree.collect { - case in @ meta.Term.ApplyInfix(lhs, meta.Name("in"), _, _) => { - // TODO (next pr): mark as test + treeOpt + .map { tree => + tree.collect { + case in @ meta.Term.ApplyInfix( + meta.Lit.String(lhs), + meta.Name("in"), + _, + _, + ) => { + val testName = s"${suiteName.value}$lhs" + TestCaseEntry(testName, in.pos.toLsp.toLocation(path.toURI)) + } } } - } + .getOrElse(Nil) } } From 34c124094a9d2ab6fa1cffd4877d5194f7f4d14e Mon Sep 17 00:00:00 2001 From: Ryan Ferguson Date: Wed, 18 Dec 2024 10:29:36 -0700 Subject: [PATCH 2/3] cleanup noisy logs --- .../main/scala/scala/meta/internal/metals/doctor/Doctor.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/metals/src/main/scala/scala/meta/internal/metals/doctor/Doctor.scala b/metals/src/main/scala/scala/meta/internal/metals/doctor/Doctor.scala index 11be0b5ec16..0684b000975 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/doctor/Doctor.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/doctor/Doctor.scala @@ -107,7 +107,7 @@ final class Doctor( scribe.info(s"running doctor check") val scalaTargets = buildTargets.allScala.toList val javaTargets = buildTargets.allJava.toList - scribe.info( + scribe.debug( s"java targets: ${javaTargets.map(_.info.getDisplayName()).mkString(", ")}" ) val summary = problemResolver.problemMessage(scalaTargets, javaTargets) From 3dce84eaf205d561e4b7d3a6595ffcce6c40972a Mon Sep 17 00:00:00 2001 From: Ryan Ferguson Date: Wed, 18 Dec 2024 16:23:29 -0700 Subject: [PATCH 3/3] only detect tests on opened files --- .../testProvider/TestSuitesProvider.scala | 112 ++++++++++-------- 1 file changed, 65 insertions(+), 47 deletions(-) diff --git a/metals/src/main/scala/scala/meta/internal/metals/testProvider/TestSuitesProvider.scala b/metals/src/main/scala/scala/meta/internal/metals/testProvider/TestSuitesProvider.scala index 815e74fdbcf..4d64a763fda 100644 --- a/metals/src/main/scala/scala/meta/internal/metals/testProvider/TestSuitesProvider.scala +++ b/metals/src/main/scala/scala/meta/internal/metals/testProvider/TestSuitesProvider.scala @@ -177,13 +177,45 @@ final class TestSuitesProvider( /** * Check if opened file contains test suite and update test cases if yes. */ - def didOpen(file: AbsolutePath): Future[Unit] = + def didOpen(file: AbsolutePath): Future[Unit] = { // no need to check the index, checking for tests in a single file is cheap if (isExplorerEnabled) Future { - val buildTargetUpdates = getTestCasesForPath(file, None) - updateClientIfNonEmpty(buildTargetUpdates) + val targetOpt = + buildTargets.inverseSources(file).flatMap(buildTargets.info) + targetOpt.map { target => + if (target.getCapabilities().getCanTest()) { + scribe.info(s"Calculating test cases for ${target.getId.getUri}") + // Note: this is terrible, bu t it's the best I could find. + // This could obviously use some cleanup if someone knows a way + val entries = Map(target -> computeTestEntries(target)) + entries.values.foreach(_.foreach(index.put(_))) + val addedEntries = getTestCasesForPath(file, None) + val addedSuites = + entries.mapValues(_.map(_.suiteDetails.asAddEvent)).toMap + val addedTestCases = entries.mapValues { + _.flatMap { entry => + val canResolve = entry.suiteDetails.framework.canResolveChildren + if (canResolve && buffers.contains(entry.path)) + getTestCasesForSuites( + entry.path, + Vector(entry.suiteDetails), + None, + ) + else Nil + } + }.toMap + val buildTargetUpdates = + getBuildTargetUpdates( + Map.empty, + addedSuites, + addedTestCases, + ) + updateClientIfNonEmpty(buildTargetUpdates ++ addedEntries) + } + } } else Future.unit + } /** * Discover tests: @@ -390,7 +422,8 @@ final class TestSuitesProvider( } val deletedSuites = removeStaleTestSuites(symbolsPerTarget) - val addedEntries = getTestEntries(symbolsPerTarget) + val addedEntries: Map[BuildTarget, List[TestEntry]] = + getTestEntries(symbolsPerTarget) // update cached suites with currently discovered addedEntries.foreach { case (_, entries) => @@ -470,48 +503,34 @@ final class TestSuitesProvider( val currentlyCached = index.getSuiteNames(currentTarget.target) val cachedSuites = mutable.Set.from(currentlyCached) - /* buildTarget/scalaTestClasses is deprecated in favor of buildTargetjvmTestEnvironment - * which doesn't provide a framework in the response. - * Since we'll need to find the framework here anyway, relying on it to get the "main" classes seems unnecessary. - */ - if ( - currentTarget.testSymbols.isEmpty && currentTarget.target - .getCapabilities() - .getCanTest() - ) { - val testEntries = computeTestEntries(currentTarget.target) - // buildTargetClasses.cacheTestClasses(currentTarget.target, testEntries) - testEntries - } else { - currentTarget.testSymbols - .readOnlySnapshot() - .toList - // sort the symbols lexically so that symbols with the same fullyQualifiedName - // will be grouped, and the class will come before companion object (i.e. - // `a.b.WordSpec#` < `a.b.WordSpec.`). This ensures that the class is put into the cache - // instead of the companion object. - .sortBy { case (symbol, _) => symbol } - .foldLeft(List.empty[TestEntry]) { - case (entries, (symbol, testSymbolInfo)) => - val fullyQualifiedName = - FullyQualifiedName(testSymbolInfo.fullyQualifiedName) - if (cachedSuites.contains(fullyQualifiedName)) entries - else { - val entryOpt = computeTestEntry( - currentTarget.target, - mtags.Symbol(symbol), - fullyQualifiedName, - testSymbolInfo, - ) - entryOpt match { - case Some(entry) => - cachedSuites.add(entry.suiteDetails.fullyQualifiedName) - entry :: entries - case None => entries - } + currentTarget.testSymbols + .readOnlySnapshot() + .toList + // sort the symbols lexically so that symbols with the same fullyQualifiedName + // will be grouped, and the class will come before companion object (i.e. + // `a.b.WordSpec#` < `a.b.WordSpec.`). This ensures that the class is put into the cache + // instead of the companion object. + .sortBy { case (symbol, _) => symbol } + .foldLeft(List.empty[TestEntry]) { + case (entries, (symbol, testSymbolInfo)) => + val fullyQualifiedName = + FullyQualifiedName(testSymbolInfo.fullyQualifiedName) + if (cachedSuites.contains(fullyQualifiedName)) entries + else { + val entryOpt = computeTestEntry( + currentTarget.target, + mtags.Symbol(symbol), + fullyQualifiedName, + testSymbolInfo, + ) + entryOpt match { + case Some(entry) => + cachedSuites.add(entry.suiteDetails.fullyQualifiedName) + entry :: entries + case None => entries } - } - } + } + } } entries.groupBy(_.buildTarget) @@ -544,7 +563,7 @@ final class TestSuitesProvider( private def computeTestEntries(buildTarget: BuildTarget): List[TestEntry] = { val sources = buildTargets.buildTargetSources(buildTarget.getId()) - val ret = sources.flatMap { path => + sources.flatMap { path => val docOpt: Option[TextDocument] = semanticdbs().textDocument(path).documentIncludingStale val detailsPerClass = docOpt @@ -558,7 +577,6 @@ final class TestSuitesProvider( ) } }.toList - ret } /**