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

fix: adjust tests for 3.2.0-NIGHTLY #3687

Merged
merged 1 commit into from
Mar 10, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,9 @@ object CompletionValue:
case class Keyword(label: String, insertText: String) extends CompletionValue

def fromCompiler(completion: Completion): List[CompletionValue] =
completion.symbols.map(Compiler(completion.label, _))
def undoBacktick(label: String): String =
label.stripPrefix("`").stripSuffix("`")
Comment on lines +75 to +76
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming these are due to my changes in scala/scala3#14594. Is this how we want to handle this though? In these cases aren't we stripping them here and then adding them back later? Would it be better to instead check when we are about to add them and if they exist already, then don't add them?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ckipp01 I agree that it isn't perfect but they anyway should be removed from completion label

completion.symbols.map(Compiler(undoBacktick(completion.label), _))

def namedArg(label: String, sym: Symbol)(using Context): CompletionValue =
NamedArg(label, sym.info.widenTermRefExpr)
Expand Down
85 changes: 36 additions & 49 deletions mtags/src/main/scala/scala/meta/internal/semver/SemVer.scala
Original file line number Diff line number Diff line change
@@ -1,79 +1,66 @@
package scala.meta.internal.semver

import scala.util.Try

object SemVer {

case class Version(
major: Int,
minor: Int,
patch: Int,
releaseCandidate: Option[Int],
milestone: Option[Int]
releaseCandidate: Option[Int] = None,
milestone: Option[Int] = None,
nightlyDate: Option[Int] = None
) {
def >(that: Version): Boolean = {
lazy val baseVersionEqual =
this.major == this.major && this.minor == that.minor && this.patch == that.patch
val diff = toList
.zip(that.toList)
.collectFirst {
case (a, b) if a - b != 0 => a - b
}
.getOrElse(0)
diff > 0
}

def <(that: Version): Boolean =
that > this

this.major > that.major ||
(this.major == that.major && this.minor > that.minor) ||
(this.major == that.major && this.minor == that.minor && this.patch > that.patch) ||
// 3.0.0-RC1 > 3.0.0-M1
baseVersionEqual && this.releaseCandidate.isDefined && that.milestone.isDefined ||
// 3.0.0 > 3.0.0-M2 and 3.0.0 > 3.0.0-RC1
baseVersionEqual && (this.milestone.isEmpty && this.releaseCandidate.isEmpty && (that.milestone.isDefined || that.releaseCandidate.isDefined)) ||
// 3.0.0-RC2 > 3.0.0-RC1
baseVersionEqual && comparePreRelease(
that,
(v: Version) => v.releaseCandidate
) ||
// 3.0.0-M2 > 3.0.0-M1
baseVersionEqual && comparePreRelease(that, (v: Version) => v.milestone)
private def toList: List[Int] = {
val rcMilestonePart =
releaseCandidate
.map(v => List(1, v))
.orElse(milestone.map(v => List(0, v)))
.getOrElse(List(2, 0))

List(major, minor, patch) ++ rcMilestonePart ++
List(nightlyDate.getOrElse(Int.MaxValue))
}

def >=(that: Version): Boolean = this > that || this == that

private def comparePreRelease(
that: Version,
preRelease: Version => Option[Int]
): Boolean = {
val thisPrerelease = preRelease(this)
val thatPrerelease = preRelease(that)
this.major == that.major && this.minor == that.minor && this.patch == that.patch &&
thisPrerelease.isDefined && thatPrerelease.isDefined && thisPrerelease
.zip(thatPrerelease)
.exists { case (a, b) => a > b }
}

override def toString: String =
List(
Some(s"$major.$minor.$patch"),
releaseCandidate.map(s => s"-RC$s"),
milestone.map(s => s"-M$s")
milestone.map(s => s"-M$s"),
nightlyDate.map(d => s"-$d-NIGHTLY")
).flatten.mkString("")

}

object Version {
def fromString(version: String): Version = {
val Array(major, minor, patch) =
version.replaceAll("(-|\\+).+$", "").split('.').map(_.toInt)

val prereleaseString = version.stripPrefix(s"$major.$minor.$patch")

def fromSuffix(name: String) = {
if (prereleaseString.startsWith(name))
Try(
prereleaseString.stripPrefix(name).replaceAll("\\-.*", "").toInt
).toOption
else None
}
val releaseCandidate = fromSuffix("-RC")
val milestone = fromSuffix("-M")

Version(major, minor, patch, releaseCandidate, milestone)
val parts = version.split("\\.|-")
val Array(major, minor, patch) = parts.take(3).map(_.toInt)
val (rc, milestone) = parts
.lift(3)
.map { v =>
if (v.startsWith("RC")) (Some(v.stripPrefix("RC").toInt), None)
else (None, Some(v.stripPrefix("M").toInt))
}
.getOrElse((None, None))
val date = parts.lift(5).map(_.toInt)
Version(major, minor, patch, rc, milestone, date)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

}

def isCompatibleVersion(minimumVersion: String, version: String): Boolean = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ class HoverScala3TypeSuite extends BaseHoverSuite {
"""|case Red: Red
|""".stripMargin.hover,
compat = Map(
"3.1.3-RC1" ->
">=3.1.3-RC1-bin-20220301-fae7c09-NIGHTLY" ->
"""|case Red: Color
|""".stripMargin.hover
)
Expand All @@ -81,7 +81,7 @@ class HoverScala3TypeSuite extends BaseHoverSuite {
|""".stripMargin,
"",
compat = Map(
"3.1.3-RC1" ->
">=3.1.3-RC1-bin-20220301-fae7c09-NIGHTLY" ->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we say >3.1.3-RC1 here? (this is not a strong opinion. If you prefer this way, go ahead 👍 )

  • If we write >3.1.3-RC1, the nightly releases of 3.1.3-RC1 before 20220301 will fail, but do we test against older Scala3 nightly release?
  • I feel it's too specific
  • If we can write >3.1.3-RC1, we can omit nightlyDate field from Version case class, and keep it simple :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tanishiking Initially I wanted to use just > but then realized that the actual usage of this might be gte only. Some change happens in specific version so you add a new compat case that should be used from this version and next ones.
I agree that >= is more verbose than > but I think at least it's more clear.

If we write >3.1.3-RC1, the nightly releases of 3.1.3-RC1 before 20220301 will fail, but do we test against older Scala3 nightly release?

We have a ci workflow that tests and backpublishes support of new scala3-nightly versions for the latest Metals version.
Being too specific with versions will help to stabilize this workflow as currently it often fails.

Copy link
Member

@tanishiking tanishiking Mar 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the detailed explanation!

We have a ci workflow that tests and backpublishes support of new scala3-nightly versions for the latest Metals version.
Being too specific with versions will help to stabilize this workflow as currently it often fails.

I'm convinced that being specific with versions would be better. Now, it's LGTM from my part.

"""|case Blue: Color
|""".stripMargin.hover
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ class CompletionBacktickSuite extends BaseCompletionSuite {
|""".stripMargin,
filterText = "type",
compat = Map(
"3" -> "type: Int"
"3" -> "type: Int",
">=3.2.0" -> "`type`: Int"
)
)

Expand Down
4 changes: 2 additions & 2 deletions tests/cross/src/test/scala/tests/pc/CompletionSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -949,7 +949,7 @@ class CompletionSuite extends BaseCompletionSuite {
"""|Some scala
|""".stripMargin,
compat = Map(
"3.1" ->
">=3.1.0" ->
"""|Some scala
|SomeToExpr[T: Type: ToExpr]: SomeToExpr[T]
|SomeFromExpr[T](using Type[T], FromExpr[T]): SomeFromExpr[T]
Expand All @@ -976,7 +976,7 @@ class CompletionSuite extends BaseCompletionSuite {
"""|Some scala
|""".stripMargin,
compat = Map(
"3.1" ->
">=3.1.0" ->
"""|Some scala
|SomeToExpr[T: Type: ToExpr]: SomeToExpr[T]
|SomeFromExpr[T](using Type[T], FromExpr[T]): SomeFromExpr[T]
Expand Down
7 changes: 2 additions & 5 deletions tests/mtest/src/main/scala/tests/BaseSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -86,11 +86,8 @@ abstract class BaseSuite extends munit.FunSuite with Assertions {
compat: Map[String, A],
scalaVersion: String
): A =
compat
.collect {
case (ver, compatCode) if scalaVersion.startsWith(ver) => compatCode
}
.headOption
Compat
.forScalaVersion(scalaVersion, compat)
.getOrElse(default)

protected def toJsonArray(list: List[String]): String = {
Expand Down
40 changes: 40 additions & 0 deletions tests/mtest/src/main/scala/tests/Compat.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package tests

import scala.meta.internal.semver.SemVer

object Compat {

/**
* Cases might be described as:
* - Starts with: "3.0.1" -> value OR "3.0" -> value OR "3" -> value
* - Greater or equal: ">=3.0.0" -> value
*/
def forScalaVersion[A](
scalaVersion: String,
cases: Map[String, A]
): Option[A] = {
val (startsWith, gt) =
cases.partition { case (v, _) => !v.startsWith(">=") }

val fromStartWith = startsWith.collectFirst {
case (v, a) if scalaVersion.startsWith(v) => a
}

matchesGte(scalaVersion, gt).orElse(fromStartWith)
}

private def matchesGte[A](
version: String,
gtCases: Map[String, A]
): Option[A] = {
val parsed = SemVer.Version.fromString(version)
val less = gtCases
.map { case (v, a) =>
(SemVer.Version.fromString(v.stripPrefix(">=")), a)
}
.filter { case (v, _) => parsed >= v }

less.toList.sortWith(_._1 < _._1).lastOption.map(_._2)
}

}
9 changes: 9 additions & 0 deletions tests/unit/src/test/scala/tests/ScalaVersionsSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,15 @@ class ScalaVersionsSuite extends BaseSuite {
)
}

test("compare-NIGTLY") {
assert(
SemVer.isLaterVersion(
"3.0.0-RC1-bin-20201125-1c3538a-NIGHTLY",
"3.0.0-RC1-bin-20201126-1c3538a-NIGHTLY"
)
)
}

test("not-future-3-M1") {
assert(
!ScalaVersions.isFutureVersion("3.0.0-M1")
Expand Down
35 changes: 35 additions & 0 deletions tests/unit/src/test/scala/tests/SemVerSuite.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package tests

import scala.meta.internal.semver.SemVer

import munit.FunSuite

class SemVerSuite extends FunSuite {

val expected: List[(String, SemVer.Version)] = List(
("3.0.0", SemVer.Version(3, 0, 0)),
("3.0.0-M1", SemVer.Version(3, 0, 0, milestone = Some(1))),
("3.0.0-RC1", SemVer.Version(3, 0, 0, releaseCandidate = Some(1))),
(
"3.2.0-RC1-bin-20220307-6dc591a-NIGHTLY",
SemVer.Version(
3,
2,
0,
releaseCandidate = Some(1),
nightlyDate = Some(20220307)
)
)
)

test("fromString") {
val incorrect = expected
.map { case (s, e) => (SemVer.Version.fromString(s), e) }
.filter({ case (parsed, expected) => parsed != expected })

assert(
incorrect.isEmpty,
incorrect.mkString("Failed to parse versions(expected, got):", "\n", "")
)
}
}