From dbd99dd04b6bb68a51c905e1325b0ece07980581 Mon Sep 17 00:00:00 2001 From: Nathan Regner Date: Mon, 19 May 2025 11:08:22 -0600 Subject: [PATCH 1/2] Prevent garbage collection if `junit.jupiter.api.TestFactory` annotations are present --- jvm/CHANGELOG.md | 1 + .../src/main/kotlin/com/diffplug/selfie/junit5/SelfieGC.kt | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/jvm/CHANGELOG.md b/jvm/CHANGELOG.md index 52dfa48a..01ef9fc2 100644 --- a/jvm/CHANGELOG.md +++ b/jvm/CHANGELOG.md @@ -13,6 +13,7 @@ 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) ## [2.5.2] - 2025-04-28 ### Fixed diff --git a/jvm/selfie-runner-junit5/src/main/kotlin/com/diffplug/selfie/junit5/SelfieGC.kt b/jvm/selfie-runner-junit5/src/main/kotlin/com/diffplug/selfie/junit5/SelfieGC.kt index 3b5c6960..dce505e4 100644 --- a/jvm/selfie-runner-junit5/src/main/kotlin/com/diffplug/selfie/junit5/SelfieGC.kt +++ b/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 ) From 99852ac8fe41ee1f335c9aab46bfb4e977fa8776 Mon Sep 17 00:00:00 2001 From: Nathan Regner Date: Fri, 16 May 2025 16:55:08 -0600 Subject: [PATCH 2/2] Fix race conditions writing snapshots when `junit.jupiter.execution.parallel.enabled` is true --- jvm/CHANGELOG.md | 1 + .../diffplug/selfie/guts/CommentTracker.kt | 5 +- .../selfie/junit5/SnapshotSystemJUnit5.kt | 50 ++++++++++--------- .../selfie/junit5/ConcurrencyStressTest.kt | 45 +++++++++++++++++ .../junit5/UT_ConcurrencyStressTest.kt | 27 ++++++++++ .../test/resources/junit-platform.properties | 4 ++ 6 files changed, 107 insertions(+), 25 deletions(-) create mode 100644 jvm/selfie-runner-junit5/src/test/kotlin/com/diffplug/selfie/junit5/ConcurrencyStressTest.kt create mode 100644 jvm/undertest-junit5/src/test/java/undertest/junit5/UT_ConcurrencyStressTest.kt create mode 100644 jvm/undertest-junit5/src/test/resources/junit-platform.properties diff --git a/jvm/CHANGELOG.md b/jvm/CHANGELOG.md index 01ef9fc2..a78819e3 100644 --- a/jvm/CHANGELOG.md +++ b/jvm/CHANGELOG.md @@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### 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 diff --git a/jvm/selfie-lib/src/commonMain/kotlin/com/diffplug/selfie/guts/CommentTracker.kt b/jvm/selfie-lib/src/commonMain/kotlin/com/diffplug/selfie/guts/CommentTracker.kt index 36ba76e5..ad70a71e 100644 --- a/jvm/selfie-lib/src/commonMain/kotlin/com/diffplug/selfie/guts/CommentTracker.kt +++ b/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 } } diff --git a/jvm/selfie-runner-junit5/src/main/kotlin/com/diffplug/selfie/junit5/SnapshotSystemJUnit5.kt b/jvm/selfie-runner-junit5/src/main/kotlin/com/diffplug/selfie/junit5/SnapshotSystemJUnit5.kt index b9fa1299..a0358d0d 100644 --- a/jvm/selfie-runner-junit5/src/main/kotlin/com/diffplug/selfie/junit5/SnapshotSystemJUnit5.kt +++ b/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(null) private val tests = atomic(ArrayMap.empty()) - private var diskWriteTracker: DiskWriteTracker? = DiskWriteTracker() + private var diskWriteTracker = atomic(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() diff --git a/jvm/selfie-runner-junit5/src/test/kotlin/com/diffplug/selfie/junit5/ConcurrencyStressTest.kt b/jvm/selfie-runner-junit5/src/test/kotlin/com/diffplug/selfie/junit5/ConcurrencyStressTest.kt new file mode 100644 index 00000000..53ca71f3 --- /dev/null +++ b/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() + 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() + } +} diff --git a/jvm/undertest-junit5/src/test/java/undertest/junit5/UT_ConcurrencyStressTest.kt b/jvm/undertest-junit5/src/test/java/undertest/junit5/UT_ConcurrencyStressTest.kt new file mode 100644 index 00000000..e19a4c1d --- /dev/null +++ b/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)) + } + } +} diff --git a/jvm/undertest-junit5/src/test/resources/junit-platform.properties b/jvm/undertest-junit5/src/test/resources/junit-platform.properties new file mode 100644 index 00000000..48d1fff3 --- /dev/null +++ b/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