Add test runner indicators #4737

HollandDM · 2025-03-16T16:49:15Z

This PR add some calculation to add test progress & failure indicator for each test runner while testParallelism is set to true

To add progress indicators, this pull request introduces a new file within each worker's claim folder. This file is overwritten every time runClaimedTestClass is called. The parent process will checks these files in each worker, accumulates their contents, and outputs them via a ticker.

For test timing, this pull request uses a separate map, similar to workerStatusMap, to store the starting time. The time calculation is then straightforward, and the result is output via a ticker alongside the test class name.

lihaoyi · 2025-03-22T02:04:52Z

testrunner/src/mill/testrunner/TestRunnerUtils.scala

+  @internal private[TestRunnerUtils] final class TestEventSummary(outputFileOpt: Option[os.Path])
+      extends AtomicBoolean(true) {


This class wrapper and inheritance is kind of weird. Let's convert them to just static helper methods and pass in outputFileOpt and the AtomicBoolean as arguments where necessary

This is a trick to better the memory layout of object, I guess it's kind of overkill here

lihaoyi · 2025-03-22T02:07:22Z

testrunner/src/mill/testrunner/TestRunnerUtils.scala

+        val (success, failure) = upickle.default.read[(Long, Long)](os.read.stream(outputFile))
+        val (newSuccess, newFailure) =
+          if (isSuccess) (success + 1, failure) else (success, failure + 1)


This workflow to both read and write to the file on disk is unnecessarily complicated. Let's just keep the success/failure count as in-memory Long (or AtomicLong) and have the file be write-only purely for the purpose of passing the information to the parent process

lihaoyi · 2025-03-22T02:07:40Z

scalalib/src/mill/scalalib/TestModuleUtil.scala

+        // help gc
+        workerStatusMap.clear()
+        workerStatsSet.clear()
+        testClassTimeMap.clear()


This should be unnecessary, let's skip it

lihaoyi · 2025-03-22T02:08:36Z

scalalib/src/mill/scalalib/TestModuleUtil.scala

        val result = callTestRunnerSubprocess(
          base,
          Right((startingTestClass, testClassQueueFolder, claimFolder))
        )
        workerStatusMap.remove(claimLog)
+        // We don't remove workerStatsSet entry, as we still need them for calculation


Can elaborate a bit more ...for calculation of total success/failure counts

lihaoyi · 2025-03-22T02:09:03Z

@HollandDM Looks pretty good, some small changes requested. Please also update the PR description with a summary of your implementation strategy

HollandDM · 2025-03-22T05:19:03Z

Updated to adrees the feedbacks

lihaoyi · 2025-03-22T05:33:24Z

testrunner/src/mill/testrunner/TestRunnerUtils.scala

+            event.status match {
+              case Status.Error => taskStatus.set(false)
+              case Status.Failure => taskStatus.set(false)
+              case _ => taskStatus.compareAndSet(true, true) // consider success as a default


I think we can leave out this compareAndSet; this branch of the match being a no-op would be sufficient

lihaoyi · 2025-03-22T05:37:55Z

scalalib/src/mill/scalalib/TestModuleUtil.scala

@@ -225,6 +225,8 @@ private final class TestModuleUtil(
  )(implicit ctx: mill.api.Ctx) = {

    val workerStatusMap = new java.util.concurrent.ConcurrentHashMap[os.Path, String => Unit]()
+    val workerStatsSet = new java.util.concurrent.ConcurrentHashMap[os.Path, Unit]()


Maybe call this workerResultsSet? Stats and Status sounds too similar

lihaoyi · 2025-03-22T05:40:13Z

scalalib/src/mill/scalalib/TestModuleUtil.scala

+                val suffix = ((now - last) / 1000).toInt match {
+                  case 0 => ""
+                  case n => s" ${n}s"
+                }


Let's move PromptLoggerUtil.renderSecondsSuffix to mill.internal.Util so we can re-use it here

HollandDM · 2025-03-22T06:04:13Z

Updated again. After the ticket is good to merge, I'll squash them back into 1 commit, I think it'll be better

lihaoyi · 2025-03-22T06:17:08Z

I think this looks good for testParallelism = true. Could you also make the progress count and failure count show in testParallelism = false? I think a lot of code would be re-usable, and it would help improve the UX of the 0.12.x branch where test parallelism is false by default

lihaoyi · 2025-03-22T06:17:42Z

No need to worry about squashing your commits, we'll squash it during the merge using github's squash-and-merge button

lihaoyi · 2025-03-22T08:07:13Z

scalalib/src/mill/scalalib/TestModuleUtil.scala

+              totalFailure += failure
+            }
+            ctx.log.ticker(
+              s"${totalSuccess + totalFailure}/${filteredClassCount} completed, ${totalFailure} failures"


Let's only print n failures for n > 0, since that's what we do in the main prompt for failed tasks

HollandDM · 2025-03-22T08:38:12Z

Updated, I added progress indicator (complete/failure) to default run as well.

lihaoyi · 2025-03-22T09:14:25Z

scalalib/src/mill/scalalib/TestModuleUtil.scala

+    try {
+      // Periodically check the result log file and tick the relevant infos
+      executor.scheduleWithFixedDelay(
+        () => {
+          val (totalSuccess, totalFailure) = TestModuleUtil.calculateTestProgress(workerResultSet)
+          ctx.log.ticker(s"${totalSuccess + totalFailure}/${filteredClassCount} completed${
+              if totalFailure > 0 then s", ${totalFailure} failures" else ""
+            }")
+        },
+        0,
+        20,
+        java.util.concurrent.TimeUnit.MILLISECONDS
+      )


Let's consolidate the whole val workerResultSet, newScheduledThreadPool, scheduleWithFixedDelay, finally executor.shutdown logic into a single

def withTestProgressTickerThread(updateWorkerStatuses: Boolean)(block: => T): T

That should help DRY up the duplicate code, and also shorten some of these def runTestQueueScheduler/def runTestDefault methods which are getting rather long

lihaoyi · 2025-03-22T09:16:55Z

testrunner/src/mill/testrunner/TestRunnerUtils.scala

@@ -203,15 +211,34 @@ import java.io.PrintStream
  }

  def runTasks(
-      tasks: Seq[Task],
+      tasksSeq: Seq[Seq[Task]],


What's the reason for this change in signature from Seq[Task] to Seq[Seq[Task]]? I've been looking at the code and haven't managed to figure out the intention

I guess each element of the outer Seq represents one test class, and each inner Seq represents the tasks for that test class? If so, probably worth a comment somewhere

Yeah, I'm adding the comments in the backport PR, will add it in this as well. But it's meaning is exactly what you've said

lihaoyi · 2025-03-22T09:22:51Z

Some last nitpicks. Tried it out locally and works great. Turned on CI, once it's green it should be good to merge

lihaoyi · 2025-03-22T09:46:28Z

The mill.scalalib.TestRunnerTests.doneMessage.failure seems to happen reliably on my laptop as well, might need to tweak the test or code to make it pass

HollandDM · 2025-03-22T13:59:14Z

updated, the test failing is due to the changes of Seq[Task] to Seq[Seq[Task]], I left a comment there

lihaoyi · 2025-03-22T14:59:49Z

scalalib/src/mill/scalalib/TestModuleUtil.scala

+        val resultPath = base / s"result.log"
+        os.write.over(resultPath, upickle.default.write((0L, 0L)))
+        workerResultSet.put(resultPath, ())
+
        val result = callTestRunnerSubprocess(
          base,
+          resultPath,
          Right((startingTestClass, testClassQueueFolder, claimFolder))
        )


Can we consolidate this logic to use runTestRunnerSubprocess as well?

runTestRunnerSubprocess's are quite different between queueRunner and defaultRunner, so I have to create 2 versions for it, each in the corresponding runners function.
I'm also thinking of a way to further clean up the logic, if we can generalize both runners to just produce blueprint for subprocess, and do the spawn + logging in a shared function, then the code should be clearer. But that required some refactoring.
If you want, I can try to go that way and see if we can do any better.

Got it, I think for now let's just leave it as is then

back port of #4737 --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>

lihaoyi · 2025-03-22T16:07:55Z

Thanks @HollandDM , i will send the bounty using your existing transfer details

HollandDM · 2025-03-22T16:31:44Z

Thanks @lihaoyi, lets use that as my transfer detail

test-runner-indicators

4e18fe0

This was referenced Mar 20, 2025

Test runner indicators backport #4756

Merged

Add progress and failure indicators to Mill test runner (500USD Bounty) #4735

Closed

lihaoyi reviewed Mar 22, 2025

View reviewed changes

update feedback

0e2ce69

HollandDM force-pushed the test-runner-indicators branch from 2ce3ad7 to 0e2ce69 Compare March 22, 2025 05:18

HollandDM requested a review from lihaoyi March 22, 2025 05:18

lihaoyi reviewed Mar 22, 2025

View reviewed changes

update feedback 2

88d8467

HollandDM force-pushed the test-runner-indicators branch from e704d40 to 88d8467 Compare March 22, 2025 06:03

HollandDM requested a review from lihaoyi March 22, 2025 06:03

lihaoyi reviewed Mar 22, 2025

View reviewed changes

HollandDM force-pushed the test-runner-indicators branch from 053cd80 to a2aab21 Compare March 22, 2025 08:37

HollandDM requested a review from lihaoyi March 22, 2025 08:37

update feedbacks 3

0810718

HollandDM force-pushed the test-runner-indicators branch from a2aab21 to 0810718 Compare March 22, 2025 08:39

lihaoyi marked this pull request as ready for review March 22, 2025 09:04

lihaoyi reviewed Mar 22, 2025

View reviewed changes

update feedback 4

0e1dfa3

HollandDM force-pushed the test-runner-indicators branch from 09fc0c4 to 0e1dfa3 Compare March 22, 2025 13:58

HollandDM requested a review from lihaoyi March 22, 2025 13:58

[autofix.ci] apply automated fixes

8a2080c

lihaoyi reviewed Mar 22, 2025

View reviewed changes

lihaoyi merged commit 6fd66af into com-lihaoyi:main Mar 22, 2025
61 of 63 checks passed

lihaoyi pushed a commit that referenced this pull request Mar 22, 2025

Test runner indicators backport (#4756)

a942e8a

back port of #4737 --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>

lefou added this to the 0.13.0 milestone Mar 22, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test runner indicators #4737

Add test runner indicators #4737

HollandDM commented Mar 16, 2025 •

edited

Loading

lihaoyi Mar 22, 2025

HollandDM Mar 22, 2025

lihaoyi Mar 22, 2025

lihaoyi Mar 22, 2025

lihaoyi Mar 22, 2025

lihaoyi commented Mar 22, 2025

HollandDM commented Mar 22, 2025

lihaoyi Mar 22, 2025

lihaoyi Mar 22, 2025

lihaoyi Mar 22, 2025

HollandDM commented Mar 22, 2025

lihaoyi commented Mar 22, 2025

lihaoyi commented Mar 22, 2025

lihaoyi Mar 22, 2025

HollandDM commented Mar 22, 2025

lihaoyi Mar 22, 2025

lihaoyi Mar 22, 2025

lihaoyi Mar 22, 2025

HollandDM Mar 22, 2025

lihaoyi commented Mar 22, 2025

lihaoyi commented Mar 22, 2025

HollandDM commented Mar 22, 2025

lihaoyi Mar 22, 2025 •

edited

Loading

HollandDM Mar 22, 2025

lihaoyi Mar 22, 2025

lihaoyi commented Mar 22, 2025

HollandDM commented Mar 22, 2025

		@internal private[TestRunnerUtils] final class TestEventSummary(outputFileOpt: Option[os.Path])
		extends AtomicBoolean(true) {

Add test runner indicators #4737

Add test runner indicators #4737

Conversation

HollandDM commented Mar 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lihaoyi commented Mar 22, 2025

HollandDM commented Mar 22, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HollandDM commented Mar 22, 2025

lihaoyi commented Mar 22, 2025

lihaoyi commented Mar 22, 2025

Choose a reason for hiding this comment

HollandDM commented Mar 22, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lihaoyi commented Mar 22, 2025

lihaoyi commented Mar 22, 2025

HollandDM commented Mar 22, 2025

lihaoyi Mar 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lihaoyi commented Mar 22, 2025

HollandDM commented Mar 22, 2025

HollandDM commented Mar 16, 2025 •

edited

Loading

lihaoyi Mar 22, 2025 •

edited

Loading