Add tests for System.ComponentModel by AlexGhiondea · Pull Request #12238 · dotnet/corefx

AlexGhiondea · 2016-09-30T17:28:07Z

These were ported over from Mono

AlexGhiondea · 2016-09-30T17:43:05Z

/cc @danmosemsft @weshaggard

stephentoub · 2016-09-30T17:31:54Z

+			Assert.True (ibl.SupportsChangeNotification, "7");
+			Assert.False (ibl.SupportsSearching, "8");
+			Assert.False (ibl.SupportsSorting, "9");
+			Assert.False (((IRaiseItemChangedEvents)l).RaisesItemChangedEvents, "10");


Nit: instead of a number, can we use the name of the thing being tested? e.g.

Assert.True(l.AllowEdit, nameof(l.AllowEdit));

?
Same comment applies elsewhere.

stephentoub · 2016-09-30T17:32:22Z

+			Assert.True (l.RaiseListChangedEvents, "4");
+
+			Assert.False (ibl.IsSorted, "5");
+			Assert.Equal (ibl.SortDirection, ListSortDirection.Ascending);


Nit: in Assert.Equal, the first arg is the expected value and the second is the actual value, so these should be swapped

stephentoub · 2016-09-30T17:32:33Z

+			Assert.True (l.RaiseListChangedEvents, "4");
+
+			Assert.False (ibl.IsSorted, "5");
+			Assert.Equal (ibl.SortDirection, ListSortDirection.Ascending);


Ditto on swapping the order

stephentoub · 2016-09-30T17:32:45Z

+			Assert.True (l.RaiseListChangedEvents, "4");
+
+			Assert.False (ibl.IsSorted, "5");
+			Assert.Equal (ibl.SortDirection, ListSortDirection.Ascending);


Nit: ditto on swapping the order

stephentoub · 2016-09-30T17:32:51Z

+			Assert.True (l.RaiseListChangedEvents, "4");
+
+			Assert.False (ibl.IsSorted, "5");
+			Assert.Equal (ibl.SortDirection, ListSortDirection.Ascending);


Nit: ditto on swapping the order

stephentoub · 2016-09-30T17:43:36Z

+			Assert.Null (compB.Site);
+		}
+
+		[Fact] // bug #522474


Same nits about bug numbers as earlier.

stephentoub · 2016-09-30T17:44:45Z

+			try {
+				object context = stack [-5];
+				Assert.False(true);
+			} catch (ArgumentOutOfRangeException ex) {


All of the tests that follow this pattern should be switched to use Assert.Throws

I fixed all of them that did not check the caught exception.

I fixed all of them that did not check the caught exception.

You can still do that with Assert.Throws, which returns the caught exception.

Nice! I didn't know about that overload! :)

stephentoub · 2016-09-30T17:47:01Z

+
+			result = converter.ConvertToString (null, CultureInfo.InvariantCulture,
+				new CultureInfo ("nl-BE"));
+			Assert.Equal ("nl-BE", result);


Nit: we don't actually need the local in cases like these, e.g.

Assert.Equal ("nl-BE", converter.ConvertToString(null, CultureInfo.InvariantCulture, new CultureInfo("nl-BE")));

stephentoub · 2016-09-30T17:47:26Z

+			public override string EnglishName {
+				get { return "english"; }
+			}
+		}


Nit: things like this can be simplified with expression-bodied members, e.g.

public override string EnglishName => "english";

stephentoub · 2016-09-30T17:47:53Z

+{
+	public class InstanceDescriptorTest {
+
+		private const string url = "http://www.mono-project.com/";


Nit: consider changing

justinvp · 2016-09-30T17:55:47Z

+	public class BindingListTest
+	{
+		[Fact]
+		public void BindingListDefaults ()


Can we cleanup the formatting to match the CoreFX coding style? Mono code style places a space before parens, which would ideally be removed here. Applies throughout.

justinvp · 2016-09-30T17:56:59Z

+using System.ComponentModel.Design.Serialization;
+using System.Collections.Generic;
+using Xunit;
+using System.Reflection;


Nit: "Remove and Sort usings". Haven't looked at the other files, but applies to those too, if needed.

justinvp · 2016-09-30T21:14:42Z

@@ -0,0 +1,681 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// See the LICENSE file in the project root for more information. 
+


Nit: Why the blank line?

I wanted to have a visual differentiator between the license header and the slightly different copyright line added.

Changed this one more time :). Thanks for reviewing!

AlexGhiondea · 2016-09-30T23:21:16Z

@dotnet-bot please test Innerloop CentOS7.1 Debug Build and Test (exit code 139)

justinvp · 2016-10-02T16:38:11Z

+            Assert.Equal(CultureInfo.InvariantCulture, c);
+        }
+
+        [ActiveIssue(11611, Xunit.PlatformID.AnyUnix)]


PlatformID has been renamed TestPlatforms. It would be good if it was renamed here before merging. See #12284.

e.g. [ActiveIssue(11611, TestPlatforms.AnyUnix)]

justinvp · 2016-10-02T16:38:25Z

+                "nl-B");
+        }
+
+        [ActiveIssue(11611, Xunit.PlatformID.AnyUnix)]


PlatformID has been renamed TestPlatforms. It would be good if it was renamed here before merging. See #12284.

e.g. [ActiveIssue(11611, TestPlatforms.AnyUnix)]

justinvp · 2016-10-02T16:39:14Z

+            Assert.True(converter.CanConvertTo(typeof(InstanceDescriptor)));
+        }
+
+        [ActiveIssue(11611, Xunit.PlatformID.AnyUnix)]


PlatformID has been renamed TestPlatforms. It would be good if it was renamed here before merging. See #12284.

e.g. [ActiveIssue(11611, TestPlatforms.AnyUnix)]

Thank you! Will do!

AlexGhiondea · 2016-10-03T18:04:05Z

@dotnet-bot please test Innerloop CentOS7.1 Debug Build and Test (machine setup issue)

AlexGhiondea · 2016-10-04T03:32:31Z

@justinvp, @stephentoub let me know if you have any more feedback!

stephentoub · 2016-10-04T15:12:16Z

Thanks, Alex. I don't need to review again.

justinvp · 2016-10-04T16:13:53Z

Same here. Thanks.

AlexGhiondea · 2016-10-05T00:02:08Z

Argh...

These were ported over from Mono.

AlexGhiondea · 2016-10-05T16:27:13Z

@dotnet-bot please test Innerloop CentOS7.1 Release Build and Test (machine issue)

AlexGhiondea · 2016-10-05T16:27:46Z

@dotnet-bot please test Innerloop CentOS7.1 Debug Build and Test (machine issue)

…oSystemComponentModel Add tests for System.ComponentModel Commit migrated from dotnet/corefx@795777a

dnfclas added the cla-already-signed label Sep 30, 2016

stephentoub reviewed Sep 30, 2016

View reviewed changes

justinvp reviewed Sep 30, 2016

View reviewed changes

AlexGhiondea force-pushed the AddingTestsToSystemComponentModel branch from 6ae2579 to 356f2fd Compare September 30, 2016 22:02

justinvp reviewed Oct 2, 2016

View reviewed changes

justinvp mentioned this pull request Oct 2, 2016

Remove Xunit.PlatformID enum dotnet/buildtools#1084

Merged

AlexGhiondea force-pushed the AddingTestsToSystemComponentModel branch from 356f2fd to 1ac157b Compare October 3, 2016 17:48

AlexGhiondea force-pushed the AddingTestsToSystemComponentModel branch 2 times, most recently from ad2dfa5 to 3b8b25a Compare October 4, 2016 23:06

AlexGhiondea force-pushed the AddingTestsToSystemComponentModel branch from 79f7595 to 3b8b25a Compare October 5, 2016 00:02

Add tests for System.ComponentModel

7564675

These were ported over from Mono.

AlexGhiondea force-pushed the AddingTestsToSystemComponentModel branch from 3b8b25a to 7564675 Compare October 5, 2016 00:03

AlexGhiondea merged commit 795777a into dotnet:master Oct 5, 2016

AlexGhiondea deleted the AddingTestsToSystemComponentModel branch October 5, 2016 17:44

karelz modified the milestone: 1.2.0 Dec 3, 2016

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#12238 from AlexGhiondea/AddingTestsT…

2988106

…oSystemComponentModel Add tests for System.ComponentModel Commit migrated from dotnet/corefx@795777a

		@@ -0,0 +1,681 @@
		// Licensed to the .NET Foundation under one or more agreements.
		// See the LICENSE file in the project root for more information.

Conversation

AlexGhiondea commented Sep 30, 2016

Uh oh!

AlexGhiondea commented Sep 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexGhiondea commented Sep 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexGhiondea commented Oct 3, 2016

Uh oh!

AlexGhiondea commented Oct 4, 2016

Uh oh!

stephentoub commented Oct 4, 2016

Uh oh!

justinvp commented Oct 4, 2016

Uh oh!

AlexGhiondea commented Oct 5, 2016

Uh oh!

AlexGhiondea commented Oct 5, 2016

Uh oh!

AlexGhiondea commented Oct 5, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone