[jvm] NullPointerException in SnapshotSystemJUnit5

Start with SnapshotSystemJUnit5.kt at forClass and the callers in SelfieExtension.kt and SelfieTestExecutionListener.kt. Inspect how the atomic progressPerClass map is updated and reproduce the test-order-dependent NullPointerException, since no simple reproducer is provided. Done means explaining or preventing the missing SnapshotFileProgress case with coverage for the intermittent JUnit5 scenario.

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)