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

Failing test case for #1253 #1254

Merged
merged 7 commits into from Jan 5, 2020
Merged

Conversation

@gabro
Copy link
Member

gabro commented Jan 5, 2020

Following an offline conversation with @olafurpg, this is a stab at "failing tests", inspired from https://github.com/avajs/ava/blob/master/docs/01-writing-tests.md#failing-tests

gabro added 2 commits Jan 5, 2020
These are tests that are expected to fail, which can be useful to
document known bugs for which we don't have a fix yet.
@gabro gabro requested a review from olafurpg Jan 5, 2020
@@ -184,4 +184,29 @@ object CompletionMatchSuite extends BaseCompletionSuite {
"2.11" -> "match"
)
)

// https://github.com/scalameta/metals/issues/1253
checkEditBug(

This comment has been minimized.

Copy link
@gabro

gabro Jan 5, 2020

Author Member

this test passes because it fails 🙃

@@ -149,6 +149,11 @@ class BaseSuite extends TestSuite {
myTests += FlatTest(name, () => fun)
}
}

def failingTest(name: String)(fun: => Any): Unit = {
test(name)(intercept[TestFailedException](fun))

This comment has been minimized.

Copy link
@gabro

gabro Jan 5, 2020

Author Member

I'm tempted to do

    test(utest.ufansi.Color.LightRed(s"EXPECTED FAILURE - $name").toString()) {
      intercept[TestFailedException](fun)
    }

and obtain

image

but then it becomes hard to select a single test to run.

This comment has been minimized.

Copy link
@olafurpg

olafurpg Jan 5, 2020

Member

I agree it’s better to leave the test name unchanged.

Copy link
Member

olafurpg left a comment

Cool to see this idea in action. I think it needs a bit refinement to reduce the ceremony. It’s not reasonable for example to expect first time contributors to figure out how to write failing tests. What do you?

@@ -149,6 +149,11 @@ class BaseSuite extends TestSuite {
myTests += FlatTest(name, () => fun)
}
}

def failingTest(name: String)(fun: => Any): Unit = {

This comment has been minimized.

Copy link
@olafurpg

olafurpg Jan 5, 2020

Member

“testFailing”?

@@ -101,6 +103,52 @@ abstract class BaseCompletionSuite extends BasePCSuite {
}
}

def checkEdit(

This comment has been minimized.

Copy link
@olafurpg

olafurpg Jan 5, 2020

Member

This signature is so complicated it’s probably better to keep adding more parameters instead of duplicating the method. What do you think?

This comment has been minimized.

Copy link
@gabro

gabro Jan 5, 2020

Author Member

yeah, I agree. I was uneasy too with the amount of duplication

@@ -158,6 +163,15 @@ class BaseSuite extends TestSuite {
}
}

def failingAsyncTest(name: String, maxDuration: Duration = Duration("10min"))(

This comment has been minimized.

Copy link
@olafurpg

olafurpg Jan 5, 2020

Member

I think we can avoid some duplicate logic by encoding the failure bit where we collect the tests

@gabro gabro requested a review from olafurpg Jan 5, 2020
@gabro

This comment has been minimized.

Copy link
Member Author

gabro commented Jan 5, 2020

@olafurpg new attempt, which just parametrizes test and testAsync (since we rarely use them directly, it probably makes more sense this way than exposing another "primitive")

@gabro gabro force-pushed the gabro:exhaustive-match-inline-fqn branch from 4dc68a4 to a7772fb Jan 5, 2020
@gabro

This comment has been minimized.

Copy link
Member Author

gabro commented Jan 5, 2020

Here's how RenameLspSuite could be changed to "document" tests that are expected to fail c9cc928 /cc @tgodzik

@@ -144,15 +144,26 @@ class BaseSuite extends TestSuite {

def isTestSuiteEnabled: Boolean = true

def test(name: String)(fun: => Any): Unit = {
def test(name: String, expectFailure: Boolean = false)(fun: => Any): Unit = {

This comment has been minimized.

Copy link
@olafurpg

olafurpg Jan 5, 2020

Member

This is an improvement! I am not sure it's a good idea to add such a specific parameter to this signature however. I have wanted to add tag support down the road so that we can classify some tests for example as flaky. What do you think about this approach instead?

Suggested change
def test(name: String, expectFailure: Boolean = false)(fun: => Any): Unit = {
class Tag
case object ExpectFailure extends Tag
def test(name: String, tags: Tag*)(fun: => Any): Unit = {
val isExpectFailure = tags.contains(ExpectFailure)

This comment has been minimized.

Copy link
@olafurpg

olafurpg Jan 5, 2020

Member

Another design I have thought of could be to replace name: String with a custom data structure

case class TestData(name: String, isExpectedFailure: Boolean, tags: List[Tag]) {
  def isFailing: TestData = copy(isExpectedFailure = true)
  def tag(t: Tags): TestData.copy(tags = t :: tags)
}
object TestData {
  implicit def generate(name: String): TestData = TestData(name, false, Nil)
}
def test(data: TestData)(fun: => Unit): Unit = ...

The implicit conversion will make existing tests continue to work unchanged and we can more easily introduce new APIs to tweak the test metadata

test("issue-42".isFailing.tag(Flaky)) { ... }

This comment has been minimized.

Copy link
@olafurpg

olafurpg Jan 5, 2020

Member

Just validated this compiles and works as expected
Screenshot 2020-01-05 at 17 17 27

This comment has been minimized.

Copy link
@gabro

gabro Jan 5, 2020

Author Member

I like the design! I've submitted a proposal that implements it

@gabro gabro requested a review from olafurpg Jan 5, 2020
def checkEdit(
name: String,
name: TestOptions,

This comment has been minimized.

Copy link
@gabro

gabro Jan 5, 2020

Author Member

tests suites that need tagging support, can gradually make this conversion


// https://github.com/scalameta/metals/issues/1253
checkEdit(
"exhaustive-fully-qualify".expectedToFail,

This comment has been minimized.

Copy link
@gabro

gabro Jan 5, 2020

Author Member

here's an example. You could technically also do

Suggested change
"exhaustive-fully-qualify".expectedToFail,
"exhaustive-fully-qualify".tag(Tag.ExpectedFailure),

but since we'll reasonably have a few tags, I opted for a utility "DSL" method

This comment has been minimized.

Copy link
@olafurpg

olafurpg Jan 5, 2020

Member

I think it's fine to have a method here to ease discovery.

val testName = nonempty.headOption.getOrElse("<empty>")
test(testName) {
Comment on lines +9 to +10

This comment has been minimized.

Copy link
@gabro

gabro Jan 5, 2020

Author Member

this was needed in order to please the type checker, which was inferring Serializable when inlined.

This comment has been minimized.

Copy link
@olafurpg

olafurpg Jan 5, 2020

Member

What 😟 That's surprising. Looks like a compiler bug in that case. Is it worth adding a def test(name: String, tags: Tag*) overload to avoid similarly confusing failures for basic cases?

gabro added 3 commits Jan 5, 2020
Copy link
Member

olafurpg left a comment

LGTM 👍 This is great! Thank you @gabro for implementing this idea, I'm excited to try it out in practice 😁


// https://github.com/scalameta/metals/issues/1253
checkEdit(
"exhaustive-fully-qualify".expectedToFail,

This comment has been minimized.

Copy link
@olafurpg

olafurpg Jan 5, 2020

Member

I think it's fine to have a method here to ease discovery.

val testName = nonempty.headOption.getOrElse("<empty>")
test(testName) {

This comment has been minimized.

Copy link
@olafurpg

olafurpg Jan 5, 2020

Member

What 😟 That's surprising. Looks like a compiler bug in that case. Is it worth adding a def test(name: String, tags: Tag*) overload to avoid similarly confusing failures for basic cases?

@gabro

This comment has been minimized.

Copy link
Member Author

gabro commented Jan 5, 2020

Failing test is not related

@gabro gabro merged commit c816bfa into scalameta:master Jan 5, 2020
11 of 12 checks passed
11 of 12 checks passed
windows-latest jdk-11 unit tests
Details
macOS-latest jdk-11 unit tests
Details
ubuntu-latest jdk-8 unit tests ubuntu-latest jdk-8 unit tests
Details
ubuntu-latest jdk-11 unit tests
Details
Sbt integration
Details
Maven integration
Details
Gradle integration
Details
Mill integration
Details
Pants integration
Details
LSP integration tests
Details
Scala cross tests
Details
Scalafmt/Scalafix/Docs
Details
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.