Skip to content

Commit

Permalink
Cleanup Testing & Retry logic (#3477)
Browse files Browse the repository at this point in the history
* Consolidate all Scala-level tries into one `mill.api.Retry` helper,
add some minimal unit tests, and use it throughout Mill
* Swap out `lambdatest` for `junit` so we can use the standard JUnit
retry facilities on `FileToStreamTailerTest` (nothing built in, but
googling brings up lots of examples to copy-paste)
* Remove `os.temp.dir` from Integration tester `workspacePath` and
replace it with a shorter `run-$i` folder that serves the same purpose
* Try to be more aggressive at cleaning up prior `out/` folder to force
the processes to terminate
  • Loading branch information
lihaoyi authored Sep 8, 2024
1 parent 2e187bf commit 922d4ef
Show file tree
Hide file tree
Showing 22 changed files with 430 additions and 266 deletions.
4 changes: 2 additions & 2 deletions .github/workflows/publish-artifacts.yml
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ jobs:

- uses: actions/setup-java@v4
with:
java-version: 11
java-version: '11'
distribution: temurin

- run: ci/release-maven.sh
Expand All @@ -73,7 +73,7 @@ jobs:

- uses: actions/setup-java@v4
with:
java-version: 11
java-version: '11'
distribution: temurin

- run: ./mill -i uploadToGithub $REPO_ACCESS_TOKEN
2 changes: 1 addition & 1 deletion .github/workflows/publish-bridges.yml
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ jobs:

- uses: actions/setup-java@v4
with:
java-version: 8
java-version: '11'
distribution: temurin

- run: ci/release-bridge-maven.sh
2 changes: 1 addition & 1 deletion .github/workflows/publish-docs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ jobs:

- uses: actions/setup-java@v4
with:
java-version: 8
java-version: '11'
distribution: temurin

- run: ci/publish-docs.sh
5 changes: 3 additions & 2 deletions .github/workflows/run-mill-action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,12 @@ jobs:
if: inputs.buildcmd != ''

- name: Run Mill '${{ inputs.millargs }}'
run: ./mill -i -k ${{ inputs.millargs }}
# Mill tests are pretty heavy so run them only 3x parallel on 4 core Github Actions runners
run: ./mill -i -j3 -k ${{ inputs.millargs }}
if: inputs.millargs != '' && !startsWith(inputs.os, 'windows')

- name: Run Mill (on Windows) '${{ inputs.millargs }}'
run: cmd /C %GITHUB_WORKSPACE%\ci\mill.bat -i -k ${{ inputs.millargs }}
run: cmd /C %GITHUB_WORKSPACE%\ci\mill.bat -i -j3 -k ${{ inputs.millargs }}
if: inputs.millargs != '' && startsWith(inputs.os, 'windows')

- name: Run Mill (on Windows) Worker Cleanup
Expand Down
24 changes: 12 additions & 12 deletions .github/workflows/run-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,15 @@ jobs:
build-linux:
uses: ./.github/workflows/run-mill-action.yml
with:
java-version: 11
java-version: '11'
millargs: __.compile
populate_cache: true

build-windows:
uses: ./.github/workflows/run-mill-action.yml
with:
os: windows-latest
java-version: 11
java-version: '11'
millargs: __.compile
populate_cache: true

Expand All @@ -53,7 +53,7 @@ jobs:
needs: build-linux
uses: ./.github/workflows/run-mill-action.yml
with:
java-version: '8'
java-version: '11'
millargs: bridge.__.publishLocal
env-bridge-versions: 'essential'

Expand All @@ -71,7 +71,7 @@ jobs:
matrix:
include:
# bootstrap tests
- java-version: 11 # Have one job on oldest JVM
- java-version: '11' # Have one job on oldest JVM
buildcmd: ci/test-mill-dev.sh && ci/test-mill-release.sh && ./mill -i -k __.ivyDepsTree && ./mill -i -k __.ivyDepsTree --withRuntime
- java-version: 17 # Have one job on default JVM
buildcmd: ci/test-mill-bootstrap.sh
Expand All @@ -94,7 +94,7 @@ jobs:
# We also try to group tests together to manuaully balance out the runtimes of each jobs
- java-version: 17
millargs: "'{main,scalalib,testrunner,bsp,testkit}.__.testCached'"
- java-version: 11
- java-version: '11'
millargs: "'{scalajslib,scalanativelib}.__.testCached'"
- java-version: 17
millargs: "contrib.__.testCached"
Expand All @@ -103,16 +103,16 @@ jobs:
millargs: "'example.javalib.__.local.testCached'"
- java-version: 17
millargs: "'example.scalalib.__.local.testCached'"
- java-version: 11
- java-version: '11'
millargs: "'example.thirdparty[{mockito,acyclic,commons-io}].local.testCached'"
- java-version: 17
millargs: "'example.thirdparty[{fansi,jimfs,netty,gatling}].local.testCached'"
- java-version: 11
- java-version: '11'
millargs: "'example.{depth,extending}.__.local.testCached'"

# Most of these integration tests should not depend on which mode they
# are run in, so just run them in `local`
- java-version: 11
- java-version: '11'
millargs: "'integration.{failure,feature,ide}.__.local.testCached'"

# These invalidation tests need to be exercised in both execution modes
Expand All @@ -135,15 +135,15 @@ jobs:
include:
# just run a subset of examples/ on Windows, because for some reason running
# the whole suite can take hours on windows v.s. half an hour on linux
- java-version: 11
- java-version: '11'
millargs: '"{main,scalalib,bsp}.__.testCached"'
- java-version: 11
- java-version: '11'
millargs: '"example.scalalib.{basic,web}.__.fork.testCached"'
- java-version: 17
millargs: "'integration.{feature,failure}[_].fork.testCached'"
- java-version: 11
- java-version: '11'
millargs: "'integration.invalidation[_].server.testCached'"
- java-version: 11
- java-version: '11'
millargs: "contrib.__.testCached"

uses: ./.github/workflows/run-mill-action.yml
Expand Down
1 change: 0 additions & 1 deletion build.mill
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,6 @@ object Deps {

val junitInterface = ivy"com.github.sbt:junit-interface:0.13.3"
val commonsIO = ivy"commons-io:commons-io:2.16.1"
val lambdaTest = ivy"de.tototec:de.tobiasroeser.lambdatest:0.8.0"
val log4j2Core = ivy"org.apache.logging.log4j:log4j-core:2.23.1"
val osLib = ivy"com.lihaoyi::os-lib:0.10.7-M1"
val pprint = ivy"com.lihaoyi::pprint:0.9.0"
Expand Down
56 changes: 56 additions & 0 deletions main/api/src/mill/api/Retry.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
package mill.api

import java.util.concurrent.TimeUnit
import scala.concurrent.duration.Duration
import scala.concurrent.{Await, Promise}

case class Retry(
count: Int = 5,
backoffMillis: Long = 10,
backoffMultiplier: Double = 2.0,
timeoutMillis: Long = -1,
filter: (Int, Throwable) => Boolean = (_, _) => true
) {

/**
* Generic retry functionality
*
* @param count How many times to retry before giving up
* @param backoffMillis What is the initial backoff time
* @param backoffMultiplier How much to multiply the initial backoff each time
* @param timeoutMillis How much time we want to allow [[t]] to run. If passed,
* runs [[t]] in a separate thread and throws a `TimeoutException`
* if it takes too long
* @param filter Whether or not we want to retry a given exception at a given retryCount;
* defaults to `true` to retry all exceptions, but can be made more fine-grained
* to only retry specific exceptions, or log them together with the retryCount
* @param t The code block that we want to retry
* @return the value of evaluating [[t]], or throws an exception if evaluating
* [[t]] fails more than [[count]] times
*/
def apply[T](t: => T): T = {
indexed(i => t)
}

def indexed[T](t: Int => T): T = {
def rec(retryCount: Int, currentBackoffMillis: Long): T = {
try {
if (timeoutMillis == -1) t(retryCount)
else {
val result = Promise[T]
val thread = new Thread(() => {
result.complete(scala.util.Try(t(retryCount)))
})
thread.start()
Await.result(result.future, Duration.apply(timeoutMillis, TimeUnit.MILLISECONDS))
}
} catch {
case e: Throwable if retryCount < count && filter(retryCount + 1, e) =>
Thread.sleep(currentBackoffMillis)
rec(retryCount + 1, (currentBackoffMillis * backoffMultiplier).toInt)
}
}

rec(0, backoffMillis)
}
}
75 changes: 75 additions & 0 deletions main/api/test/src/mill/api/RetryTests.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
package mill.api

import utest._

object RetryTests extends TestSuite {
val tests: Tests = Tests {
test("fail") {
var count = 0
try {
Retry() {
count += 1
throw new Exception("boom")
}
} catch {
case ex =>
assert(ex.getMessage == "boom")
}

assert(count == 6) // 1 original try + 5 retries
}
test("succeed") {
var count = 0
Retry() {
count += 1
if (count < 3) throw new Exception("boom")
}
assert(count == 3)
}
test("filter") {
var count = 0
try {
Retry(
filter = {
case (i, ex: RuntimeException) => true
case _ => false
}
) {
count += 1
if (count < 3) throw new RuntimeException("boom")
else throw new Exception("foo")
}
} catch {
case e: Exception =>
assert(e.getMessage == "foo")
}
assert(count == 3)
}
test("timeout") {
test("fail") {
var count = 0
try {
Retry(timeoutMillis = 100) {
count += 1
Thread.sleep(1000)
}
} catch {
case e: Exception =>
assert(e.getMessage == "Future timed out after [100 milliseconds]")
}

assert(count == 6) // 1 original try + 5 retries
}
test("success") {
var count = 0
Retry(timeoutMillis = 100) {
count += 1
if (count < 3) Thread.sleep(1000)
}

assert(count == 3)
}

}
}
}
9 changes: 6 additions & 3 deletions main/client/test/src/mill/main/client/ClientTests.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package mill.main.client;

import static de.tobiasroeser.lambdatest.Expect.expectEquals;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;

Expand All @@ -12,6 +11,10 @@
import java.util.*;

public class ClientTests {

@org.junit.Rule
public RetryRule retryRule = new RetryRule(3);

@Test
public void readWriteInt() throws Exception{
int[] examples = {
Expand Down Expand Up @@ -58,8 +61,8 @@ public void checkStringRoundTrip(String example) throws Exception{
Util.writeString(o, example);
ByteArrayInputStream i = new ByteArrayInputStream(o.toByteArray());
String s = Util.readString(i);
expectEquals(example, s, "String as bytes: ["+example.getBytes()+"] differs from expected: ["+s.getBytes()+"]");
expectEquals(i.available(), 0);
assertEquals(example, s);
assertEquals(i.available(), 0);
}

public byte[] readSamples(String ...samples) throws Exception{
Expand Down
Loading

0 comments on commit 922d4ef

Please sign in to comment.