Fix race conditions writing snapshots when junit.jupiter.execution.parallel.enabled is true (#534)

Author: nedtwigg

jvm/CHANGELOG.md

@@ -13,6 +13,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## [Unreleased]
 ### Fixed
 - Restore support for JRE 11. (fixes [#528](https://github.com/diffplug/selfie/issues/528))
+- snapshots created by `junit.jupiter.api.TestFactory` are no longer garbage-collected (#534)
+- support parallel testing under `junit.jupiter.execution.parallel.enabled=true` (#534)
 
 ## [2.5.2] - 2025-04-28
 ### Fixed

jvm/selfie-lib/src/commonMain/kotlin/com/diffplug/selfie/guts/CommentTracker.kt

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2024 DiffPlug
+ * Copyright (C) 2024-2025 DiffPlug
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -39,7 +39,8 @@ class CommentTracker {
       comment.writable
     } else {
       val newComment = commentAndLine(path, layout.fs).first
-      cache.updateAndGet { it.plus(path, newComment) }
+      // may race get(), ignore if already present
+      cache.updateAndGet { it.plusOrNoOp(path, newComment) }
       newComment.writable
     }
   }

jvm/selfie-runner-junit5/src/main/kotlin/com/diffplug/selfie/junit5/SelfieGC.kt

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2023-2024 DiffPlug
+ * Copyright (C) 2023-2025 DiffPlug
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -22,6 +22,7 @@ import com.diffplug.selfie.guts.WithinTestGC
 private val testAnnotations =
     listOf(
             "org.junit.jupiter.api.Test", // junit5,
+            "org.junit.jupiter.api.TestFactory", // junit5,
             "org.junit.jupiter.params.ParameterizedTest",
             "org.junit.Test" // junit4
             )

jvm/selfie-runner-junit5/src/main/kotlin/com/diffplug/selfie/junit5/SnapshotSystemJUnit5.kt

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2023-2024 DiffPlug
+ * Copyright (C) 2023-2025 DiffPlug
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -177,9 +177,9 @@ internal class SnapshotFileProgress(val system: SnapshotSystemJUnit5, val classN
     require(tests.get() !== TERMINATED) { "Cannot call methods on a terminated ClassProgress" }
   }
 
-  private var file: SnapshotFile? = null
+  private var file = atomic<SnapshotFile?>(null)
   private val tests = atomic(ArrayMap.empty<String, WithinTestGC>())
-  private var diskWriteTracker: DiskWriteTracker? = DiskWriteTracker()
+  private var diskWriteTracker = atomic<DiskWriteTracker?>(DiskWriteTracker())
   private val timesStarted = AtomicInteger(0)
   private val hasFailed = AtomicBoolean(false)
 
@@ -209,23 +209,24 @@ internal class SnapshotFileProgress(val system: SnapshotSystemJUnit5, val classN
   }
   private fun finishedClassWithSuccess(success: Boolean) {
     assertNotTerminated()
-    diskWriteTracker = null // don't need this anymore
+    diskWriteTracker.updateAndGet { null } // don't need this anymore
     val tests = tests.getAndUpdate { TERMINATED }
     require(tests !== TERMINATED) { "Snapshot $className already terminated!" }
+    val file = file.getAndUpdate { null }
     if (file != null) {
       val staleSnapshotIndices =
           WithinTestGC.findStaleSnapshotsWithin(
-              file!!.snapshots, tests, findTestMethodsThatDidntRun(className, tests))
-      if (staleSnapshotIndices.isNotEmpty() || file!!.wasSetAtTestTime) {
-        file!!.removeAllIndices(staleSnapshotIndices)
+              file.snapshots, tests, findTestMethodsThatDidntRun(className, tests))
+      if (staleSnapshotIndices.isNotEmpty() || file.wasSetAtTestTime) {
+        file.removeAllIndices(staleSnapshotIndices)
         val snapshotPath = system.layout.snapshotPathForClass(className)
-        if (file!!.snapshots.isEmpty()) {
+        if (file.snapshots.isEmpty()) {
           deleteFileAndParentDirIfEmpty(snapshotPath)
         } else {
           system.markPathAsWritten(system.layout.snapshotPathForClass(className))
           Files.createDirectories(snapshotPath.toPath().parent)
           Files.newBufferedWriter(snapshotPath.toPath(), StandardCharsets.UTF_8).use { writer ->
-            file!!.serialize(writer)
+            file.serialize(writer)
           }
         }
       }
@@ -239,8 +240,6 @@ internal class SnapshotFileProgress(val system: SnapshotSystemJUnit5, val classN
         deleteFileAndParentDirIfEmpty(snapshotFile)
       }
     }
-    // now that we are done, allow our contents to be GC'ed
-    file = null
   }
   // the methods below are called from the test thread for I/O on snapshots
   fun keep(test: String, suffixOrAll: String?) {
@@ -260,7 +259,7 @@ internal class SnapshotFileProgress(val system: SnapshotSystemJUnit5, val classN
   ) {
     assertNotTerminated()
     val key = "$test$suffix"
-    diskWriteTracker!!.record(key, snapshot, callStack, layout)
+    diskWriteTracker.get()!!.record(key, snapshot, callStack, layout)
     tests.get()[test]!!.keepSuffix(suffix)
     read().setAtTestTime(key, snapshot)
   }
@@ -273,17 +272,22 @@ internal class SnapshotFileProgress(val system: SnapshotSystemJUnit5, val classN
     return snapshot
   }
   private fun read(): SnapshotFile {
-    if (file == null) {
-      val snapshotPath = system.layout.snapshotPathForClass(className).toPath()
-      file =
-          if (Files.exists(snapshotPath) && Files.isRegularFile(snapshotPath)) {
-            val content = Files.readAllBytes(snapshotPath)
-            SnapshotFile.parse(SnapshotValueReader.of(content))
-          } else {
-            SnapshotFile.createEmptyWithUnixNewlines(system.layout.unixNewlines)
-          }
-    }
-    return file!!
+    val file = this.file.get()
+    if (file != null) return file
+
+    return this.file.updateAndGet { file ->
+      if (file != null) {
+        file
+      } else {
+        val snapshotPath = system.layout.snapshotPathForClass(className).toPath()
+        if (Files.exists(snapshotPath) && Files.isRegularFile(snapshotPath)) {
+          val content = Files.readAllBytes(snapshotPath)
+          SnapshotFile.parse(SnapshotValueReader.of(content))
+        } else {
+          SnapshotFile.createEmptyWithUnixNewlines(system.layout.unixNewlines)
+        }
+      }
+    }!!
   }
   fun incrementContainers() {
     assertNotTerminated()

jvm/selfie-runner-junit5/src/test/kotlin/com/diffplug/selfie/junit5/ConcurrencyStressTest.kt

@@ -0,0 +1,45 @@
+/*
+ * Copyright (C) 2025 DiffPlug
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.diffplug.selfie.junit5
+
+import com.diffplug.selfie.ArrayMap
+import com.diffplug.selfie.Snapshot
+import com.diffplug.selfie.SnapshotFile
+import kotlin.text.format
+import org.junit.jupiter.api.Test
+
+class ConcurrencyStressTest : HarnessJUnit() {
+
+  @Test
+  fun smoke() {
+    gradleWriteSS()
+    ut_snapshot().assertContent(expectedSnapshot())
+    gradleReadSS()
+    ut_snapshot().deleteIfExists()
+  }
+  private fun expectedSnapshot(): String {
+    var expectedSnapshots = ArrayMap.empty<String, Snapshot>()
+    for (d in 1..1000) {
+      expectedSnapshots =
+          expectedSnapshots.plus(String.format("testFactory/%04d", d), Snapshot.of(d.toString()))
+    }
+    val snapshotFile = SnapshotFile()
+    snapshotFile.snapshots = expectedSnapshots
+    val buffer = kotlin.text.StringBuilder()
+    snapshotFile.serialize(buffer)
+    return buffer.toString()
+  }
+}

jvm/undertest-junit5/src/test/java/undertest/junit5/UT_ConcurrencyStressTest.kt

@@ -0,0 +1,27 @@
+package undertest.junit5
+
+import com.diffplug.selfie.Selfie.expectSelfie
+import java.util.concurrent.CountDownLatch
+import java.util.concurrent.TimeUnit
+import kotlin.streams.asStream
+import org.junit.jupiter.api.DynamicTest.dynamicTest
+import org.junit.jupiter.api.TestFactory
+import org.junit.jupiter.api.parallel.Execution
+import org.junit.jupiter.api.parallel.ExecutionMode
+
+@Execution(ExecutionMode.CONCURRENT)
+class UT_ConcurrencyStressTest {
+// sanity check: make sure our junit-platform.properties file is getting picked up
+  private val latch = CountDownLatch(8)
+
+  @TestFactory
+  fun testFactory() =
+      (1..1000).asSequence().asStream().map { digit ->
+        dynamicTest(String.format("%04d", digit)) {
+          latch.countDown()
+          latch.await(5, TimeUnit.SECONDS)
+          println(Thread.currentThread())
+          expectSelfie(digit.toString()).toMatchDisk(String.format("%04d", digit))
+        }
+      }
+}

jvm/undertest-junit5/src/test/resources/junit-platform.properties

@@ -0,0 +1,4 @@
+junit.jupiter.execution.parallel.enabled=true
+# force parallelism, even if there's only 1 CPU core
+junit.jupiter.execution.parallel.config.strategy=fixed
+junit.jupiter.execution.parallel.config.fixed.parallelism=8