-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Add System.ComponentModel.Annotations tests #20934
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,24 +56,19 @@ public override bool IsValid(object value) | |
{ | ||
return true; | ||
} | ||
var str = value as string; | ||
if (str != null) | ||
if (value is string str) | ||
{ | ||
length = str.Length; | ||
} | ||
else | ||
{ | ||
ICollection collection = value as ICollection; | ||
|
||
if (collection != null) | ||
if (value is ICollection collection) | ||
{ | ||
length = collection.Count; | ||
} | ||
else | ||
{ | ||
// A cast exception previously occurred if a non-{string|array} property was passed | ||
// in so preserve this behavior if the value does not implement ICollection | ||
length = ((Array)value).Length; | ||
throw new InvalidCastException(SR.Format(SR.LengthAttribute_InvalidValueType, value.GetType())); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
As for MaxLengthAttribute: what if it is an array? You're removing functionality here which was added to solve a previous issue. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So that's what I first thought when I saw the code and why I fixed this. All Arrays are ICollections, so this code path will never be hit as they will take the precious if block. We have tests that cover this in the project. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That makes sense - thanks for clarifying. |
||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
// 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.DataAnnotations.Tests | ||
{ | ||
public class DisplayColumnAttributeTests | ||
{ | ||
[Theory] | ||
[InlineData(null)] | ||
[InlineData("")] | ||
[InlineData("DisplayColumn")] | ||
public void Ctor_DisplayColumn(string displayColumn) | ||
{ | ||
var attribute = new DisplayColumnAttribute(displayColumn); | ||
Assert.Equal(displayColumn, attribute.DisplayColumn); | ||
Assert.Null(attribute.SortColumn); | ||
Assert.False(attribute.SortDescending); | ||
} | ||
|
||
[Theory] | ||
[InlineData(null, null)] | ||
[InlineData("", "")] | ||
[InlineData("DisplayColumn", "SortColumn")] | ||
public void Ctor_DisplayColumn_SortColumn(string displayColumn, string sortColumn) | ||
{ | ||
var attribute = new DisplayColumnAttribute(displayColumn, sortColumn); | ||
Assert.Equal(displayColumn, attribute.DisplayColumn); | ||
Assert.Equal(sortColumn, attribute.SortColumn); | ||
Assert.False(attribute.SortDescending); | ||
} | ||
|
||
[Theory] | ||
[InlineData(null, null, false)] | ||
[InlineData("", "", false)] | ||
[InlineData("DisplayColumn", "SortColumn", true)] | ||
public void Ctor_DisplayColumn_SortColumn_SortDescending(string displayColumn, string sortColumn, bool sortDescending) | ||
{ | ||
var attribute = new DisplayColumnAttribute(displayColumn, sortColumn, sortDescending); | ||
Assert.Equal(displayColumn, attribute.DisplayColumn); | ||
Assert.Equal(sortColumn, attribute.SortColumn); | ||
Assert.Equal(sortDescending, attribute.SortDescending); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know for sure that InnerException will always be non-null in all circumstances?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the
try
block:The only place where the TIE can be thrown is from reflection invoke, and there will always be a non-null inner exception in this case.