better test infrastructure for Scala 3 #1528

bjaglin · 2022-01-23T21:59:28Z

Unlocks #1480
Should make testing #1583 easier
Exposed one bug #1594

See commit messages

sbt:scalafix> projects
[info] In file:/home/brice/git/opensource/scalafix/
[info] 	   cli2_11
[info] 	   cli2_12
[info] 	   cli2_13
[info] 	   core2_11
[info] 	   core2_12
[info] 	   core2_13
[info] 	   docs2_13
[info] 	   interfaces2_11
[info] 	   interfaces2_12
[info] 	   interfaces2_13
[info] 	   reflect2_11
[info] 	   reflect2_12
[info] 	   reflect2_13
[info] 	   rules2_11
[info] 	   rules2_12
[info] 	   rules2_13
[info] 	 * scalafix
[info] 	   testkit2_11
[info] 	   testkit2_12
[info] 	   testkit2_13
[info] 	   testsInput2_11
[info] 	   testsInput2_12
[info] 	   testsInput2_13
[info] 	   testsInput3
[info] 	   testsOutput2_11
[info] 	   testsOutput2_12
[info] 	   testsOutput2_13
[info] 	   testsOutput3
[info] 	   testsShared2_11
[info] 	   testsShared2_12
[info] 	   testsShared2_13
[info] 	   testsShared3
[info] 	   unit2_11Target2_11
[info] 	   unit2_12Target2_12
[info] 	   unit2_12Target3
[info] 	   unit2_13Target2_13

bjaglin · 2022-04-09T20:23:42Z

build.sbt

+    buildScalaVersions.map(VirtualAxis.scalaABIVersion) :+ VirtualAxis.jvm: _*
+  )
+  .customRow(
+    scalaVersions = Seq(scala212),


Why not 2.13? Because 2.12 is still the default in sbt-scalafix

bjaglin · 2022-04-09T20:27:44Z

project/ScalafixBuild.scala

-    crossPublishLocalBinTransitive := {
-      val currentState = state.value
-      val ref = thisProjectRef.value
-      val versions = crossScalaVersions.value


https://github.com/sbt/sbt-projectmatrix/blob/e46c2e3c3a4eba186a204303834e5d01893455b7/src/main/scala/sbt/internal/ProjectMatrix.scala#L145 has exactly what we need to iterate over versions so we can get rid of this (which would have required some tweaks anyway since crossScalaVersions is not set anymore). Note that we do lose the optimization of not generating/publishing docs & src, but the removal in complexity is worth it IMHO.

bjaglin · 2022-04-09T21:26:27Z

project/plugins.sbt

@@ -5,3 +5,4 @@ addSbtPlugin("ch.epfl.scala" % "sbt-version-policy" % "2.0.1")
 addSbtPlugin("org.scalameta" % "sbt-mdoc" % "2.3.2")
 addSbtPlugin("org.scoverage" % "sbt-scoverage" % "1.9.3")
 addSbtPlugin("ch.epfl.scala" % "sbt-scalafix" % "0.9.34")
+addSbtPlugin("com.eed3si9n" % "sbt-projectmatrix" % "0.9.0")


see scalacenter/scalafix.g8#19 for some prior discussions

bjaglin · 2022-04-10T15:02:42Z

scalafix-tests/shared/src/main/scala/test/PrettyTest.scala

@@ -93,7 +93,6 @@ object Test {
    val annType1: T @ann(42) = ???
    val annType2: T @ann1 @ann2 = ???

-    val existentialType1: T forSome { type T } = ???


Existential types are longer supported in Scala 3 and rarely used in Scala 2 so for the sake of simplicity, I dropped this completely.

bjaglin · 2022-04-10T15:02:53Z

scalafix-tests/unit/src/main/resources/expect/Pretty.expect

@@ -63,7 +63,7 @@
 [59:9..59:10]:    test/Test.M#m(). => method m: Int
 [62:9..62:10]:    test/Test.N# => trait N extends AnyRef { +1 decls }
 [63:9..63:10]:    test/Test.N#n(). => method n: Int
-[66:9..66:10]:    test/Test.C# => class C extends M { +36 decls }
+[66:9..66:10]:    test/Test.C# => class C extends M { +35 decls }


sbt save-expect

bjaglin · 2022-04-10T15:05:09Z

scalafix-tests/unit/src/test/scala-target2/scalafix/tests/v1/SymbolSuite.scala

+    // The symbol lookup fails against Scala 3.1.1 SemanticDB as the position
+    // there excludes surrounding parentheses while 2.x (scalac-semanticdb) and
+    // the parser include them


I am not familiar with changes in Scala 3, but I know there has been changes to the infix behavior. Where should we report this mismatch between scalameta's parser and dotty's semanticdb output? Is it something that should be addressed in Dotty, in scalameta, or here?

Opening a ticket for tracking as it's a bug that this PR exposes #1594

bjaglin · 2022-04-10T15:06:03Z

build.sbt

+            sourceDir / s"scala-target$n",
+            sourceDir / s"scala-target$n.$m"


this allows to restrict execution of certain tests based on what the unit combination is testing against

bjaglin · 2022-04-10T15:20:56Z

CONTRIBUTING.md

+[sbt-projectmatrix](https://github.com/sbt/sbt-projectmatrix) is used to
+generate several sbt projects `unitXTargetY` with the same source code,
+but for a combination of Scala versions:
+- used for compiling the framework and rules (`X`)
+- used for compiling and generating SemanticDB files for the test input (`Y`)


This is not ideal given the tests currently located in unit: most of them are NOT related to the target, so we end up running twice these "truely unit" tests (unit2_12Target2_12 & unit2_12Target3).

#1593

bjaglin · 2022-04-10T15:46:58Z

scalafix-tests/unit/src/test/scala-target2/scalafix/tests/cli/BaseCliSuite.scala

+  // The defaults of this helper expect to find test input which is currently
+  // built only on scala 2.x since building them for Scala 3 would trigger
+  // test failures as the rules they exercise are not supported on Scala 3.
+  // TODO: switch defaults to rely on much more simple test input/rules so
+  // that we don't need to hardcode a scala version here.


only one user of this helper is actively relying on defaults and fails on Scala 3 targets, but I moved all of them for consistency

bjaglin · 2022-04-10T15:57:52Z

scalafix-tests/unit/src/main/scala/scalafix/test/cli/Rules.scala

@@ -0,0 +1,44 @@
+package scalafix.tests.cli


These rules extracted from CliSyntacticSuite were referenced in

scalafix/scalafix-tests/unit/src/main/resources/META-INF/services/scalafix.v1.Rule

Lines 8 to 13 in 660877e

scalafix.tests.cli.CrashingRule

scalafix.tests.cli.NoOpRule

scalafix.tests.cli.DeprecatedName

scalafix.tests.cli.Scala2_9

scalafix.tests.cli.AvailableRule

scalafix.tests.cli.Disable

so they cannot be in test/scala-2

bjaglin · 2022-04-10T16:00:31Z

project/ScalafixBuild.scala

-      s"++$scala213" ::
-        "cli/crossPublishLocalBinTransitive" :: // scalafix.tests.interfaces.ScalafixSuite
-        s"unit/testOnly -- -l scalafix.internal.tests.utils.SkipWindows" ::
+      "publishLocalTransitive" :: // scalafix.tests.interfaces.ScalafixSuite


publishLocalTransitive is only defined in cli* projects so running this at the root triggers them through aggregation

bjaglin · 2022-04-10T16:20:03Z

scalafix-tests/unit/src/test/scala-target2/scalafix/tests/util/PrettyTypeSuite.scala

+  // As of scalameta 4.5.3, this relies on scalap (and not on TASTy), so it
+  // cannot work against classes compiled with Scala 3


https://github.com/scalameta/scalameta/blob/267c5966bc343ee36683ca36bd19cf6e9b825381/semanticdb/metacp/src/main/scala/scala/meta/internal/metacp/ClassfileInfos.scala#L38-L63

bjaglin · 2022-04-10T16:20:41Z

project/TargetAxis.scala

@@ -0,0 +1,50 @@
+import sbt._


https://github.com/scalacenter/scalafix.g8/pull/19/files#diff-110d8a3e6de17d60251b91e7c6a2099b3f0de4f47146db948b4bdc83ee3597e7

Goals: - make it easier to understand we use 2.12 to run/test against 3 - allow more parallel building/testing

mlachkar · 2022-04-11T12:00:14Z

project/ScalafixBuild.scala

-      s"""set testsInput/scalaVersion := "$scala3"""" ::
-        s"""set testsOutput/scalaVersion := "$scala3"""" ::
-        "unit/testOnly scalafix.tests.rule.RuleSuite" :: s
+      "unit2_12Target3/test" ::


or unit_213Target3: Both should work no ?

unit2_13Target3 does not exist for the moment, see #1528 (comment). We could add it though, but I am not sure it brings a lot of value until/unless scalafixScalaBinaryVersion is updated.

mlachkar · 2022-04-11T12:24:36Z

scalafix-tests/unit/src/test/scala-target2.12/scalafix/tests/v0/LegacySyntheticsSuite.scala

@@ -3,6 +3,7 @@ package scalafix.tests.v0
 import scala.meta._
 import scalafix.tests.core.BaseSemanticSuite

+// LegacySyntheticsTest cannot be compiled with Scala 2.13 or Scala 3


One day we would need to clean these old interfaces

Definitely. I had a quick look at it, but I am not familiar enough with the API to know what is still relevant, so I decided to limit the number of changes...

rvacaru · 2022-04-11T19:01:00Z

Thanks a lot for making #1583 easier to test!

bjaglin mentioned this pull request Apr 3, 2022

Merge OrganizeImports 0.6.0+37-596459af into Scalafix as a built-in rule #1480

Merged

9 tasks

bjaglin force-pushed the projectmatrix branch 2 times, most recently from 2c370fd to b235d4e Compare April 9, 2022 20:19

bjaglin commented Apr 9, 2022

View reviewed changes

bjaglin force-pushed the projectmatrix branch 3 times, most recently from 2367055 to 4d1b2fe Compare April 9, 2022 21:19

bjaglin commented Apr 9, 2022

View reviewed changes

bjaglin force-pushed the projectmatrix branch 5 times, most recently from b307242 to 1d42d5d Compare April 10, 2022 10:17

bjaglin changed the title ~~WIP switch build to projectmatrix~~ WIP switch build to projectmatrix & run more tests against Scala 3 Apr 10, 2022

bjaglin changed the title ~~WIP switch build to projectmatrix & run more tests against Scala 3~~ WIP switch build to sbt-projectmatrix Apr 10, 2022

bjaglin force-pushed the projectmatrix branch 3 times, most recently from 620d6f1 to b375137 Compare April 10, 2022 11:39

bjaglin changed the title ~~WIP switch build to sbt-projectmatrix~~ better test infrastructure for Scala 3 Apr 10, 2022

bjaglin force-pushed the projectmatrix branch from b375137 to 09efda8 Compare April 10, 2022 15:01