Address review comments

Duhemm · Duhemm · commit 0aef2d2f57c6 · 2018-04-12T19:04:34.000+02:00
diff --git a/language-server/test/dotty/tools/languageserver/RenameTest.scala b/language-server/test/dotty/tools/languageserver/RenameTest.scala
@@ -9,7 +9,7 @@ class RenameTest {
 
   @Test def rename0: Unit = {
     def testRenameFrom(m: CodeMarker) =
-      code"class ${m1}Foo$m2 { new ${m3}Foo$m4 }".withSource.rename(m, "Bar", List(m1 to m2, m3 to m4))
+      code"class ${m1}Foo$m2 { new ${m3}Foo$m4 }".withSource.rename(m, "Bar", Set(m1 to m2, m3 to m4))
     testRenameFrom(m1)
     testRenameFrom(m3)
   }
@@ -20,7 +20,7 @@ class RenameTest {
       withSources(
         code"class ${m1}Foo$m2 { new ${m3}Foo$m4 }",
         code"class Bar { new ${m5}Foo$m6 }"
-      ).rename(m, "Bar", List(m1 to m2, m3 to m4, m5 to m6))
+      ).rename(m, "Bar", Set(m1 to m2, m3 to m4, m5 to m6))
 
     testRenameFrom(m1)
     testRenameFrom(m3)
diff --git a/language-server/test/dotty/tools/languageserver/util/CodeTester.scala b/language-server/test/dotty/tools/languageserver/util/CodeTester.scala
@@ -90,12 +90,12 @@ class CodeTester(sources: List[SourceWithPositions], actions: List[Action]) {
    *
    * @param marker   The position from which to ask for renaming.
    * @param newName  The new name to give to the symbol.
-   * @param expected The expected list of positions to change.
+   * @param expected The expected positions to change.
    * @return This `CodeTester` after performing the action.
    *
    * @see dotty.tools.languageserver.util.actions.CodeRename
    */
-  def rename(marker: CodeMarker, newName: String, expected: List[CodeRange]): CodeTester =
+  def rename(marker: CodeMarker, newName: String, expected: Set[CodeRange]): CodeTester =
     doAction(new CodeRename(marker, newName, expected)) // TODO apply changes to the sources and positions
 
   /**
diff --git a/language-server/test/dotty/tools/languageserver/util/actions/Action.scala b/language-server/test/dotty/tools/languageserver/util/actions/Action.scala
@@ -22,8 +22,4 @@ trait Action {
   /** The server that this action targets. */
   def server: Exec[DottyLanguageServer] = implicitly[TestServer].server
 
-  // FIXME
-  // Workaroud an issue with implicit functions and phantomArgLift.
-  // That phase will dispear which should fix this issue. Just remove calls to param when that hapens
-  protected def fix[T](f: PositionContext => T): PosCtx[T] = f(implicitly[PositionContext])
 }
diff --git a/language-server/test/dotty/tools/languageserver/util/actions/CodeCompletion.scala b/language-server/test/dotty/tools/languageserver/util/actions/CodeCompletion.scala
@@ -5,6 +5,7 @@ import dotty.tools.languageserver.util.embedded.CodeMarker
 import dotty.tools.languageserver.util.server.TestFile
 
 import org.eclipse.lsp4j.CompletionItemKind
+import org.junit.Assert.assertEquals
 
 import scala.collection.JavaConverters._
 
@@ -25,6 +26,7 @@ class CodeCompletion(override val marker: CodeMarker,
     val completionResults = result.getRight.getItems.asScala.toSet.map { item =>
       (item.getLabel, item.getKind, item.getDetail)
     }
+    assertEquals(expected, completionResults)
   }
 
   override def show: PositionContext.PosCtx[String] =
diff --git a/language-server/test/dotty/tools/languageserver/util/actions/CodeDefinition.scala b/language-server/test/dotty/tools/languageserver/util/actions/CodeDefinition.scala
@@ -5,6 +5,8 @@ import dotty.tools.languageserver.util.embedded.CodeMarker
 
 import scala.collection.JavaConverters._
 
+import org.junit.Assert.assertEquals
+
 /**
  * An action requesting for the definition of the symbol inside `range`.
  * This action corresponds to the `textDocument/definition` method of the Language Server Protocol.
@@ -15,12 +17,10 @@ import scala.collection.JavaConverters._
 class CodeDefinition(override val range: CodeRange, expected: Seq[CodeRange]) extends ActionOnRange {
 
   override def onMarker(marker: CodeMarker): Exec[Unit] = {
-    val results = server.definition(fix(marker.toTextDocumentPositionParams)).get()
-    assert(results.size == expected.size, s"Expected ${expected.size} matches, found ${results.size}")
-    results.asScala.zip(expected).foreach {
-      case (result, expected) =>
-        assert(result == expected.toLocation, s"Expected ${expected.toLocation}, found $result.")
-    }
+    val results = server.definition(marker.toTextDocumentPositionParams).get().asScala.toSeq
+    val expectedLocations = expected.map(_.toLocation)
+
+    assertEquals(expectedLocations, results)
   }
 
   override def show: PositionContext.PosCtx[String] =
diff --git a/language-server/test/dotty/tools/languageserver/util/actions/CodeDocumentHighlight.scala b/language-server/test/dotty/tools/languageserver/util/actions/CodeDocumentHighlight.scala
@@ -3,6 +3,7 @@ package dotty.tools.languageserver.util.actions
 import dotty.tools.languageserver.util.embedded.CodeMarker
 import dotty.tools.languageserver.util.{CodeRange, PositionContext}
 import org.eclipse.lsp4j._
+import org.junit.Assert.assertEquals
 
 import scala.collection.JavaConverters._
 
@@ -19,14 +20,11 @@ class CodeDocumentHighlight(override val range: CodeRange,
                             expected: Seq[(CodeRange, DocumentHighlightKind)]) extends ActionOnRange {
 
   override def onMarker(marker: CodeMarker): Exec[Unit] = {
-    val (references, kinds) = expected.unzip
-    val results = server.documentHighlight(fix(marker.toTextDocumentPositionParams)).get()
-    assert(results.size() == references.size, results)
-    assert(references.size == kinds.length, results)
-    results.asScala.zip(references).zip(kinds).foreach { case ((dhl, ref), kind) =>
-      assert(dhl.getKind == kind, results)
-      assert(dhl.getRange == ref.toRange, results)
-    }
+    val expectedPairs = expected.map { case (codeRange, kind) => (codeRange.toRange, kind) }
+    val results = server.documentHighlight(marker.toTextDocumentPositionParams).get()
+    val resultPairs = results.asScala.map { result => (result.getRange, result.getKind) }
+
+    assertEquals(expectedPairs, resultPairs)
   }
 
   override def show: PositionContext.PosCtx[String] = {
diff --git a/language-server/test/dotty/tools/languageserver/util/actions/CodeDocumentSymbol.scala b/language-server/test/dotty/tools/languageserver/util/actions/CodeDocumentSymbol.scala
@@ -2,6 +2,7 @@ package dotty.tools.languageserver.util.actions
 
 import dotty.tools.languageserver.util.embedded.CodeMarker
 import dotty.tools.languageserver.util.{PositionContext, SymInfo}
+import org.junit.Assert.assertEquals
 
 import scala.collection.JavaConverters._
 
@@ -16,11 +17,10 @@ import scala.collection.JavaConverters._
 class CodeDocumentSymbol(override val marker: CodeMarker, expected: Seq[SymInfo]) extends ActionOnMarker {
 
   override def execute(): Exec[Unit] = {
-    val results = server.documentSymbol(marker.toDocumentSymbolParams).get()
-    assert(results.size == expected.size, results)
-    for ((symInfo, expected) <- results.asScala.zip(expected)) {
-      assert(symInfo == expected.toSymInformation, results)
-    }
+    val results = server.documentSymbol(marker.toDocumentSymbolParams).get().asScala
+    val expectedSymInfos = expected.map(_.toSymInformation)
+
+    assertEquals(expectedSymInfos, results)
   }
 
   override def show: PositionContext.PosCtx[String] =
diff --git a/language-server/test/dotty/tools/languageserver/util/actions/CodeHover.scala b/language-server/test/dotty/tools/languageserver/util/actions/CodeHover.scala
@@ -4,6 +4,7 @@ import dotty.tools.languageserver.util.embedded.CodeMarker
 import dotty.tools.languageserver.util.{CodeRange, PositionContext}
 
 import org.eclipse.lsp4j._
+import org.junit.Assert.{assertEquals, assertNull, assertTrue}
 
 import PositionContext._
 
@@ -17,14 +18,14 @@ import PositionContext._
 class CodeHover(override val range: CodeRange, expected: String) extends ActionOnRange {
 
   override def onMarker(marker: CodeMarker): Exec[Unit] = {
-    val result = server.hover(fix(marker.toTextDocumentPositionParams)).get()
-    assert(result.getRange == null)
-    if (expected == "") assert(result.getContents == null, "Expected null contents in " + result)
+    val result = server.hover(marker.toTextDocumentPositionParams).get()
+    assertNull(result.getRange)
+    if (expected.isEmpty) assertNull(result.getContents)
     else {
-      assert(result.getContents.size() == 1, result)
+      assertEquals(1, result.getContents.size)
       val content = result.getContents.get(0)
-      assert(content.isLeft, "Expected left but was " + content)
-      assert(content.getLeft == expected, s"Expected $expected but was ${content.getLeft}")
+      assertTrue(content.isLeft)
+      assertEquals(expected, content.getLeft)
     }
   }
 
diff --git a/language-server/test/dotty/tools/languageserver/util/actions/CodeReferences.scala b/language-server/test/dotty/tools/languageserver/util/actions/CodeReferences.scala
@@ -3,6 +3,8 @@ package dotty.tools.languageserver.util.actions
 import dotty.tools.languageserver.util.embedded.CodeMarker
 import dotty.tools.languageserver.util.{CodeRange, PositionContext}
 
+import org.junit.Assert.assertEquals
+
 import scala.collection.JavaConverters._
 
 /**
@@ -18,9 +20,10 @@ class CodeReferences(override val range: CodeRange,
                      withDecl: Boolean) extends ActionOnRange {
 
   override def onMarker(marker: CodeMarker): Exec[Unit] = {
-    val results = server.references(fix(marker.toReferenceParams(withDecl))).get()
-    assert(results.size() == expected.size)
-    results.asScala.zip(expected).foreach { case (loc, ref) => assert(loc == ref.toLocation, results) }
+    val expectedLocations = expected.map(_.toLocation)
+    val results = server.references(marker.toReferenceParams(withDecl)).get().asScala
+
+    assertEquals(expectedLocations, results)
   }
 
   override def show: PositionContext.PosCtx[String] =
diff --git a/language-server/test/dotty/tools/languageserver/util/actions/CodeRename.scala b/language-server/test/dotty/tools/languageserver/util/actions/CodeRename.scala
@@ -3,6 +3,8 @@ package dotty.tools.languageserver.util.actions
 import dotty.tools.languageserver.util.embedded.CodeMarker
 import dotty.tools.languageserver.util.{CodeRange, PositionContext}
 
+import org.junit.Assert.{assertEquals, assertNull}
+
 import scala.collection.JavaConverters._
 
 /**
@@ -15,18 +17,15 @@ import scala.collection.JavaConverters._
  */
 class CodeRename(override val marker: CodeMarker,
                  newName: String,
-                 expected: List[CodeRange]) extends ActionOnMarker {
+                 expected: Set[CodeRange]) extends ActionOnMarker {
 
   override def execute(): Exec[Unit] = {
     val results = server.rename(marker.toRenameParams(newName)).get()
-    assert(results.getDocumentChanges == null, results)
-    val editItems = results.getChanges.values().asScala.flatMap(_.asScala) // TODO use a Map
-    assert(expected.forall { exp =>
-      editItems.exists { editItem =>
-        editItem.getNewText == newName &&
-        editItem.getRange == exp.toRange
-      }
-    }, results)
+    val changes = results.getChanges.asScala.mapValues(_.asScala.toSet.map(ch => (ch.getNewText, ch.getRange)))
+    val expectedChanges = expected.groupBy(_.file.uri).mapValues(_.map(range => (newName, range.toRange)))
+
+    assertNull(results.getDocumentChanges)
+    assertEquals(expectedChanges, changes)
   }
 
   override def show: PositionContext.PosCtx[String] =
diff --git a/language-server/test/dotty/tools/languageserver/util/actions/CodeSymbol.scala b/language-server/test/dotty/tools/languageserver/util/actions/CodeSymbol.scala
@@ -1,7 +1,8 @@
 package dotty.tools.languageserver.util.actions
 
-import dotty.tools.languageserver.util.{CodeRange, PositionContext, SymInfo}
-import org.eclipse.lsp4j._
+import dotty.tools.languageserver.util.{PositionContext, SymInfo}
+import org.eclipse.lsp4j.WorkspaceSymbolParams
+import org.junit.Assert.assertEquals
 
 import scala.collection.JavaConverters._
 
@@ -12,16 +13,15 @@ import scala.collection.JavaConverters._
  * @param query    The string to query for.
  * @param expected The expected results.
  */
-class CodeSymbol(query: String, symbols: Seq[SymInfo]) extends Action {
+class CodeSymbol(query: String, expected: Seq[SymInfo]) extends Action {
 
   override def execute(): Exec[Unit] = {
-    val results = server.symbol(new WorkspaceSymbolParams(query)).get()
-    assert(results.size() == symbols.size, results)
-    for ((symInfo, expected) <- results.asScala.zip(symbols)) {
-      assert(symInfo == expected.toSymInformation, results)
-    }
+    val results = server.symbol(new WorkspaceSymbolParams(query)).get().asScala
+    val expectedSymInfo = expected.map(_.toSymInformation)
+
+    assertEquals(expectedSymInfo, results)
   }
 
   override def show: PositionContext.PosCtx[String] =
-    s"CodeDocumentSymbol($query, ${symbols.map(_.show)})"
+    s"CodeDocumentSymbol($query, ${expected.map(_.show)})"
 }
diff --git a/language-server/test/dotty/tools/languageserver/util/server/TestServer.scala b/language-server/test/dotty/tools/languageserver/util/server/TestServer.scala
@@ -12,32 +12,39 @@ import scala.collection.JavaConverters._
 
 class TestServer(testFolder: Path) {
 
-  // Fill the configuration with values populated by sbt
-  private def showSeq[T](lst: Seq[T]): String = lst.map(elem => '"' + elem.toString + '"').mkString("[ ", ", ", " ]")
-  private val dottyIdeJson: String =
-    s"""[ {
-       |  "id" : "dotty-ide-test",
-       |  "compilerVersion" : "${BuildInfo.ideTestsCompilerVersion}",
-       |  "compilerArguments" : ${showSeq(BuildInfo.ideTestsCompilerArguments)},
-       |  "sourceDirectories" : ${showSeq(BuildInfo.ideTestsSourceDirectories)},
-       |  "dependencyClasspath" : ${showSeq(BuildInfo.ideTestsDependencyClasspath)},
-       |  "classDirectory" : "${BuildInfo.ideTestsClassDirectory}"
-       |}
-       |]
-    """.stripMargin
-  private val configFile = testFolder.resolve(DottyLanguageServer.IDE_CONFIG_FILE)
-  testFolder.toFile.mkdirs()
-  testFolder.resolve("src").toFile.mkdirs()
-  testFolder.resolve("out").toFile.mkdirs()
-  new PrintWriter(configFile.toString) { write(dottyIdeJson); close() }
-
   val server = new DottyLanguageServer
-  private val client = new TestClient
-  server.connect(client)
+  init()
+
+  private[this] def init(): InitializeResult = {
+    // Fill the configuration with values populated by sbt
+    def showSeq[T](lst: Seq[T]): String = lst.map(elem => '"' + elem.toString + '"').mkString("[ ", ", ", " ]")
+    val dottyIdeJson: String =
+      s"""[ {
+         |  "id" : "dotty-ide-test",
+         |  "compilerVersion" : "${BuildInfo.ideTestsCompilerVersion}",
+         |  "compilerArguments" : ${showSeq(BuildInfo.ideTestsCompilerArguments)},
+         |  "sourceDirectories" : ${showSeq(BuildInfo.ideTestsSourceDirectories)},
+         |  "dependencyClasspath" : ${showSeq(BuildInfo.ideTestsDependencyClasspath)},
+         |  "classDirectory" : "${BuildInfo.ideTestsClassDirectory}"
+         |}
+         |]""".stripMargin
+    val configFile = testFolder.resolve(DottyLanguageServer.IDE_CONFIG_FILE)
+    testFolder.toFile.mkdirs()
+    testFolder.resolve("src").toFile.mkdirs()
+    testFolder.resolve("out").toFile.mkdirs()
+
+    new PrintWriter(configFile.toString) {
+      write(dottyIdeJson)
+      close()
+    }
 
-  private val initParams = new InitializeParams()
-  initParams.setRootUri(testFolder.toAbsolutePath.toUri.toString)
-  server.initialize(initParams).get()
+    val client = new TestClient
+    server.connect(client)
+
+    val initParams = new InitializeParams()
+    initParams.setRootUri(testFolder.toAbsolutePath.toUri.toString)
+    server.initialize(initParams).get()
+  }
 
   /** Open the code in the given file and returns the file.
    *  @param code code in file

Original file line number	Diff line number	Diff line change
`@@ -90,12 +90,12 @@ class CodeTester(sources: List[SourceWithPositions], actions: List[Action]) {`
`90`	`90`	`*`
`91`	`91`	`* @param marker The position from which to ask for renaming.`
`92`	`92`	`* @param newName The new name to give to the symbol.`
`93`		`- * @param expected The expected list of positions to change.`
	`93`	`+ * @param expected The expected positions to change.`
`94`	`94`	* @return This `CodeTester` after performing the action.
`95`	`95`	`*`
`96`	`96`	`* @see dotty.tools.languageserver.util.actions.CodeRename`
`97`	`97`	`*/`
`98`		`- def rename(marker: CodeMarker, newName: String, expected: List[CodeRange]): CodeTester =`
	`98`	`+ def rename(marker: CodeMarker, newName: String, expected: Set[CodeRange]): CodeTester =`
`99`	`99`	`doAction(new CodeRename(marker, newName, expected)) // TODO apply changes to the sources and positions`
`100`	`100`
`101`	`101`	`/**`
Original file line number	Diff line number	Diff line change
`@@ -22,8 +22,4 @@ trait Action {`
`22`	`22`	`/** The server that this action targets. */`
`23`	`23`	`def server: Exec[DottyLanguageServer] = implicitly[TestServer].server`
`24`	`24`
`25`		`- // FIXME`
`26`		`- // Workaroud an issue with implicit functions and phantomArgLift.`
`27`		`- // That phase will dispear which should fix this issue. Just remove calls to param when that hapens`
`28`		`- protected def fix[T](f: PositionContext => T): PosCtx[T] = f(implicitly[PositionContext])`
`29`	`25`	`}`
Original file line number	Diff line number	Diff line change
`@@ -5,6 +5,7 @@ import dotty.tools.languageserver.util.embedded.CodeMarker`
`5`	`5`	`import dotty.tools.languageserver.util.server.TestFile`
`6`	`6`
`7`	`7`	`import org.eclipse.lsp4j.CompletionItemKind`
	`8`	`+import org.junit.Assert.assertEquals`
`8`	`9`
`9`	`10`	`import scala.collection.JavaConverters._`
`10`	`11`
`@@ -25,6 +26,7 @@ class CodeCompletion(override val marker: CodeMarker,`
`25`	`26`	`val completionResults = result.getRight.getItems.asScala.toSet.map { item =>`
`26`	`27`	`(item.getLabel, item.getKind, item.getDetail)`
`27`	`28`	`}`
	`29`	`+ assertEquals(expected, completionResults)`
`28`	`30`	`}`
`29`	`31`
`30`	`32`	`override def show: PositionContext.PosCtx[String] =`