Add Container tests and fix a debug assertion (#22073)

* Add Container tests and fix a Debug assertion

* Cleanup Container.cs a tad

* Revert early exit change

2 files changed, 244 insertions, 94 deletions

diff --git a/src/System.ComponentModel.TypeConverter/src/System/ComponentModel/Container.cs b/src/System.ComponentModel.TypeConverter/src/System/ComponentModel/Container.cs
index 41017d3167..2c87c068a1 100644
--- a/src/System.ComponentModel.TypeConverter/src/System/ComponentModel/Container.cs
+++ b/src/System.ComponentModel.TypeConverter/src/System/ComponentModel/Container.cs
@@ -2,20 +2,10 @@
 // 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.ComponentModel;
-using System.Diagnostics;
-using System.Globalization;
-using System.IO;
-using System.Security.Permissions;
-
 namespace System.ComponentModel
 {
     /// <summary>
-    ///    <para>
-    ///       Encapsulates
-    ///       zero or more components.
-    ///    </para>
+    /// Encapsulates zero or more components.
     /// </summary>
     public class Container : IContainer
     {
@@ -25,19 +15,13 @@ namespace System.ComponentModel
         private ContainerFilterService _filter;
         private bool _checkedFilter;
 
-        private readonly object _syncObj = new Object();
+        private readonly object _syncObj = new object();
 
-        ~Container()
-        {
-            Dispose(false);
-        }
+        ~Container() => Dispose(false);
 
-        // Adds a component to the container.
         /// <summary>
-        ///    <para>
-        ///       Adds the specified component to the <see cref='System.ComponentModel.Container'/>
-        ///       . The component is unnamed.
-        ///    </para>
+        /// Adds the specified component to the <see cref='System.ComponentModel.Container'/>
+        /// The component is unnamed.
         /// </summary>
         public virtual void Add(IComponent component)
         {
@@ -46,12 +30,10 @@ namespace System.ComponentModel
 
         // Adds a component to the container.
         /// <summary>
-        ///    <para>
-        ///       Adds the specified component to the <see cref='System.ComponentModel.Container'/> and assigns a name to
-        ///       it.
-        ///    </para>
+        /// Adds the specified component to the <see cref='System.ComponentModel.Container'/> and assigns
+        /// a name to it.
         /// </summary>
-        public virtual void Add(IComponent component, String name)
+        public virtual void Add(IComponent component, string name)
         {
             lock (_syncObj)
             {
@@ -61,7 +43,6 @@ namespace System.ComponentModel
                 }
 
                 ISite site = component.Site;
-
                 if (site != null && site.Container == this)
                 {
                     return;
@@ -95,40 +76,34 @@ namespace System.ComponentModel
             }
         }
 
-        // Creates a site for the component within the container.
         /// <summary>
-        /// <para>Creates a Site <see cref='System.ComponentModel.ISite'/> for the given <see cref='System.ComponentModel.IComponent'/>
-        /// and assigns the given name to the site.</para>
+        /// Creates a Site <see cref='System.ComponentModel.ISite'/> for the given <see cref='System.ComponentModel.IComponent'/>
+        /// and assigns the given name to the site.
         /// </summary>
         protected virtual ISite CreateSite(IComponent component, string name)
         {
             return new Site(component, this, name);
         }
 
-        // Disposes of the container.  A call to the Dispose method indicates that
-        // the user of the container has no further need for it.
-        //
-        // The implementation of Dispose must:
-        //
-        // (1) Remove any references the container is holding to other components.
-        //     This is typically accomplished by assigning null to any fields that
-        //     contain references to other components.
-        //
-        // (2) Release any system resources that are associated with the container,
-        //     such as file handles, window handles, or database connections.
-        //
-        // (3) Dispose of child components by calling the Dispose method of each.
-        //
-        // Ideally, a call to Dispose will revert a container to the state it was
-        // in immediately after it was created. However, this is not a requirement.
-        // Following a call to its Dispose method, a container is permitted to raise
-        // exceptions for operations that cannot meaningfully be performed.
-        //
         /// <summary>
-        ///    <para>
-        ///       Disposes of the <see cref='System.ComponentModel.Container'/>
-        ///       .
-        ///    </para>
+        /// Disposes of the container.  A call to the Dispose method indicates that
+        /// the user of the container has no further need for it.
+        ///
+        /// The implementation of Dispose must:
+        ///
+        /// (1) Remove any references the container is holding to other components.
+        ///     This is typically accomplished by assigning null to any fields that
+        ///     contain references to other components.
+        ///
+        /// (2) Release any system resources that are associated with the container,
+        ///     such as file handles, window handles, or database connections.
+        ///
+        /// (3) Dispose of child components by calling the Dispose method of each.
+        ///
+        /// Ideally, a call to Dispose will revert a container to the state it was
+        /// in immediately after it was created. However, this is not a requirement.
+        /// Following a call to its Dispose method, a container is permitted to raise
+        /// exceptions for operations that cannot meaningfully be performed.
         /// </summary>
         public void Dispose()
         {
@@ -154,20 +129,10 @@ namespace System.ComponentModel
             }
         }
 
+        protected virtual object GetService(Type service) => service == typeof(IContainer) ? this : null;
+        
         /// <summary>
-        ///    <para>[To be supplied.]</para>
-        /// </summary>
-        protected virtual object GetService(Type service)
-        {
-            return ((service == typeof(IContainer)) ? this : null);
-        }
-
-        // The components in the container.
-        /// <summary>
-        ///    <para>
-        ///       Gets all the components in the <see cref='System.ComponentModel.Container'/>
-        ///       .
-        ///    </para>
+        /// Gets all the components in the <see cref='System.ComponentModel.Container'/>.
         /// </summary>
         public virtual ComponentCollection Components
         {
@@ -201,7 +166,6 @@ namespace System.ComponentModel
                     if (_filter != null)
                     {
                         ComponentCollection filteredComponents = _filter.FilterComponents(_components);
-                        Debug.Assert(filteredComponents != null, "Incorrect ContainerFilterService implementation.");
                         if (filteredComponents != null)
                         {
                             _components = filteredComponents;
@@ -213,17 +177,10 @@ namespace System.ComponentModel
             }
         }
 
-        // Removes a component from the container.
         /// <summary>
-        ///    <para>
-        ///       Removes a component from the <see cref='System.ComponentModel.Container'/>
-        ///       .
-        ///    </para>
+        /// Removes a component from the <see cref='System.ComponentModel.Container'/>.
         /// </summary>
-        public virtual void Remove(IComponent component)
-        {
-            Remove(component, false);
-        }
+        public virtual void Remove(IComponent component) => Remove(component, false);
 
         private void Remove(IComponent component, bool preserveSite)
         {
@@ -231,9 +188,15 @@ namespace System.ComponentModel
             {
                 ISite site = component?.Site;
                 if (site == null || site.Container != this)
+                {
                     return;
+                }
+
                 if (!preserveSite)
+                {
                     component.Site = null;
+                }
+
                 for (int i = 0; i < _siteCount; i++)
                 {
                     if (_sites[i] == site)
@@ -248,15 +211,12 @@ namespace System.ComponentModel
             }
         }
 
-        protected void RemoveWithoutUnsiting(IComponent component)
-        {
-            Remove(component, true);
-        }
+        protected void RemoveWithoutUnsiting(IComponent component) => Remove(component, true);
 
         /// <summary>
-        ///     Validates that the given name is valid for a component.  The default implementation
-        ///     verifies that name is either null or unique compared to the names of other
-        ///     components in the container.
+        /// Validates that the given name is valid for a component. The default implementation
+        /// verifies that name is either null or unique compared to the names of other
+        /// components in the container.
         /// </summary>
         protected virtual void ValidateName(IComponent component, string name)
         {
@@ -285,40 +245,45 @@ namespace System.ComponentModel
 
         private class Site : ISite
         {
-            private String _name;
+            private string _name;
 
-            internal Site(IComponent component, Container container, String name)
+            internal Site(IComponent component, Container container, string name)
             {
                 Component = component;
                 Container = container;
                 _name = name;
             }
 
-            // The component sited by this component site.
+            /// <summary>
+            /// The component sited by this component site.
+            /// </summary>
             public IComponent Component { get; }
 
-            // The container in which the component is sited.
+            /// <summary>
+            /// The container in which the component is sited.
+            /// </summary>
             public IContainer Container { get; }
 
-            public Object GetService(Type service)
+            public object GetService(Type service)
             {
                 return ((service == typeof(ISite)) ? this : ((Container)Container).GetService(service));
             }
 
-
-            // Indicates whether the component is in design mode.
+            /// <summary>
+            /// Indicates whether the component is in design mode.
+            /// </summary>
             public bool DesignMode => false;
 
-            // The name of the component.
-            //
-            public String Name
+            /// <summary>
+            /// The name of the component.
+            /// </summary>
+            public string Name
             {
                 get { return _name; }
                 set
                 {
                     if (value == null || _name == null || !value.Equals(_name))
                     {
-                        // UNDONE : This is a breaking change.  
                        ((Container)Container).ValidateName(Component, value);
                         _name = value;
                     }
diff --git a/src/System.ComponentModel.TypeConverter/tests/ContainerTests.cs b/src/System.ComponentModel.TypeConverter/tests/ContainerTests.cs
index f154d8f132..58e08bb363 100644
--- a/src/System.ComponentModel.TypeConverter/tests/ContainerTests.cs
+++ b/src/System.ComponentModel.TypeConverter/tests/ContainerTests.cs
@@ -13,6 +13,7 @@
 //
 
 using System.ComponentModel.Design;
+using System.Linq;
 using Xunit;
 
 namespace System.ComponentModel.Tests
@@ -283,6 +284,19 @@ namespace System.ComponentModel.Tests
         }
 
         [Fact]
+        public void Add_ExceedsSizeOfBuffer_Success()
+        {
+            var container = new Container();
+            var components = new Component[] { new Component(), new Component(), new Component(), new Component(), new Component() };
+            
+            for (int i = 0; i < components.Length; i++)
+            {
+                container.Add(components[i]);
+                Assert.Same(components[i], container.Components[i]);
+            }
+        }
+
+        [Fact]
         public void Add2_Component_Null()
         {
             _container.Add((IComponent)null, "A");
@@ -369,6 +383,50 @@ namespace System.ComponentModel.Tests
         }
 
         [Fact]
+        public void Add_SetSiteName_ReturnsExpected()
+        {
+            var component = new Component();
+            var container = new Container();
+
+            container.Add(component, "Name1");
+            Assert.Equal("Name1", component.Site.Name);
+
+            component.Site.Name = "OtherName";
+            Assert.Equal("OtherName", component.Site.Name);
+
+            // Setting to the same value is a nop.
+            component.Site.Name = "OtherName";
+            Assert.Equal("OtherName", component.Site.Name);
+        }
+
+        [Fact]
+        public void Add_SetSiteNameDuplicate_ThrowsArgumentException()
+        {
+            var component1 = new Component();
+            var component2 = new Component();
+            var container = new Container();
+
+            container.Add(component1, "Name1");
+            container.Add(component2, "Name2");
+
+            Assert.Throws<ArgumentException>(null, () => component1.Site.Name = "Name2");
+        }
+
+        [Fact]
+        public void Add_DuplicateNameWithInheritedReadOnly_AddsSuccessfully()
+        {
+            var component1 = new Component();
+            var component2 = new Component();
+            TypeDescriptor.AddAttributes(component1, new InheritanceAttribute(InheritanceLevel.InheritedReadOnly));
+
+            var container = new Container();
+            container.Add(component1, "Name");
+            container.Add(component2, "Name");
+
+            Assert.Equal(new IComponent[] { component1, component2 }, container.Components.Cast<IComponent>());
+        }
+
+        [Fact]
         public void AddRemove()
         {
             TestComponent component = new TestComponent();
@@ -617,6 +675,31 @@ namespace System.ComponentModel.Tests
         }
 
         [Fact]
+        public void Remove_NoSuchComponentWithoutUnsiting_Nop()
+        {
+            var component1 = new Component();
+            var component2 = new Component();
+            var container = new SitingContainer();
+
+            container.Add(component1);
+            container.Add(component2);
+
+            container.DoRemoveWithoutUnsitting(component1);
+            Assert.Equal(1, container.Components.Count);
+
+            container.DoRemoveWithoutUnsitting(component1);
+            Assert.Equal(1, container.Components.Count);
+
+            container.DoRemoveWithoutUnsitting(component2);
+            Assert.Equal(0, container.Components.Count);
+        }
+
+        private class SitingContainer : Container
+        {
+            public void DoRemoveWithoutUnsitting(IComponent component) => RemoveWithoutUnsiting(component);
+        }
+
+        [Fact]
         public void ValidateName_Component_Null()
         {
             ArgumentNullException ex = Assert.Throws<ArgumentNullException>(() => _container.InvokeValidateName((IComponent)null, "A"));
@@ -703,6 +786,108 @@ namespace System.ComponentModel.Tests
             Assert.Same(compD, container2.Components[0]);
         }
 
+        [Fact]
+        public void Components_GetWithDefaultFilterService_ReturnsAllComponents()
+        {
+            var component1 = new SubComponent();
+            var component2 = new Component();
+            var component3 = new SubComponent();
+
+            var container = new FilterContainer { FilterService = new DefaultFilterService() };
+            container.Add(component1);
+            container.Add(component2);
+            container.Add(component3);
+
+            Assert.Equal(new IComponent[] { component1, component2, component3 }, container.Components.Cast<IComponent>());
+        }
+
+        [Fact]
+        public void Components_GetWithCustomFilterService_ReturnsFilteredComponents()
+        {
+            var component1 = new SubComponent();
+            var component2 = new Component();
+            var component3 = new SubComponent();
+
+            // This filter only includes SubComponents.
+            var container = new FilterContainer { FilterService = new CustomContainerFilterService() };
+            container.Add(component1);
+            container.Add(component2);
+            container.Add(component3);
+
+            Assert.Equal(new IComponent[] { component1, component3 }, container.Components.Cast<IComponent>());
+        }
+
+        [Fact]
+        public void Components_GetWithCustomFilterServiceAfterChangingComponents_ReturnsUpdatedComponents()
+        {
+            var component1 = new SubComponent();
+            var component2 = new Component();
+            var component3 = new SubComponent();
+
+            // This filter only includes SubComponents.
+            var container = new FilterContainer { FilterService = new CustomContainerFilterService() };
+            container.Add(component1);
+            container.Add(component2);
+            container.Add(component3);
+
+            Assert.Equal(new IComponent[] { component1, component3 }, container.Components.Cast<IComponent>());
+
+            container.Remove(component1);
+            Assert.Equal(new IComponent[] { component3 }, container.Components.Cast<IComponent>());
+        }
+
+        [Fact]
+        public void Components_GetWithNullFilterService_ReturnsUnfiltered()
+        {
+            var component1 = new SubComponent();
+            var component2 = new Component();
+            var component3 = new SubComponent();
+
+            // This filter only includes SubComponents.
+            var container = new FilterContainer { FilterService = new NullContainerFilterService() };
+            container.Add(component1);
+            container.Add(component2);
+            container.Add(component3);
+
+            Assert.Equal(new IComponent[] { component1, component2, component3 }, container.Components.Cast<IComponent>());
+        }
+
+        private class FilterContainer : Container
+        {
+            public ContainerFilterService FilterService { get; set; }
+
+            protected override object GetService(Type service)
+            {
+                if (service == typeof(ContainerFilterService))
+                {
+                    return FilterService;
+                }
+
+                return base.GetService(service);
+            }
+        }
+
+        private class SubComponent : Component { }
+
+        private class DefaultFilterService : ContainerFilterService { }
+
+        private class CustomContainerFilterService : ContainerFilterService
+        {
+            public override ComponentCollection FilterComponents(ComponentCollection components)
+            {
+                SubComponent[] newComponents = components.OfType<SubComponent>().ToArray();
+                return new ComponentCollection(newComponents);
+            }
+        }
+
+        private class NullContainerFilterService : ContainerFilterService
+        {
+            public override ComponentCollection FilterComponents(ComponentCollection components)
+            {
+                return null;
+            }
+        }
+
         private class MyComponent : Component
         {
             private Container container;