-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add IDE tests #3766
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
Add IDE tests #3766
Conversation
.drone.yml
Outdated
group: test | ||
image: lampepfl/dotty:2017-11-17 | ||
commands: | ||
- cp -R . /tmp/3/ && cd /tmp/3/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be a unique directory
f9185a4
to
315f0f8
Compare
f159168
to
55175f0
Compare
134878d
to
0f93ed8
Compare
0f93ed8
to
204f82e
Compare
4411728
to
e2e0846
Compare
@@ -59,6 +59,13 @@ object DiffUtil { | |||
(fnd, exp, totalChange.toDouble / (expected.length + found.length)) | |||
} | |||
|
|||
def mkColoredLineDiff(expected: Seq[String], actual: Seq[String]): String = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add some documentation?
|
||
class CompletionTest { | ||
|
||
@Test def competion0: Unit = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: competion -> completion
|
||
import dotty.tools.languageserver.util.embedded._ | ||
|
||
object Code { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file, and every other file in this directory would benefit from some documentation.
|
||
ai.next() match { | ||
case emb: CodeMarker => | ||
positions += Tuple3(emb, line, char) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use += ((a, b, c))
var char = 0 | ||
def scan(str: String): Unit = { | ||
for (c <- str) | ||
if (c == '\n') { line += 1; char = 0 } else { char += 1 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this just line = str.count(_ == '\n'); char = str.length - line
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not exactly, char
needs to be set to 0 when we encounter \n
|
||
override def onMarker(marker: CodeMarker): Exec[Unit] = { | ||
val res = server.definition(fix(marker.toTextDocumentPositionParams)).get() | ||
assert(res.size() == refOpt.size, res) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
size()
-> size
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
res
is a java collection, hence it requires size()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was complaining or waring at the time. I will double check.
import dotty.tools.languageserver.util._ | ||
import dotty.tools.languageserver.util.embedded.CodeMarker | ||
|
||
class CodeDefinition(val range: CodeRange, refOpt: Option[CodeRange]) extends ActionOnRange { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can return more than one definition.
|
||
def to(other: CodeMarker): CodeRange = CodeRange(this, other) | ||
|
||
def file: PosCtx[TestFile] = implicitly[PositionContext].positionOf(this)._1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could add implicit def posCtx(implicit ctx: PositionContext): PositionContext = ctx
in the class to be able to do posCtx.positionOf ...
| "compilerVersion" : "0.6.0-bin-SNAPSHOT-nonbootstrapped", | ||
| "compilerArguments" : [ "-feature", "-deprecation", "-unchecked", "-Xfatal-warnings", "-encoding", "UTF8", "-language:existentials,higherKinds,implicitConversions" ], | ||
| "sourceDirectories" : [ "$baseDir/out/ide-tests/src" ], | ||
| "dependencyClasspath" : [ "$baseDir/library/../out/bootstrap/dotty-library-bootstrapped/scala-0.7/classes", "$ivyDir/cache/org.scala-lang/scala-library/jars/scala-library-2.12.4.jar" ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded version number should be fixed before this is merged.
import dotty.tools.languageserver.util.PositionContext._ | ||
import org.eclipse.lsp4j._ | ||
|
||
class SymInfo(name: String, kind: String, range: CodeRange, container: String) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use SymbolKind
for kind
instead of String
?
Java methods can be called without parenthesis.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great!
@@ -21,8 +21,8 @@ class CodeCompletion(override val marker: CodeMarker, | |||
|
|||
override def execute(): Exec[Unit] = { | |||
val result = server.completion(marker.toTextDocumentPositionParams).get() | |||
assert(result.isRight, result) | |||
assert(!result.getRight.isIncomplete, s"Completion results were 'incomplete': $result") | |||
assertTrue(s"Completion results where not 'right': $result", result.isRight) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: where -> where
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-> were
* | ||
* @see dotty.tools.languageserver.util.actions.CodeHover | ||
*/ | ||
def hover(range: CodeRange, expected: String): CodeTester = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method and the following ones could return this.type
instead of CodeTester
, that way you don't have to document @return
, it's obvious.
@nicolasstucki Do you want to add something? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes from @Duhemm LGTM
@Duhemm Can you please merge |
While working on the IDE tests, I noticed that the Dotty IDE didn't behave as I expected in the following scenario: // Rename `Foo` to `Bar` class Foo { new Foo } The Dotty IDE would consider that 3 places need changing, instead of only 2: - The `Ident(Foo)` in the `TypeDef` - The `Ident(Foo)` in the constructor - The `Ident(Foo)` in `Select(new Foo, <init>)` The third tree, however is synthetic: it doesn't need to be changed in the editor. Trees whose positions are not derived from the source are now excluded when doing a rename.
No description provided.