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 current CI flakiness with DiagnosticLspSuite and LensesLspSuite #1000

Merged
merged 2 commits into from Oct 18, 2019

Conversation

@tgodzik
Copy link
Collaborator

tgodzik commented Oct 17, 2019

No description provided.

|}
|""".stripMargin
)
// broken when being run on a fresh Bloop instance

This comment has been minimized.

Copy link
@tgodzik

tgodzik Oct 17, 2019

Author Collaborator

Working on fixing that separately

@tgodzik

This comment has been minimized.

Copy link
Collaborator Author

tgodzik commented Oct 17, 2019

Seems we have another flakines:

2019-10-17T17:34:04.9163732Z INFO  compiling b (1 scala source)
2019-10-17T17:34:05.9380001Z     """|/a/src/main/scala/a/Main.scala
2019-10-17T17:34:05.9382686Z        |package a
2019-10-17T17:34:05.9383247Z        |import java.util.concurrent.Future/*Future.java*/ // unused
2019-10-17T17:34:05.9383671Z        |import scala.util.Failure/*Try.scala*/ // unused
2019-10-17T17:34:05.9384022Z        |object Main/*MainSuite.scala:6*/ extends App/*App.scala*/ {
2019-10-17T17:34:05.9384424Z        |  val message/*L6*/ = Message/*Message.java:1*/.message/*Message.java:2*/
2019-10-17T17:34:05.9384827Z        |  new java.io.PrintStream/*PrintStream.java*/(new java.io.ByteArrayOutputStream/*ByteArrayOutputStream.java*/())
2019-10-17T17:34:05.9385252Z        |  println/*Predef.scala*/(message/*L4*/)
2019-10-17T17:34:05.9385622Z        |}
2019-10-17T17:34:05.9386050Z        |/b/src/main/scala/a/MainSuite.scala
2019-10-17T17:34:05.9386419Z        |package a
2019-10-17T17:34:05.9386770Z        |import java.util.concurrent.Future/*Future.java*/ // unused
2019-10-17T17:34:05.9387136Z        |import scala.util.Failure/*Try.scala*/ // unused
2019-10-17T17:34:05.9387502Z        |import org.scalatest.FunSuite/*FunSuite.scala*/
2019-10-17T17:34:05.9391274Z        |object MainSuite/*L4*/ extends FunSuite/*FunSuite.scala*/ {
2019-10-17T17:34:05.9392342Z        |  test/*FunSuiteLike.scala*/("a") {
2019-10-17T17:34:05.9392769Z        |    val condition/*L7*/ = Main/*Main.scala:3*/.message/*Main.scala:4*/.contains/*String.java*/("Hello")
2019-10-17T17:34:05.9393173Z        |    assert/*Assertions.scala*/(condition/*L6*/)
2019-10-17T17:34:05.9393532Z        |  }
2019-10-17T17:34:05.9393865Z        |}
2019-10-17T17:34:05.9464436Z �[31mX�[39m tests.DefinitionLspSuite.definition �[2m10609ms�[0m 
2019-10-17T17:34:05.9465594Z   �[91m�[4mtests.TestFailedException�[39m�[24m: failed assertion at /home/runner/work/metals/metals/tests/unit/src/test/scala/tests/DefinitionLspSuite.scala:71
2019-10-17T17:34:05.9466195Z failed assertion at /home/runner/work/metals/metals/tests/mtest/src/main/scala/tests/DiffAssertions.scala:24
2019-10-17T17:34:05.9466614Z ===========
2019-10-17T17:34:05.9466973Z => Obtained
2019-10-17T17:34:05.9467317Z ===========
2019-10-17T17:34:05.9467661Z     """|/a/src/main/scala/a/Main.scala
2019-10-17T17:34:05.9468025Z        |package a
2019-10-17T17:34:05.9468386Z        |import java.util.concurrent.Future/*Future.java*/ // unused
2019-10-17T17:34:05.9469321Z        |import scala.util.Failure/*Try.scala*/ // unused
2019-10-17T17:34:05.9469990Z        |object Main/*MainSuite.scala:6*/ extends App/*App.scala*/ {
2019-10-17T17:34:05.9470387Z        |  val message/*L6*/ = Message/*Message.java:1*/.message/*Message.java:2*/
2019-10-17T17:34:05.9471186Z        |  new java.io.PrintStream/*PrintStream.java*/(new java.io.ByteArrayOutputStream/*ByteArrayOutputStream.java*/())
2019-10-17T17:34:05.9471600Z        |  println/*Predef.scala*/(message/*L4*/)
2019-10-17T17:34:05.9471968Z        |}
2019-10-17T17:34:05.9472317Z        |/b/src/main/scala/a/MainSuite.scala
2019-10-17T17:34:05.9472668Z        |package a
2019-10-17T17:34:05.9473022Z        |import java.util.concurrent.Future/*Future.java*/ // unused
2019-10-17T17:34:05.9473399Z        |import scala.util.Failure/*Try.scala*/ // unused
2019-10-17T17:34:05.9473787Z        |import org.scalatest.FunSuite/*FunSuite.scala*/
2019-10-17T17:34:05.9474155Z        |object MainSuite/*L4*/ extends FunSuite/*FunSuite.scala*/ {
2019-10-17T17:34:05.9474514Z        |  test/*FunSuiteLike.scala*/("a") {
2019-10-17T17:34:05.9474915Z        |    val condition/*L7*/ = Main/*Main.scala:3*/.message/*Main.scala:4*/.contains/*String.java*/("Hello")
2019-10-17T17:34:05.9475318Z        |    assert/*Assertions.scala*/(condition/*L6*/)
2019-10-17T17:34:05.9485476Z        |  }
2019-10-17T17:34:05.9485980Z        |}
2019-10-17T17:34:05.9486268Z 
2019-10-17T17:34:05.9486574Z 
@tgodzik

This comment has been minimized.

Copy link
Collaborator Author

tgodzik commented Oct 18, 2019

Last one looks like the semanticDB document was not created and there are no references - pretty sure it's a race.

@olafurpg

This comment has been minimized.

Copy link
Member

olafurpg commented Oct 18, 2019

Indexing "find references" from *.semanticdb files relies on file watching which is inherently unreliable for stable tests. I'm fine with disabling unstable tests for now since having a green CI is more important IMO.

@tgodzik tgodzik force-pushed the tgodzik:ignore-flaky-tests branch 3 times, most recently from a828e44 to 1e96b5e Oct 18, 2019
@tgodzik

This comment has been minimized.

Copy link
Collaborator Author

tgodzik commented Oct 18, 2019

Indexing "find references" from *.semanticdb files relies on file watching which is inherently unreliable for stable tests. I'm fine with disabling unstable tests for now since having a green CI is more important IMO.

Does it make sense to search only for definitions in that suite instead of just ignoring it altogether?

The other solution I see is registering for file events inside of TestServer and waiting for those. But everything like that heavily relies on changes to metals server itself.

@tgodzik tgodzik force-pushed the tgodzik:ignore-flaky-tests branch from 1e96b5e to 78678e1 Oct 18, 2019
@tgodzik tgodzik force-pushed the tgodzik:ignore-flaky-tests branch from 78678e1 to d528dd4 Oct 18, 2019
@@ -74,8 +74,8 @@ object DefinitionLspSuite extends BaseLspSuite("definition") {
|package a
|import java.util.concurrent.Future/*Future.java*/ // unused
|import scala.util.Failure/*Try.scala*/ // unused
|object Main/*MainSuite.scala:6*/ extends App/*App.scala*/ {

This comment has been minimized.

Copy link
@olafurpg

olafurpg Oct 18, 2019

Member

I hadn't realized we were calling "find references" in this test suite. That explains why the test was flaky, "find references" triggers a background cascade compilation. Generally, any discarded Future[T] is a likely source of flaky test failures. It's a good change to only test for definitions in DefinitionLspSuite 👍

Copy link
Member

olafurpg left a comment

LGTM 👍 CI is green 🎉

@tgodzik tgodzik merged commit b5b941f into scalameta:master Oct 18, 2019
3 checks passed
3 checks passed
build
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
scalameta.metals Build #20191018.11 succeeded
Details
@tgodzik tgodzik deleted the tgodzik:ignore-flaky-tests branch Oct 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.