Get System.ComponentModel.Annotations tests passing with netfx #17874

hughbe · 2017-04-04T10:04:02Z

Commit 1

Fixes a regression from the full .NET framework when porting the library to .net core

Fixes #17872

Commit 2

Nerfs tests for fixed bugs and added features

See #2650, #4465 and #5203

Commit 3

Nerfs tests for .NET core bug. I'm not sure how to fix this one, though!

See #17873

Fixes: #18149
Fixes: #18150
Fixes: #18148
Fixes: #18145
Fixes: #18146
Fixes #18542

Fixed by using the code from referencesource. The RangeAttribute tests now all pass with `msbuild /T:BuildAndTest /P:TargetGroup=netfx`

This is almost certainly a .NET core bug caused by #4319

safern

Left some comments. Thanks for doing this @hughbe

safern · 2017-04-05T05:12:09Z

src/System.ComponentModel.Annotations/tests/MaxLengthAttributeTests.cs

+            // MaxLengthAttribute in the full .NET framework doesn't support ICollection.Count.
+            // See https://github.com/dotnet/corefx/pull/2650".
+            if (!PlatformDetection.IsFullFramework)
+            {


This makes it a little bit confusing and when this gets fixed in Desktop then we would not be testing that and would be "hard" to find why we are not testing it. It would be better to have 2 tests one for netcore and one for netfx where each one receives its supported input and add SkipOnTargetFramework with the reason why it is skipped.

safern · 2017-04-05T05:12:33Z

src/System.ComponentModel.Annotations/tests/MaxLengthAttributeTests.cs

+
+            // MaxLengthAttribute in the full .NET framework doesn't support ICollection.Count.
+            // See https://github.com/dotnet/corefx/pull/2650".
+            if (!PlatformDetection.IsFullFramework)


safern · 2017-04-05T05:14:54Z

src/System.ComponentModel.Annotations/tests/MaxLengthAttributeTests.cs

+        [MemberData(nameof(ValidValues_ICollection))]
+        [MemberData(nameof(InvalidValues_ICollection))]
+        [SkipOnTargetFramework(~TargetFrameworkMonikers.NetFramework, "MaxLengthAttribute in the full .NET framework doesn't support ICollection.Count. See https://github.com/dotnet/corefx/pull/2650")]
+        public void Validate_ICollection_NetFx_ThrowsInvalidCastException(MaxLengthAttribute attribute, object value)


I see you have a Netfx test but I'm missing the netcoreapp tests using this collections.

Sorry I'm not sure I understand (this also applies to the comment about the confusion above)

It/when netfx adds the ICollection feature, then this test that is skipped on non-netfx frameworks (I.e runs for netfx) will fail.

The respective netcoreapp test uses the Invalid/Valid_ICollection_TestData in Valid/InvalidValues()

Maybe I could clarify this with a comment?

So what I meant is that we should have to tests (one for netcoreapp and one for netfx) and split the data Enumerable into to different data sets instead of constructing this data sets in runtime depending on the target that we are running on. While building this sets in runtime it makes it confusing to know which data sets work on netcoreapp and which ones on netfx.

See: #17618

Does this clarify my comment?

safern · 2017-04-05T05:15:34Z

src/System.ComponentModel.Annotations/tests/MinLengthAttributeTests.cs

-            yield return new TestCase(new MinLengthAttribute(16), new Collection<string>(new string[16]));
+            // MinLengthAttribute in the full .NET framework doesn't support ICollection.Count.
+            // See https://github.com/dotnet/corefx/pull/2650".
+            if (!PlatformDetection.IsFullFramework)


Same comment here. Can it be done the other way I mentioned earlier?

safern · 2017-04-05T05:17:21Z

src/System.ComponentModel.Annotations/tests/PhoneAttributeTests.cs

+            // Certain invalid phone numbers are reported as valid with .NET core.
+            // The full .NET framework considers them invalid. This is likely a bug
+            // in .NET core.
+            if (!PlatformDetection.IsFullFramework)


Could we instead disable the tests with ActiveIssueAttribute? since this is likely a bug in .NET Core?

cc: @stephentoub

I'm not sure how to disable individual test data in MemberData, since this is part of a list of tests

I believe we break the test into two (possibly with a shared body) so they can have different member data.

I think In the past we've just commented out the test and added a reference to the issue. I can do that if it helps - would also avoid us having to do a bunch of code movement.

@danmosemsft is that a suggestion or a comment in reply to the question?

What I meant was to disable the test that is failing in netcoreapp due to bug: https://github.com/dotnet/corefx/issues/17873

Instead of making this test pass, as with doing that we would be testing a buggy behavior. So we should add [ActiveIssue(17873)] to the netcoreapp test itself instead of hacking the input to have the "expected" behavior which we claim it to be a bug.

Another approach would be skipping the test in netcoreapp and running it on netfx so that we don't loose test coverage in Desktop.

[EDIT] Actually I think it is better to just skip this test in netcoreapp and link it to the issue tracking this bug.

@hughbe I just meant, if we need to disable a single member data, we've historically made two tests, possibly calling into a single body function, each taking a subset of the member data, and each annotated as necessray. But sure if it's just temporarily because of a .NET core bug, as opposed to some .NET Framework bug etc, perhaps it's not worth it.
I'll let @safern code review this and bow out :)

Actually I think it is better to just skip this test in netcoreapp and link it to the issue tracking this bug.

I went with that

safern

Thanks @hughbe

safern · 2017-04-05T17:34:52Z

I will wait for one of the area owners to be ok with this changes before merging.

danmoseley · 2017-04-07T04:39:00Z

@divega @lajones @ajcvickers look OK?

safern · 2017-04-10T22:54:14Z

ping @divega @lajones @ajcvickers :)

ajcvickers · 2017-04-10T22:57:09Z

@safern We have some time allocated on Wednesday for Data Annotations issues.

lajones · 2017-04-11T21:19:38Z

I'm not sure we should follow this route. Basically what this does is accept that the NETCore and NETFx versions of this code produce different results (for the same input) and updates the tests to expect that. I think we should be fixing the code so that NETCore and NETFx always produce the same thing. the question then becomes should NETCore reflect NETFx or vice versa. To discuss in triage tomorrow.

hughbe · 2017-04-12T02:38:25Z

I suggest NetCore aligns with netfx - this is typically what we've gone with for issues that aren't bugs in the .NET framework. These aren't really bugs - I've fixed the first incompatability with TypeDescriptor.

For features added and bugs fixed in .NET Core (e.g. ICollection, better validation of contexts) I believe we've been OK with nerfing the tests for .NET framework.

ajcvickers

The changes look good. However, we want to make sure that behavior differences between .NET Core and .NET Framework are intentional and still make sense given the evolved understanding of compatibility. Therefore, before we merge this, we are requesting that each behavior difference is tracked by an issue (not a closed PR) such that we can evaluate if we need to make any compat changes to either .NET Core or .NET Framework. I have tried to indicate in code review comments where I think issues are missing.

ajcvickers · 2017-04-13T20:44:16Z

src/System.ComponentModel.Annotations/tests/CustomValidationAttributeTests.cs

-            Assert.Equal(expected, attribute.RequiresValidationContext);
+
+            // The full .NET framework has a bug where CustomValidationAttribute doesn't
+            // validate the context. See https://github.com/dotnet/corefx/pull/5203.


Please file an issue describing this difference in behavior. (#3794 and #5203 don't seem to have any associated issue.)

ajcvickers · 2017-04-13T20:45:51Z

src/System.ComponentModel.Annotations/tests/CustomValidationAttributeTests.cs

@@ -96,14 +106,24 @@ public static IEnumerable<object[]> BadlyFormed_TestData()
        }

        [Theory]
+        [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, ".NET core fixes a bug where CustomValidationAttribute doesn't validate the context. See https://github.com/dotnet/corefx/pull/5203")]


Likewise, please file an issue for this behavior difference.

This is the same as #18360

ajcvickers · 2017-04-13T20:46:06Z

src/System.ComponentModel.Annotations/tests/CustomValidationAttributeTests.cs

        {
            CustomValidationAttribute attribute = new CustomValidationAttribute(validatorType, method);
            Assert.Throws<InvalidOperationException>(() => attribute.RequiresValidationContext);
        }

        [Theory]
+        [SkipOnTargetFramework(~TargetFrameworkMonikers.NetFramework, "The full .NET framework has a bug where CustomValidationAttribute doesn't validate the context. See https://github.com/dotnet/corefx/pull/5203")]


This is the same as #18360

ajcvickers · 2017-04-13T20:51:13Z

src/System.ComponentModel.Annotations/tests/MaxLengthAttributeTests.cs

@@ -58,6 +66,34 @@ public static void Ctor_Int(int length)
        }

        [Theory]
+        [MemberData(nameof(ValidValues_ICollection))]
+        [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "MaxLengthAttribute in the .NET Framework doesn't support ICollection.Count. See https://github.com/dotnet/corefx/pull/2650")]


Likewise for these behavior differences. (#2650 doesn't seem to have an issue associated, unless I missed it.)

hughbe · 2017-04-14T02:06:53Z

src/System.ComponentModel.Annotations/tests/CustomValidationAttributeTests.cs

-            Assert.Equal(expected, attribute.RequiresValidationContext);
+
+            // The full .NET framework has a bug where CustomValidationAttribute doesn't
+            // validate the context. See https://github.com/dotnet/corefx/pull/5203.


hughbe · 2017-04-14T02:07:19Z

src/System.ComponentModel.Annotations/tests/CustomValidationAttributeTests.cs

@@ -96,14 +106,24 @@ public static IEnumerable<object[]> BadlyFormed_TestData()
        }

        [Theory]
+        [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, ".NET core fixes a bug where CustomValidationAttribute doesn't validate the context. See https://github.com/dotnet/corefx/pull/5203")]


This is the same as #18360

hughbe · 2017-04-14T02:07:22Z

src/System.ComponentModel.Annotations/tests/CustomValidationAttributeTests.cs

        {
            CustomValidationAttribute attribute = new CustomValidationAttribute(validatorType, method);
            Assert.Throws<InvalidOperationException>(() => attribute.RequiresValidationContext);
        }

        [Theory]
+        [SkipOnTargetFramework(~TargetFrameworkMonikers.NetFramework, "The full .NET framework has a bug where CustomValidationAttribute doesn't validate the context. See https://github.com/dotnet/corefx/pull/5203")]


This is the same as #18360

hughbe · 2017-04-14T02:09:56Z

src/System.ComponentModel.Annotations/tests/MaxLengthAttributeTests.cs

@@ -58,6 +66,34 @@ public static void Ctor_Int(int length)
        }

        [Theory]
+        [MemberData(nameof(ValidValues_ICollection))]
+        [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "MaxLengthAttribute in the .NET Framework doesn't support ICollection.Count. See https://github.com/dotnet/corefx/pull/2650")]


hughbe · 2017-04-14T02:10:01Z

src/System.ComponentModel.Annotations/tests/MinLengthAttributeTests.cs

@@ -49,6 +56,34 @@ public static void Ctor(int length)
            Assert.Equal(length, new MinLengthAttribute(length).Length);
        }

+        [Theory]
+        [MemberData(nameof(ValidValues_ICollection))]
+        [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "MinLengthAttribute in the .NET Framework doesn't support ICollection.Count. See https://github.com/dotnet/corefx/pull/2650")]


Could you just please add the issue number to the SkipOnTargetFrameworkAttribute so that is easy to track when we look for the tests being skipped?

hughbe · 2017-04-16T05:02:35Z

Friendly Ping?

hughbe · 2017-04-19T00:55:47Z

Rebased to fix a merge conflict

hughbe added 3 commits April 4, 2017 16:50

Fix RangeAttribute validation regression from netfx

a338d75

Fixed by using the code from referencesource. The RangeAttribute tests now all pass with `msbuild /T:BuildAndTest /P:TargetGroup=netfx`

Fix netfx for tests with new features and .net core bug fixes

de5d7ee

Nerf PhoneAttributes tests for netfx

1d77da9

This is almost certainly a .NET core bug caused by #4319

dnfclas added the cla-already-signed label Apr 4, 2017

danmoseley requested a review from safern April 5, 2017 04:41

safern reviewed Apr 5, 2017

View reviewed changes

karelz added the area-System.ComponentModel.DataAnnotations label Apr 5, 2017

karelz assigned hughbe, divega, ajcvickers and lajones Apr 5, 2017

Address PR feedback

4348ea2

safern approved these changes Apr 5, 2017

View reviewed changes

ajcvickers suggested changes Apr 13, 2017

View reviewed changes

hughbe commented Apr 14, 2017

View reviewed changes

Fix names of test skips on netfx

b763128

Merge branch 'master' into componentmodel-netfx

e49c257

ajcvickers approved these changes Apr 19, 2017

View reviewed changes

danmoseley merged commit b372d61 into dotnet:master Apr 19, 2017

hughbe deleted the componentmodel-netfx branch April 19, 2017 03:57

karelz added this to the 2.0.0 milestone Apr 20, 2017

safern mentioned this pull request Apr 25, 2017

Fix System.ComponentModel.Annotations test bug #18886

Merged

Get System.ComponentModel.Annotations tests passing with netfx #17874

Get System.ComponentModel.Annotations tests passing with netfx #17874

Conversation

hughbe commented Apr 4, 2017 • edited Loading

Commit 1

Commit 2

Commit 3

safern left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

safern Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

safern left a comment • edited Loading

Choose a reason for hiding this comment

safern commented Apr 5, 2017

danmoseley commented Apr 7, 2017

safern commented Apr 10, 2017

ajcvickers commented Apr 10, 2017

lajones commented Apr 11, 2017

hughbe commented Apr 12, 2017

ajcvickers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hughbe commented Apr 16, 2017 • edited Loading

hughbe commented Apr 19, 2017

hughbe commented Apr 4, 2017 •

edited

Loading

safern Apr 5, 2017 •

edited

Loading

safern left a comment •

edited

Loading

hughbe commented Apr 16, 2017 •

edited

Loading