[jvm] NullPointerException in SnapshotSystemJUnit5

[racevedoo]

HI everyone! Thanks for the amazing project, it really makes jvm testing fun/easy.

When using Junit5, I'm intermittently getting the following NullPointerException:

    java.lang.NullPointerException
        at com.diffplug.selfie.junit5.SnapshotSystemJUnit5.forClass(SnapshotSystemJUnit5.kt:77)
        at com.diffplug.selfie.junit5.SelfieTestExecutionListener.executionStarted(SelfieTestExecutionListener.kt:33)

When investigating, I noticed that it's linked to the order of execution for the tests. In my case, only a single test in the suite uses selfie.

Digging deeper, I've found that the following patch solves the issue for my particular use case:

diff --git a/jvm/selfie-runner-junit5/src/main/kotlin/com/diffplug/selfie/junit5/SelfieExtension.kt b/jvm/selfie-runner-junit5/src/main/kotlin/com/diffplug/selfie/junit5/SelfieExtension.kt
index aed9aee4..dcc5f391 100644
--- a/jvm/selfie-runner-junit5/src/main/kotlin/com/diffplug/selfie/junit5/SelfieExtension.kt
+++ b/jvm/selfie-runner-junit5/src/main/kotlin/com/diffplug/selfie/junit5/SelfieExtension.kt
@@ -36,13 +36,13 @@ import kotlinx.coroutines.withContext
 class SelfieExtension(projectConfig: AbstractProjectConfig) :
     Extension, BeforeSpecListener, TestCaseExtension, FinalizeSpecListener, AfterProjectListener {
   override suspend fun beforeSpec(spec: Spec) {
-    SnapshotSystemJUnit5.forClass(spec.javaClass.name).incrementContainers()
+    SnapshotSystemJUnit5.forClass(spec.javaClass.name)?.incrementContainers()
   }
   override suspend fun intercept(
       testCase: TestCase,
       execute: suspend (TestCase) -> TestResult
   ): TestResult {
-    val file = SnapshotSystemJUnit5.forClass(testCase.spec::class.java.name)
+    val file = SnapshotSystemJUnit5.forClass(testCase.spec::class.java.name)!!
     val coroutineLocal = CoroutineDiskStorage(DiskStorageJUnit5(file, testCase.name.testName))
     return withContext(currentCoroutineContext() + coroutineLocal) {
       file.startTest(testCase.name.testName, false)
@@ -58,12 +58,12 @@ class SelfieExtension(projectConfig: AbstractProjectConfig) :
     val file = SnapshotSystemJUnit5.forClass(kclass.java.name)
     results.entries.forEach {
       if (it.value.isIgnored) {
-        file.startTest(it.key.name.testName, false)
-        file.finishedTestWithSuccess(it.key.name.testName, false, false)
+        file?.startTest(it.key.name.testName, false)
+        file?.finishedTestWithSuccess(it.key.name.testName, false, false)
       }
     }
-    SnapshotSystemJUnit5.forClass(kclass.java.name)
-        .decrementContainersWithSuccess(results.values.all { it.isSuccess })
+      SnapshotSystemJUnit5.forClass(kclass.java.name)
+          ?.decrementContainersWithSuccess(results.values.all { it.isSuccess })
   }
   override suspend fun afterProject() {
     // If you run from the CLI, `SelfieTestExecutionListener` will run and so will `afterProject`
diff --git a/jvm/selfie-runner-junit5/src/main/kotlin/com/diffplug/selfie/junit5/SelfieTestExecutionListener.kt b/jvm/selfie-runner-junit5/src/main/kotlin/com/diffplug/selfie/junit5/SelfieTestExecutionListener.kt
index b6cbe3fb..1e5c0a97 100644
--- a/jvm/selfie-runner-junit5/src/main/kotlin/com/diffplug/selfie/junit5/SelfieTestExecutionListener.kt
+++ b/jvm/selfie-runner-junit5/src/main/kotlin/com/diffplug/selfie/junit5/SelfieTestExecutionListener.kt
@@ -32,9 +32,9 @@ class SelfieTestExecutionListener : TestExecutionListener {
       val (clazz, test) = parseClassTest(testIdentifier)
       val snapshotFile = system.forClass(clazz)
       if (test == null) {
-        snapshotFile.incrementContainers()
+        snapshotFile?.incrementContainers()
       } else {
-        system.forClass(clazz).startTest(test, true)
+        system.forClass(clazz)?.startTest(test, true)
       }
     } catch (e: Throwable) {
       system.layout.smuggledError.set(e)
@@ -44,8 +44,8 @@ class SelfieTestExecutionListener : TestExecutionListener {
     try {
       val (clazz, test) = parseClassTest(testIdentifier)
       if (test == null) {
-        system.forClass(clazz).incrementContainers()
-        system.forClass(clazz).decrementContainersWithSuccess(false)
+        system.forClass(clazz)?.incrementContainers()
+        system.forClass(clazz)?.decrementContainersWithSuccess(false)
       } else {
         // TODO: using reflection right now, but we should probably listen to these
       }
@@ -63,9 +63,9 @@ class SelfieTestExecutionListener : TestExecutionListener {
       val isSuccess = testExecutionResult.status == TestExecutionResult.Status.SUCCESSFUL
       val snapshotFile = system.forClass(clazz)
       if (test == null) {
-        snapshotFile.decrementContainersWithSuccess(isSuccess)
+        snapshotFile?.decrementContainersWithSuccess(isSuccess)
       } else {
-        snapshotFile.finishedTestWithSuccess(test, true, isSuccess)
+        snapshotFile?.finishedTestWithSuccess(test, true, isSuccess)
       }
     } catch (e: Throwable) {
       system.layout.smuggledError.set(e)
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 a0358d0d..1e4b6aa8 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
@@ -68,7 +68,7 @@ internal object SnapshotSystemJUnit5 : SnapshotSystem {
   private val inlineWriteTracker = InlineWriteTracker()
   private val toBeFileWriteTracker = ToBeFileWriteTracker()
   private val progressPerClass = atomic(ArrayMap.empty<String, SnapshotFileProgress>())
-  fun forClass(className: String): SnapshotFileProgress {
+  fun forClass(className: String): SnapshotFileProgress? {
     // optimize for reads
     progressPerClass.get()[className]?.let {
       return it
@@ -76,7 +76,7 @@ internal object SnapshotSystemJUnit5 : SnapshotSystem {
     // sometimes we'll have to write
     return progressPerClass
         .updateAndGet { it.plusOrNoOp(className, SnapshotFileProgress(this, className)) }[
-            className]!!
+            className]
   }
   private val checkForInvalidStale = atomic<ArraySet<TypedPath>?>(ArraySet.empty())
   internal fun markPathAsWritten(typedPath: TypedPath) {

This makes SnapshotSystemJUnit5.forClass return SnapshotFileProgress? instead of SnapshotFileProgress and updates references.

I couldn't come up with a simple reproducer yet, but my question is: should this happen at all? progressPerClass is an atomic reference to an ArrayMap which should always contain className (thanks to the plusOrNoOp call)

[nedtwigg]

it really makes jvm testing fun/easy

Glad you like it!

should this happen at all?

No, it should not:

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

Lines 71 to 80 in e6fb9e6

	fun forClass(className: String): SnapshotFileProgress {
	// optimize for reads
	progressPerClass.get()[className]?.let {
	return it
	}
	// sometimes we'll have to write
	return progressPerClass
	.updateAndGet { it.plusOrNoOp(className, SnapshotFileProgress(this, className)) }[
	className]!!
	}

• I wonder if there is something similar to [jvm] Snapshots are not being written to disk when there are tests with numbered names #540 going on.

I couldn't come up with a simple reproducer yet

Can you share some of the class names of your tests? Do they happen to have numbers in them?

[racevedoo]

Can you share some of the class names of your tests? Do they happen to have numbers in them?

Some of them do, but those do not uses selfie assertions. I'll work on providing a reproducer soon

[racevedoo]

I've narrowed it down a bit more: the classes that trigger are something like my.package.MyClass30DaysMaxAllTimeV1Test and my.package.MyClass3DaysMaxAllTimeV1Test

It does seem related to #540 somehow. I've confirmed that this is order-related since I cannot reproduce the issue when applying the following patch:

Patch

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 a0358d0d..e37d5316 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
@@ -38,6 +38,9 @@ import kotlin.io.path.readBytes
 import kotlin.io.path.writeBytes
 import kotlin.streams.asSequence
 import org.opentest4j.AssertionFailedError
+import java.util.concurrent.ConcurrentHashMap
+import java.util.concurrent.ConcurrentMap
+
 internal fun TypedPath.toPath(): java.nio.file.Path = java.nio.file.Path.of(absolutePath)
 
 internal object FSJava : FS {
@@ -67,16 +70,9 @@ internal object SnapshotSystemJUnit5 : SnapshotSystem {
   private val commentTracker = CommentTracker()
   private val inlineWriteTracker = InlineWriteTracker()
   private val toBeFileWriteTracker = ToBeFileWriteTracker()
-  private val progressPerClass = atomic(ArrayMap.empty<String, SnapshotFileProgress>())
+  private val progressPerClass = ConcurrentHashMap<String, SnapshotFileProgress>()
   fun forClass(className: String): SnapshotFileProgress {
-    // optimize for reads
-    progressPerClass.get()[className]?.let {
-      return it
-    }
-    // sometimes we'll have to write
-    return progressPerClass
-        .updateAndGet { it.plusOrNoOp(className, SnapshotFileProgress(this, className)) }[
-            className]!!
+    return progressPerClass.computeIfAbsent(className) { SnapshotFileProgress(this, className) }
   }
   private val checkForInvalidStale = atomic<ArraySet<TypedPath>?>(ArraySet.empty())
   internal fun markPathAsWritten(typedPath: TypedPath) {

This basically changes the Atomic<ArrayMap<...>> to a ConcurrentHashMap. The order of entries in progressPerClass does not seem relevant.

IMO this patch is safe enough to be merged into main. Thoughts?