Guard more bulk-updates behind lock (#13)

README.md

@@ -17,7 +17,7 @@ An [expect-testing](https://blog.janestreet.com/the-joy-of-expect-tests/) librar
 
 The basic mechanism works.
 Snapshot updating is vibe-coded with Opus 4 and is purely text-based; I didn't want to use the F# compiler services because that's a pretty heavyweight dependency which should be confined to a separate test runner entity.
-It's not very well tested, and I expect it to be kind of brittle.
+It's fairly well tested, but you will certainly be able to find ways to break it; try not to be too fancy with your syntax around the `snapshot` statement.
 
 # How to use
 

WoofWare.Expect/Builder.fs

@@ -254,7 +254,7 @@ type ExpectBuilder (mode : Mode) =
                 |> ExpectException
                 |> raise
             | Mode.Assert ->
-                if GlobalBuilderConfig.bulkUpdate.Value > 0 then
+                if GlobalBuilderConfig.isBulkUpdateMode () then
                     GlobalBuilderConfig.registerTest state
                 else
                     sprintf
@@ -275,9 +275,9 @@ type ExpectBuilder (mode : Mode) =
 
         match CompletedSnapshotGeneric.passesAssertion state with
         | None ->
-            match mode, GlobalBuilderConfig.bulkUpdate.Value with
+            match mode, GlobalBuilderConfig.isBulkUpdateMode () with
             | Mode.Update, _
-            | _, 1 ->
+            | _, true ->
                 failwith
                     "Snapshot assertion passed, but we are in snapshot-updating mode. Use the `expect` builder instead of `expect'` to assert the contents of a single snapshot; disable `GlobalBuilderConfig.bulkUpdate` to move back to assertion-checking mode."
             | _ -> ()

WoofWare.Expect/Config.fs

@@ -1,12 +1,19 @@
 namespace WoofWare.Expect
 
-open System.Threading
-
 /// Module holding global mutable state controlling the behaviour of WoofWare.Expect
 /// when running in bulk-update mode.
 [<RequireQualifiedAccess>]
 module GlobalBuilderConfig =
-    let internal bulkUpdate = ref 0
+    /// All access to the global mutable state locks on this.
+    let private locker = obj ()
+
+    // Global mutable state ensuring there is at most one `enterBulkUpdateMode`/`updateAllSnapshots` pair running at once.
+    let private bulkUpdate = ref 0
+
+    let private allTests : ResizeArray<CompletedSnapshot> = ResizeArray ()
+
+    let internal isBulkUpdateMode () : bool =
+        lock locker (fun () -> bulkUpdate.Value > 0)
 
     /// <summary>
     /// Call this to make the <c>expect</c> builder register all tests for bulk update as it runs.
@@ -16,11 +23,15 @@ module GlobalBuilderConfig =
     /// The implied global mutable state is liable to interfere with other expect builders in other fixtures otherwise.
     /// </remarks>
     let enterBulkUpdateMode () =
-        if Interlocked.Increment bulkUpdate <> 1 then
-            failwith
-                "WoofWare.Expect requires bulk updates to happen serially: for example, make the test fixture `[<NonParallelizable>]` if you're using NUnit."
+        lock
+            locker
+            (fun () ->
+                if bulkUpdate.Value <> 0 then
+                    failwith
+                        "WoofWare.Expect requires bulk updates to happen serially: for example, make the test fixture `[<NonParallelizable>]` if you're using NUnit."
 
-    let private allTests : ResizeArray<CompletedSnapshot> = ResizeArray ()
+                bulkUpdate.Value <- bulkUpdate.Value + 1
+            )
 
     /// <summary>
     /// Clear the set of failing tests registered by any previous bulk-update runs.
@@ -30,23 +41,31 @@ module GlobalBuilderConfig =
     /// You probably don't need to do this, because your test runner is probably tearing down
     /// anyway after the tests have failed; this is mainly here for WoofWare.Expect's own internal testing.
     /// </remarks>
-    let clearTests () = lock allTests allTests.Clear
+    let clearTests () = lock locker allTests.Clear
 
     let internal registerTest (s : CompletedSnapshotGeneric<'T>) : unit =
         let toAdd = s |> CompletedSnapshot.make
-        lock allTests (fun () -> allTests.Add toAdd)
+        lock locker (fun () -> allTests.Add toAdd)
 
     /// <summary>
     /// For all tests whose failures have already been registered,
     /// transform the files on disk so that the failing snapshots now pass.
     /// </summary>
     let updateAllSnapshots () =
-        let bulkUpdate' = Interlocked.Decrement bulkUpdate
+        // It's OK for this to be called when `enterBulkUpdateMode` has not been called, i.e. when `bulkUpdate` has
+        // value 0. That just means we aren't in bulk-update mode, so we expect the following simply to do nothing.
+        // (This is an expected workflow: we expect users to run `updateAllSnapshots` unconditionally in a
+        // one-time tear-down of the test suite, and they use the one-time setup to control whether any work is actually
+        // performed here.)
+        lock
+            locker
+            (fun () ->
+                let allTests = Seq.toArray allTests
 
-        try
-            if bulkUpdate' = 0 then
-                let allTests = lock allTests (fun () -> Seq.toArray allTests)
-                SnapshotUpdate.updateAll allTests
-
-        finally
-            clearTests ()
+                try
+                    SnapshotUpdate.updateAll allTests
+                finally
+                    // double acquiring of reentrant lock is OK, we're not switching threads
+                    clearTests ()
+                    bulkUpdate.Value <- 0
+            )