Add DesignerTransaction tests and fix debug assertions (dotnet#30107)

* Add DesignerTransaction tests and fix debug assertions

* Address PR feedback

Author: hughbe

src/System.ComponentModel.TypeConverter/src/System/ComponentModel/Design/DesignerTransaction.cs

@@ -2,55 +2,29 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
-using System;
-using System.Security.Permissions;
-
 namespace System.ComponentModel.Design
 {
     /// <summary>
-    ///     Identifies a transaction within a designer.  Transactions are
-    ///     used to wrap several changes into one unit of work, which 
-    ///     helps performance.
+    /// Identifies a transaction within a designer. Transactions are
+    /// used to wrap several changes into one unit of work, which 
+    /// helps performance.
     /// </summary>
     public abstract class DesignerTransaction : IDisposable
     {
         private bool _suppressedFinalization;
 
-        /// <summary>
-        ///    <para>[To be supplied.]</para>
-        /// </summary>
         protected DesignerTransaction() : this("")
         {
         }
+        
+        protected DesignerTransaction(string description) => Description = description;
 
-
-        /// <summary>
-        ///    <para>[To be supplied.]</para>
-        /// </summary>
-        protected DesignerTransaction(string description)
-        {
-            Description = description;
-        }
-
-
-        /// <summary>
-        ///    <para>[To be supplied.]</para>
-        /// </summary>
         public bool Canceled { get; private set; }
 
-        /// <summary>
-        ///    <para>[To be supplied.]</para>
-        /// </summary>
         public bool Committed { get; private set; }
 
-        /// <summary>
-        ///    <para>[To be supplied.]</para>
-        /// </summary>
         public string Description { get; }
 
-        /// <summary>
-        ///    <para>[To be supplied.]</para>
-        /// </summary>
         public void Cancel()
         {
             if (!Canceled && !Committed)
@@ -63,11 +37,9 @@ public void Cancel()
         }
 
         /// <summary>
-        ///     Commits this transaction.  Once a transaction has
-        ///     been committed, further calls to this method
-        ///     will do nothing.  You should always call this
-        ///     method after creating a transaction to ensure
-        ///     that the transaction is closed properly.
+        /// Commits this transaction. Once a transaction has been committed, further
+        /// calls to this method will do nothing. You should always call this method
+        /// after creating a transaction to ensure that the transaction is closed properly.
         /// </summary>
         public void Commit()
         {
@@ -81,51 +53,36 @@ public void Commit()
         }
 
         /// <summary>
-        ///     User code should implement this method to perform
-        ///     the actual work of committing a transaction.
+        /// User code should implement this method to perform the actual work of
+        /// committing a transaction.
         /// </summary>
         protected abstract void OnCancel();
 
         /// <summary>
-        ///     User code should implement this method to perform
-        ///     the actual work of committing a transaction.
+        /// User code should implement this method to perform the actual work of
+        /// committing a transaction.
         /// </summary>
         protected abstract void OnCommit();
 
         /// <summary>
-        ///     Overrides Object to commit this transaction
-        ///     in case the user forgot.
+        /// Overrides Object to commit this transaction in case the user forgot.
         /// </summary>
-        ~DesignerTransaction()
-        {
-            Dispose(false);
-        }
+        ~DesignerTransaction() => Dispose(false);
 
-        /// <internalonly/>
         /// <summary>
-        /// Private implementation of IDisaposable.
-        /// When a transaction is disposed it is
-        /// committed.
+        /// Private implementation of IDisaposable. When a transaction is disposed
+        /// it is committed.
         /// </summary>
         void IDisposable.Dispose()
         {
             Dispose(true);
 
-            // note - Dispose calls Cancel which sets this bit, so
-            //        this should never be hit.
-            //
             if (!_suppressedFinalization)
             {
-                System.Diagnostics.Debug.Fail("Invalid state. Dispose(true) should have called cancel which does the SuppressFinalize");
                 GC.SuppressFinalize(this);
             }
         }
-        protected virtual void Dispose(bool disposing)
-        {
-            System.Diagnostics.Debug.Assert(disposing, "Designer transaction garbage collected, unable to cancel, please Cancel, Close, or Dispose your transaction.");
-            System.Diagnostics.Debug.Assert(disposing || Canceled || Committed, "Disposing DesignerTransaction that has not been comitted or canceled; forcing Cancel");
-            Cancel();
-        }
+
+        protected virtual void Dispose(bool disposing) => Cancel();
     }
 }
-

src/System.ComponentModel.TypeConverter/tests/Design/DesignerTransactionTests.cs

@@ -0,0 +1,130 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using Xunit;
+
+namespace System.ComponentModel.Design.Tests
+{
+    public class DesignerTransactionTests
+    {
+        [Fact]
+        public void Ctor_Default()
+        {
+            using (var transaction = new TestDesignerTransaction())
+            {
+                Assert.Empty(transaction.Description);
+                Assert.False(transaction.Canceled);
+                Assert.False(transaction.Committed);
+            }
+        }
+
+        [Theory]
+        [InlineData(null)]
+        [InlineData("Description")]
+        public void Ctor_Default(string description)
+        {
+            using (var transaction = new TestDesignerTransaction(description))
+            {
+                Assert.Same(description, transaction.Description);
+                Assert.False(transaction.Canceled);
+                Assert.False(transaction.Committed);
+            }
+        }
+
+        [Fact]
+        public void Cancel_NotCommitted_Success()
+        {
+            using (var transaction = new TestDesignerTransaction())
+            {
+                transaction.Cancel();
+                Assert.Equal(1, transaction.CancelCount);
+                Assert.True(transaction.Canceled);
+
+                transaction.Cancel();
+                Assert.Equal(1, transaction.CancelCount);
+                Assert.True(transaction.Canceled);
+            }
+        }
+
+        [Fact]
+        public void Cancel_Committed_Success()
+        {
+            using (var transaction = new TestDesignerTransaction())
+            {
+                transaction.Commit();
+
+                transaction.Cancel();
+                Assert.Equal(0, transaction.CancelCount);
+                Assert.False(transaction.Canceled);
+            }
+        }
+
+        [Fact]
+        public void Commit_NotCommitted_Success()
+        {
+            using (var transaction = new TestDesignerTransaction())
+            {
+                transaction.Commit();
+                Assert.Equal(1, transaction.CommitCount);
+                Assert.True(transaction.Committed);
+
+                transaction.Commit();
+                Assert.Equal(1, transaction.CommitCount);
+                Assert.True(transaction.Committed);
+            }
+        }
+
+        [Fact]
+        public void Commit_Cancelled_Success()
+        {
+            using (var transaction = new TestDesignerTransaction())
+            {
+                transaction.Cancel();
+
+                transaction.Commit();
+                Assert.Equal(0, transaction.CommitCount);
+                Assert.False(transaction.Committed);
+            }
+        }
+
+        [Fact]
+        public void Dispose_FinalizeSuppressed_Success()
+        {
+            var transaction = new NonDisposingDesignerTransaction();
+            transaction.Cancel();
+            ((IDisposable)transaction).Dispose();
+
+            Assert.True(transaction.Canceled);
+        }
+
+        [Fact]
+        public void Dispose_FinalizeNotSuppressed_Success()
+        {
+            var transaction = new NonDisposingDesignerTransaction();
+            ((IDisposable)transaction).Dispose();
+
+            Assert.False(transaction.Canceled);
+        }
+
+        private class NonDisposingDesignerTransaction : DesignerTransaction
+        {
+            protected override void Dispose(bool disposing) { }
+
+            protected override void OnCancel() { }
+            protected override void OnCommit() { }
+        }
+
+        private class TestDesignerTransaction : DesignerTransaction
+        {
+            public TestDesignerTransaction() : base() { }
+            public TestDesignerTransaction(string description) : base(description) { }
+
+            public int CancelCount { get; set; }
+            protected override void OnCancel() => CancelCount++;
+
+            public int CommitCount { get; set; }
+            protected override void OnCommit() => CommitCount++;
+        }
+    }
+}

src/System.ComponentModel.TypeConverter/tests/System.ComponentModel.TypeConverter.Tests.csproj

@@ -84,6 +84,7 @@
     <Compile Include="ContextStackTests.cs" />
     <Compile Include="InstanceDescriptorTests.cs" />
     <Compile Include="Design\DesignerOptionServiceTests.cs" />
+    <Compile Include="Design\DesignerTransactionTests.cs" />
     <Compile Include="Security\Authentication\ExtendedProtection\ExtendedProtectionPolicyTypeConverterTests.cs" />
   </ItemGroup>
   <ItemGroup>