Replace utest with MUnit #1277
Merged
Replace utest with MUnit #1277
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gabro
commented
Jan 9, 2020
Comment on lines
75
to
99
| override def funsuiteRunTest(options: TestOptions, body: => Any): Any = { | ||
| // We are unable to infer the JDK jars on Appveyor | ||
| // tests.BasePCSuite.indexJDK(BasePCSuite.scala:44) | ||
| val testName = | ||
| if (isCI && BuildInfo.scalaCompilerVersion != BuildInfoVersions.scala212) | ||
| s"${BuildInfo.scalaCompilerVersion}-${options.name}" | ||
| else options.name | ||
| super.test(options.copy(name = testName)) { | ||
| try { | ||
| fun | ||
| } catch { | ||
| case NonFatal(e) | ||
| if e.getMessage != null && | ||
| e.getMessage.contains("x$1") && | ||
| !hasJdkSources => | ||
| // ignore failing test if jdk sources are missing | ||
| () | ||
| super.funsuiteRunTest( | ||
| options.copy(name = testName), { | ||
| try { | ||
| body | ||
| } catch { | ||
| case NonFatal(e) | ||
| if e.getMessage != null && | ||
| e.getMessage.contains("x$1") && | ||
| !hasJdkSources => | ||
| // ignore failing test if jdk sources are missing | ||
| () | ||
| } | ||
| } | ||
| } | ||
| ) | ||
| } |
There was a problem hiding this comment.
since we're at it, is this still true for Windows on GitHub Actions? The comment mentions Appveyor
There was a problem hiding this comment.
From what I remember sources are available on Github Actions.
Comment on lines
9
to
28
| def unifiedDiff(expected: String, obtained: String): String = { | ||
| def splitIntoLines(string: String): Seq[String] = | ||
| string.trim.replace("\r\n", "\n").split("\n") | ||
|
|
||
| val references = splitIntoLines(expected).asJava | ||
| val definition = splitIntoLines(obtained).asJava | ||
| val diff = difflib.DiffUtils.diff(references, definition) | ||
| difflib.DiffUtils | ||
| .generateUnifiedDiff( | ||
| "references", | ||
| "definition", | ||
| references, | ||
| diff, | ||
| 1 | ||
| ) | ||
| .asScala | ||
| .iterator | ||
| .filterNot(_.startsWith("@@")) | ||
| .mkString("\n") | ||
| } |
There was a problem hiding this comment.
This exists almost verbatim in funsuite, but it's a private method
There was a problem hiding this comment.
We can make it public in FunSuite
Comment on lines
30
to
14
| def assertNoDiffOrPrintObtained( | ||
| obtained: String, | ||
| expected: String | ||
| )(implicit loc: Location): Unit = { | ||
| // FIXME(gabro): surface this in funsuite Assertions | ||
| funsuite.internal.Diffs.assertNoDiffOrPrintExpected(obtained, expected) | ||
| } |
| } | ||
| override def funsuiteTests(): Seq[Test] = { | ||
| if (skipSuite) Seq.empty | ||
| else super.funsuiteTests() |
There was a problem hiding this comment.
The skipSuite utility may end up in funsuite (as discussed with @olafurpg)
| cancelServer() | ||
| if (context.test.tags.contains(Ignore)) return | ||
| newServer(context.test.name) | ||
| } |
There was a problem hiding this comment.
moved cancelServer and newServer to separate methods to that we can invoke them without having to invoke beforeEach
| } yield () | ||
| } | ||
| testAsync("completion") { | ||
| assume(!isWindows, "This test is flaky on Windows") |
There was a problem hiding this comment.
assume saves us from indentation
| super.testAsync(options, maxDuration)(run) | ||
| } | ||
| } | ||
| override def skipSuite: Boolean = isWindows |
There was a problem hiding this comment.
using skipSuite is way more straightforward than overriding testAsync
Comment on lines
55
to
59
| // FIXME(gabro): intercept is not available in FunSuite | ||
| // // check that we get an exception using the default nio method | ||
| // intercept[FileAlreadyExistsException] { | ||
| // Files.createDirectories(symlinkPluginsPath) | ||
| // } |
There was a problem hiding this comment.
This is the assertion I had to disable, because intercept is missing in funsuite
| @@ -182,8 +182,7 @@ object DefinitionLspSuite extends BaseLspSuite("definition") { | |||
| // requires org.scalamacros:macroparadise and io.spire-match:kind-projector. | |||
| // Navigation continues to mostly work, except for areas that have compilation | |||
| // errors. | |||
| // NOTE(olafur): ignored because this test case is flaky and regularly failing the CI. | |||
| ignore("missing-compiler-plugin") { | |||
| test("missing-compiler-plugin".flaky) { | |||
There was a problem hiding this comment.
This re-enables the test, but marks it as flaky
olafurpg
approved these changes
Jan 9, 2020
| "true".equalsIgnoreCase(System.getenv("CI")) | ||
| def beforeAll(): Unit = () | ||
| def afterAll(): Unit = () | ||
| def intercept[T: ClassTag](exprs: Unit): T = macro Asserts.interceptProxy[T] |
There was a problem hiding this comment.
should we override def isFlakyFailureOk = isCI?
There was a problem hiding this comment.
BTW, we should be able to implement a basic intercept assertion that's not macro based
def intercept[T <: Throwable]body: => Unit)(implicit ev: ClassTag[T]): T = {
try {
body
fail("expected exception but body evaluated successfully")
}
catch {
case NonFatal(e) =>
if (!ev.isAssignableFrom(e)) fail("expected ${ev.runtimeClass}, obtained ${e.getClass}")
}There was a problem hiding this comment.
I'm torn about this. What's the benefit of marking flaky tests and then ignoring them vs just ignoring them? Or a bit further: why don't we just remove those tests?
There was a problem hiding this comment.
as per intercept, I've added it to unblock this PR, although I think it would make sense in funsuite, so that it's properly tested
| } | ||
| override def funsuiteTests(): Seq[Test] = { | ||
| if (skipSuite) Seq.empty | ||
| else super.funsuiteTests() | ||
| } | ||
|
|
||
| def testAsync( |
There was a problem hiding this comment.
I have considered adding built-in support in FunSuite that captures Future[_] values and automatically awaits then with a default timeout, which would avoid testAsync
|
Tests are failing with By the looks of it, I would say it's an issue with Java 11, but I don't know how I could have possibly have introduced it. |
|
Ok, it's exactly scalacenter/bloop#743 but again, I don't know why it's popping up now |
|
I updated this PR to the latest version of FunSuite, which is now called "MUnit" and has been moved to the scalameta organization https://github.com/scalameta/munit Let's see if CI complains again about the same cryptic error 🤔 |
|
The new assertion errors look nice :) |
|
@gabro The tests are failing with the same error. I'm able to reproduce locally I believe that test case should not be running in JDK 11 because Scala 2.11 doesn't properly run in JDK 11. |
|
This should fix the issue commit ca55f62794c8d53906ea2a82302a434f3e0abc98
Author: Olafur Pall Geirsson <lgeirsson@twitter.com>
Date: Sat Jan 11 22:30:29 2020 +0000
Flip "skipSuite" condition
diff --git a/tests/unit/src/main/scala/tests/BaseWorksheetLspSuite.scala b/tests/unit/src/main/scala/tests/BaseWorksheetLspSuite.scala
index cc6d173..c8d33ca 100644
--- a/tests/unit/src/main/scala/tests/BaseWorksheetLspSuite.scala
+++ b/tests/unit/src/main/scala/tests/BaseWorksheetLspSuite.scala
@@ -13,7 +13,7 @@ abstract class BaseWorksheetLspSuite(scalaVersion: String)
override def userConfig: UserConfiguration =
super.userConfig.copy(worksheetScreenWidth = 40, worksheetCancelTimeout = 1)
- override def skipSuite: Boolean = isValidScalaVersionForEnv(scalaVersion)
+ override def skipSuite: Boolean = !isValidScalaVersionForEnv(scalaVersion)
testAsync("completion") {
assume(!isWindows, "This test is flaky on Windows")
|
olafurpg
force-pushed
the
funsuite
branch
2 times, most recently
from
9465b9d
to
a6864c3
Compare
gabro
commented
Jan 16, 2020
| if e.getMessage != null && | ||
| e.getMessage.contains("x$1") && | ||
| !hasJdkSources => | ||
| // ignore failing test if jdk sources are missing |
| expected: String | ||
| )(implicit loc: Location): Unit = { | ||
| // FIXME(gabro): surface this in funsuite Assertions | ||
| munit.Diffs.assertNoDiff(obtained, expected) |
There was a problem hiding this comment.
Does this print the obtained when it fails?
There was a problem hiding this comment.
Yes. That’s the default behavior
There was a problem hiding this comment.
cool, than we can just remove this method and use assertNoDiff directly
| } | ||
| def assertEmpty(string: String): Unit = { | ||
| if (!string.isEmpty) { | ||
| def intercept[T <: Throwable]( |
| () => () | ||
| ) | ||
| } | ||
| def skipSuite: Boolean = false |
There was a problem hiding this comment.
replace with munitIgnore (which is now in munit)
|
Upgraded to the latest MUnit and rebased. Let's see if the CI is happy! |
gabro
force-pushed
the
funsuite
branch
2 times, most recently
from
d6fb906
to
26131c6
Compare
The latest MUnit release includes fixes for handling ANSI colored strings.
The test failed in 2.11 with
```diff
=> Diff (- obtained, + expected)
x match {
-\tcase Some(x) =>
+\tcase Some(value) =>
```
Previously, before MUnit, we ran the "2.11" test on 2.13 due to a
copy-paste bug.
|
This test failure is happening consistently in CI but I'm unable to reproduce locally (tried both JDK 8 and JDK 11) Need to look closer, but I can't figure out why this PR should cause the failure. |
|
I see this happening locally when running the entire suite and not just the single test. Sometimes match-211 fails as well 🤔 |
|
Interesting 🤔 I'll try that locally. I BTW removed |
added 4 commits
January 25, 2020 13:43
We should try to eliminate all flaky tests. These tests are causing more problems than they're worth so it's better to ignore their failures than fail the build.
I'm unable to reproduce the CI test failures.
Previously, this test passed because the old assertNoDiff implementation trimmed lines that had only whitespace.
|
CI is green now! An important change is that failures in tests that are marked with In the future, I'd like to start collecting statistics for how frequently flaky tests are failing so that we can take action to fix flaky test failures. For example, it would be good to know if test is failing only 1% of the time of 99% of the time. |
olafurpg
approved these changes
Jan 25, 2020
gabro
commented
Jan 26, 2020
added 2 commits
January 26, 2020 10:28
It's better to run the tests instead of ignoring them.
|
@gabro Great suggestions! Done. |
tgodzik
approved these changes
Jan 27, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously we used a heavily customized
BaseSuiteon top of utest. We didn't really use any feature of utest, except some pretty printing utility for the stack traces.@olafurpg recently created https://github.com/olafurpg/funsuite which (not coincidentally) is extremely similar to our custom
BaseSuite, and it provides some extra goodies that help in simplifying our suites.This PR:
.flaky,.failandassume(...)to simplify our test suites(implicit loc Location)to all our customcheck...andassert...methods in order to benefit from the fancy pretty printing in funsuiteStill missing:
interceptfrom utest, which is currently missing from funsuiteassertNoDiffOrShowObtainedandunifiedDiff) that are present in funsuite but are not exposed by the public API (and I think they should)Main benefits of this PR: