Fix verifier cleanup composition, stream/xml disposal and comparison (#1771)

* Fix verifier cleanup composition, stream/xml disposal and comparison

- Compose async cleanups with a Then helper so both are awaited; `+=` on a
  Func<Task> builds a multicast delegate that only awaits the last target, so
  converter/user async cleanups were fire-and-forget
- Run cleanup in a finally around HandleResults so it still runs when the
  comparison throws
- IsAutoVerify: only gate the global autoVerify delegate on typeName, so the
  per-settings delegate works for the file-level InnerVerifier(directory, name)
- VerifyXml(Stream) disposes the stream, matching the other stream APIs
- Comparer.Text reads the verified file with VerifierSettings.Encoding so a
  BOM-less non-UTF8 UseEncoding round-trips (writes already use it)
- StreamComparer rents its buffers from ArrayPool and compares exactly the
  bytes read via SequenceEqual (a rented buffer's trailing bytes are arbitrary)
- Add tests for the above

* Docs changes

---------

Co-authored-by: GitHub Action <action@github.com>

Author: SimonCropp

docs/comparer.md

@@ -119,35 +119,41 @@ public static async Task<CompareResult> AreEqual(Stream stream1, Stream stream2)
     EnsureAtStart(stream1);
     EnsureAtStart(stream2);
 
-    var buffer1 = new byte[bufferSize];
-    var buffer2 = new byte[bufferSize];
-
-    while (true)
+    var buffer1 = ArrayPool<byte>.Shared.Rent(bufferSize);
+    var buffer2 = ArrayPool<byte>.Shared.Rent(bufferSize);
+    try
     {
-        var count1 = await ReadBufferAsync(stream1, buffer1);
-        var count2 = await ReadBufferAsync(stream2, buffer2);
-
-        // Callers do not always guarantee the streams are the same length
-        // (e.g. a non-seekable received stream), so a length difference must
-        // be treated as not-equal instead of a short-circuit to equal.
-        if (count1 != count2)
+        while (true)
         {
-            return CompareResult.NotEqual();
-        }
+            var count1 = await ReadBufferAsync(stream1, buffer1);
+            var count2 = await ReadBufferAsync(stream2, buffer2);
 
-        if (count1 == 0)
-        {
-            return CompareResult.Equal;
-        }
+            // Callers do not always guarantee the streams are the same length
+            // (e.g. a non-seekable received stream), so a length difference must
+            // be treated as not-equal instead of a short-circuit to equal.
+            if (count1 != count2)
+            {
+                return CompareResult.NotEqual();
+            }
 
-        for (var i = 0; i < count1; i += sizeof(long))
-        {
-            if (BitConverter.ToInt64(buffer1, i) != BitConverter.ToInt64(buffer2, i))
+            if (count1 == 0)
+            {
+                return CompareResult.Equal;
+            }
+
+            // Compare exactly the bytes read (a rented buffer's trailing bytes
+            // are arbitrary, so they must not be included).
+            if (!buffer1.AsSpan(0, count1).SequenceEqual(buffer2.AsSpan(0, count1)))
             {
                 return CompareResult.NotEqual();
             }
         }
     }
+    finally
+    {
+        ArrayPool<byte>.Shared.Return(buffer1);
+        ArrayPool<byte>.Shared.Return(buffer2);
+    }
 }
 
 static void EnsureAtStart(Stream stream)
@@ -161,10 +167,13 @@ static void EnsureAtStart(Stream stream)
 
 static async Task<int> ReadBufferAsync(Stream stream, byte[] buffer)
 {
+    // Read up to bufferSize (not buffer.Length): a rented buffer may be
+    // larger, and both streams must be read in equal-sized chunks so the
+    // length comparison above stays aligned.
     var bytesRead = 0;
-    while (bytesRead < buffer.Length)
+    while (bytesRead < bufferSize)
     {
-        var read = await stream.ReadAsync(buffer, bytesRead, buffer.Length - bytesRead);
+        var read = await stream.ReadAsync(buffer, bytesRead, bufferSize - bytesRead);
         if (read == 0)
         {
             // Reached end of stream.
@@ -177,7 +186,7 @@ static async Task<int> ReadBufferAsync(Stream stream, byte[] buffer)
     return bytesRead;
 }
 ```
-<sup><a href='/src/Verify/Compare/StreamComparer.cs#L3-L70' title='Snippet source file'>snippet source</a> | <a href='#snippet-DefualtCompare' title='Start of snippet'>anchor</a></sup>
+<sup><a href='/src/Verify/Compare/StreamComparer.cs#L3-L79' title='Snippet source file'>snippet source</a> | <a href='#snippet-DefualtCompare' title='Start of snippet'>anchor</a></sup>
 <!-- endSnippet -->
 
 

docs/naming.md

@@ -789,7 +789,7 @@ public static string NameWithParent(this Type type)
     return type.Name;
 }
 ```
-<sup><a href='/src/Verify/Extensions.cs#L123-L135' title='Snippet source file'>snippet source</a> | <a href='#snippet-NameWithParent' title='Start of snippet'>anchor</a></sup>
+<sup><a href='/src/Verify/Extensions.cs#L132-L144' title='Snippet source file'>snippet source</a> | <a href='#snippet-NameWithParent' title='Start of snippet'>anchor</a></sup>
 <!-- endSnippet -->
 
 Any path calculated in `DerivePathInfo` should be fully qualified to remove the inconsistency of the current directory.

src/Verify.Tests/Converters/AsyncCleanupTests.ConverterAsyncCleanupIsAwaited.verified.txt

@@ -0,0 +1 @@
+content

src/Verify.Tests/Converters/AsyncCleanupTests.cs

@@ -0,0 +1,30 @@
+public class AsyncCleanupTests
+{
+    static bool asyncCleanupRan;
+
+    [ModuleInitializer]
+    public static void Init() =>
+        VerifierSettings.RegisterFileConverter<TargetForAsyncCleanup>(
+            (instance, _) => new(
+                info: null,
+                "txt",
+                instance.Value,
+                cleanup: async () =>
+                {
+                    await Task.Delay(200);
+                    asyncCleanupRan = true;
+                }));
+
+    [Fact]
+    public async Task ConverterAsyncCleanupIsAwaited()
+    {
+        asyncCleanupRan = false;
+        var target = new TargetForAsyncCleanup("content");
+        await Verify(target);
+        // The converter's async cleanup must be awaited before Verify returns;
+        // composing cleanups with += only awaits the last one.
+        Assert.True(asyncCleanupRan);
+    }
+
+    public record TargetForAsyncCleanup(string Value);
+}

src/Verify.Tests/StreamComparerTests.cs

@@ -9,6 +9,36 @@ public async Task BinaryEquals()
         Assert.True(result.IsEqual);
     }
 
+    [Fact]
+    public async Task EqualWithLengthNotMultipleOfEight()
+    {
+        // 13 bytes exercises a partial final block; the trailing bytes of a
+        // rented buffer must not affect the result.
+        var bytes = new byte[13];
+        for (var index = 0; index < bytes.Length; index++)
+        {
+            bytes[index] = (byte) index;
+        }
+
+        using var stream1 = new MemoryStream(bytes);
+        using var stream2 = new MemoryStream((byte[]) bytes.Clone());
+        var result = await StreamComparer.AreEqual(stream1, stream2);
+        Assert.True(result.IsEqual);
+    }
+
+    [Fact]
+    public async Task NotEqualInPartialFinalBlock()
+    {
+        var bytes1 = new byte[13];
+        var bytes2 = new byte[13];
+        bytes2[12] = 1;
+
+        using var stream1 = new MemoryStream(bytes1);
+        using var stream2 = new MemoryStream(bytes2);
+        var result = await StreamComparer.AreEqual(stream1, stream2);
+        Assert.False(result.IsEqual);
+    }
+
     [Fact]
     public async Task BinaryNotEqualsSameLength()
     {

src/Verify.Tests/VerifyXmlStreamTests.DisposesStream.verified.xml

@@ -0,0 +1 @@
+<root />

src/Verify.Tests/VerifyXmlStreamTests.cs

@@ -0,0 +1,11 @@
+public class VerifyXmlStreamTests
+{
+    [Fact]
+    public async Task DisposesStream()
+    {
+        var stream = new MemoryStream("<root />"u8.ToArray());
+        await VerifyXml(stream);
+        // VerifyXml(Stream) must dispose the stream, like the other stream APIs.
+        Assert.False(stream.CanRead);
+    }
+}

src/Verify/Compare/Comparer.cs

@@ -9,7 +9,9 @@ public static async Task<EqualityResult> Text(FilePair filePair, StringBuilder r
             return new(Equality.New, null, received, null);
         }
 
-        var verified = await File.ReadAllTextAsync(filePair.VerifiedPath);
+        // Read with the configured encoding so a BOM-less non-UTF8 UseEncoding
+        // round-trips (writes go through the same encoding).
+        var verified = await File.ReadAllTextAsync(filePair.VerifiedPath, VerifierSettings.Encoding);
         if (verified.Contains('\r'))
         {
             throw new($@"Verified file must use \n line endings, but it contains a \r (carriage return). Path: {filePair.VerifiedPath}. See https://github.com/verifytests/verify#text-file-settings");

src/Verify/Compare/StreamComparer.cs

@@ -9,35 +9,41 @@ public static async Task<CompareResult> AreEqual(Stream stream1, Stream stream2)
         EnsureAtStart(stream1);
         EnsureAtStart(stream2);
 
-        var buffer1 = new byte[bufferSize];
-        var buffer2 = new byte[bufferSize];
-
-        while (true)
+        var buffer1 = ArrayPool<byte>.Shared.Rent(bufferSize);
+        var buffer2 = ArrayPool<byte>.Shared.Rent(bufferSize);
+        try
         {
-            var count1 = await ReadBufferAsync(stream1, buffer1);
-            var count2 = await ReadBufferAsync(stream2, buffer2);
-
-            // Callers do not always guarantee the streams are the same length
-            // (e.g. a non-seekable received stream), so a length difference must
-            // be treated as not-equal instead of a short-circuit to equal.
-            if (count1 != count2)
+            while (true)
             {
-                return CompareResult.NotEqual();
-            }
+                var count1 = await ReadBufferAsync(stream1, buffer1);
+                var count2 = await ReadBufferAsync(stream2, buffer2);
 
-            if (count1 == 0)
-            {
-                return CompareResult.Equal;
-            }
+                // Callers do not always guarantee the streams are the same length
+                // (e.g. a non-seekable received stream), so a length difference must
+                // be treated as not-equal instead of a short-circuit to equal.
+                if (count1 != count2)
+                {
+                    return CompareResult.NotEqual();
+                }
 
-            for (var i = 0; i < count1; i += sizeof(long))
-            {
-                if (BitConverter.ToInt64(buffer1, i) != BitConverter.ToInt64(buffer2, i))
+                if (count1 == 0)
+                {
+                    return CompareResult.Equal;
+                }
+
+                // Compare exactly the bytes read (a rented buffer's trailing bytes
+                // are arbitrary, so they must not be included).
+                if (!buffer1.AsSpan(0, count1).SequenceEqual(buffer2.AsSpan(0, count1)))
                 {
                     return CompareResult.NotEqual();
                 }
             }
         }
+        finally
+        {
+            ArrayPool<byte>.Shared.Return(buffer1);
+            ArrayPool<byte>.Shared.Return(buffer2);
+        }
     }
 
     static void EnsureAtStart(Stream stream)
@@ -51,10 +57,13 @@ static void EnsureAtStart(Stream stream)
 
     static async Task<int> ReadBufferAsync(Stream stream, byte[] buffer)
     {
+        // Read up to bufferSize (not buffer.Length): a rented buffer may be
+        // larger, and both streams must be read in equal-sized chunks so the
+        // length comparison above stays aligned.
         var bytesRead = 0;
-        while (bytesRead < buffer.Length)
+        while (bytesRead < bufferSize)
         {
-            var read = await stream.ReadAsync(buffer, bytesRead, buffer.Length - bytesRead);
+            var read = await stream.ReadAsync(buffer, bytesRead, bufferSize - bytesRead);
             if (read == 0)
             {
                 // Reached end of stream.

src/Verify/Extensions.cs

@@ -35,6 +35,15 @@ public static string Extension(this FileStream file)
     public static Version MajorMinor(this Version version) =>
         new(version.Major, version.Minor);
 
+    // Compose two async actions so both are awaited in order. Using `+=` on a
+    // Func<Task> builds a multicast delegate that only awaits the last target.
+    public static Func<Task> Then(this Func<Task> first, Func<Task> second) =>
+        async () =>
+        {
+            await first();
+            await second();
+        };
+
     public static async Task<List<T>> ToList<T>(this IAsyncEnumerable<T> target)
     {
         var list = new List<T>();