From 3b9b20051feee1aa167c4a512b24beebbfdcc9ce Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Tue, 3 Nov 2020 09:16:57 -0800 Subject: [PATCH 1/2] Restore old scripted behavior with batch tests disabled When 1 is passed in as the sbtInstances argument to runInParallel, all of the tests get batched together in a single sbt session. The run methods in ScriptedRunner were delegating to runInParallel and as a result were causing all of the tests to be batched, which was not how it used to work in sbt 1.3.x. To fix this we can instead pass in Int.MaxValue for the number of sbt instances. The Int.MaxValue parameter causes the batch size to be set to 1 in the batchScriptedRunner method which causes the scriptedRunners variable to have the same size as the number of tests. We then can prevent a parallel array from being used if the sbtInstances is deteced to be the Int.MaxValue sentinel. --- .../sbt/scriptedtest/ScriptedTests.scala | 36 ++++++++++++++----- 1 file changed, 28 insertions(+), 8 deletions(-) diff --git a/scripted-sbt-redux/src/main/scala/sbt/scriptedtest/ScriptedTests.scala b/scripted-sbt-redux/src/main/scala/sbt/scriptedtest/ScriptedTests.scala index 3386b504d4..b0a6535980 100644 --- a/scripted-sbt-redux/src/main/scala/sbt/scriptedtest/ScriptedTests.scala +++ b/scripted-sbt-redux/src/main/scala/sbt/scriptedtest/ScriptedTests.scala @@ -376,7 +376,7 @@ class ScriptedRunner { launchOpts: Array[String], ): Unit = { val logger = TestConsoleLogger() - runInParallel( + run( resourceBaseDirectory, bufferLog, tests, @@ -384,7 +384,8 @@ class ScriptedRunner { launchOpts, prescripted = new java.util.ArrayList[File], LauncherBased(launcherJar), - 1 + 1, + parallelExecution = false, ) } @@ -403,7 +404,7 @@ class ScriptedRunner { prescripted: java.util.List[File], ): Unit = { val logger = TestConsoleLogger() - runInParallel( + run( resourceBaseDirectory, bufferLog, tests, @@ -411,7 +412,8 @@ class ScriptedRunner { launchOpts, prescripted, LauncherBased(launcherJar), - 1 + Int.MaxValue, + parallelExecution = false, ) } @@ -477,6 +479,18 @@ class ScriptedRunner { prescripted: java.util.List[File], prop: RemoteSbtCreatorProp, instances: Int + ) = run(baseDir, bufferLog, tests, logger, launchOpts, prescripted, prop, instances, true) + + private[this] def run( + baseDir: File, + bufferLog: Boolean, + tests: Array[String], + logger: Logger, + launchOpts: Array[String], + prescripted: java.util.List[File], + prop: RemoteSbtCreatorProp, + instances: Int, + parallelExecution: Boolean, ): Unit = { val addTestFile = (f: File) => { prescripted.add(f); () } val runner = new ScriptedTests(baseDir, bufferLog, launchOpts) @@ -490,11 +504,17 @@ class ScriptedRunner { // The scripted tests mapped to the inputs that the user wrote after `scripted`. val scriptedTests = get(tests, baseDir, accept, logger).map(st => (st.group, st.name)) + // Choosing Int.MaxValue will make the groupSize 1 in batchScriptedRunner + val groupCount = if (parallelExecution) instances else Int.MaxValue val scriptedRunners = - runner.batchScriptedRunner(scriptedTests, addTestFile, instances, prop, logger) - val parallelRunners = scriptedRunners.toParArray - parallelRunners.tasksupport = new ForkJoinTaskSupport(new ForkJoinPool(instances)) - runAll(parallelRunners) + runner.batchScriptedRunner(scriptedTests, addTestFile, groupCount, prop, logger) + if (parallelExecution && instances > 1) { + val parallelRunners = scriptedRunners.toParArray + parallelRunners.tasksupport = new ForkJoinTaskSupport(new ForkJoinPool(instances)) + runAll(parallelRunners) + } else { + runAll(scriptedRunners) + } } def runInParallel( baseDir: File, From 749d32df36043e57abe8c8a76b89f12c940cbf0f Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Tue, 3 Nov 2020 09:21:26 -0800 Subject: [PATCH 2/2] Set the default scriptedBatchExecution to true sbt itself effectively runs its scripted test with scriptedBatchExecution true and scriptedParallelInstances 1. The performance is much better when this works. This can cause issues, see https://github.com/sbt/sbt/issues/6042, but we inadvertently made this behavior the default in 1.4.0 and it took about a month before #6042 was reported so I think most users would benefit from this default. --- main/src/main/scala/sbt/ScriptedPlugin.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/main/src/main/scala/sbt/ScriptedPlugin.scala b/main/src/main/scala/sbt/ScriptedPlugin.scala index 9bbc7380f0..2d79de76bd 100644 --- a/main/src/main/scala/sbt/ScriptedPlugin.scala +++ b/main/src/main/scala/sbt/ScriptedPlugin.scala @@ -78,7 +78,7 @@ object ScriptedPlugin extends AutoPlugin { scriptedClasspath := getJars(ScriptedConf).value, scriptedTests := scriptedTestsTask.value, scriptedParallelInstances := 1, - scriptedBatchExecution := false, + scriptedBatchExecution := true, scriptedRun := scriptedRunTask.value, scriptedDependencies := { def use[A](@deprecated("unused", "") x: A*): Unit = () // avoid unused warnings