Test StringComparer.FromComparison#14623
Conversation
|
@AlexRadch the tests look good but they are currently failing because we haven't yet moved to a CoreCLR version that has those new methods. |
|
@AlexRadch thanks for doing this! |
|
The API needs to be exposed in the contract by adding it to src/System.Runtime.Extensions/ref/System.Runtime.Extensions.cs within Also, instead of adding the tests to |
| Assert.Equal(0, StringComparer.InvariantCultureIgnoreCase.Compare("test", "TEST")); | ||
| } | ||
|
|
||
| public static IEnumerable<object[]> FromComparison_TestData() |
There was a problem hiding this comment.
Nit: Could be simplified slightly, e.g.:
public static readonly object[][] FromComparison_TestData =
{
new object[] { StringComparison.CurrentCulture, StringComparer.CurrentCulture },
new object[] { StringComparison.CurrentCultureIgnoreCase, StringComparer.CurrentCultureIgnoreCase },
...
};
|
Another test should be added that ensures |
| public static System.StringComparer Ordinal { get { throw null; } } | ||
| public static System.StringComparer OrdinalIgnoreCase { get { throw null; } } | ||
| #if netcoreapp11 | ||
| public static System.StringComparer FromComparison(System.StringComparison comparison) { throw null; } |
There was a problem hiding this comment.
The parameter should be named comparisonType
| { | ||
| public static object[][] FromComparison_TestData() | ||
| { | ||
| return new object[][] { |
There was a problem hiding this comment.
My suggestion was to turn this into a static read only field, instead of a method. Then you won't need the return statement.
public static readonly object[][] FromComparison_TestData =
{
new object[] { StringComparison.CurrentCulture, StringComparer.CurrentCulture },
new object[] { StringComparison.CurrentCultureIgnoreCase, StringComparer.CurrentCultureIgnoreCase },
// etc.
};| public static object[][] FromComparison_TestData() | ||
| { | ||
| return new object[][] { | ||
| // StringComparison StringComparer |
There was a problem hiding this comment.
Nit: I'd just remove this comment
| { | ||
| public static partial class MiscStringComparerTests | ||
| { | ||
| public static object[][] FromComparison_TestData = new object[][] { |
There was a problem hiding this comment.
Nit: The field should be readonly and new object[][] is unnecessary and can be removed.
| StringComparison maxInvalid = Enum.GetValues(typeof(StringComparison)).Cast<StringComparison>().Max() + 1; | ||
|
|
||
| Assert.Throw<ArgumentException>(() => { StringComparer.FromComparison(minInvalid); }); | ||
| Assert.Throw<ArgumentException>(() => { StringComparer.FromComparison(maxInvalid); }); |
There was a problem hiding this comment.
These should be Assert.Throws<ArgumentException> (Throws plural).
Also, the brackets aren't needed, they can be written as:
Assert.Throws<ArgumentException>(() => StringComparer.FromComparison(minInvalid));
Assert.Throws<ArgumentException>(() => StringComparer.FromComparison(maxInvalid));
| StringComparison maxInvalid = Enum.GetValues(typeof(StringComparison)).Cast<StringComparison>().Max() + 1; | ||
|
|
||
| Assert.Throw<ArgumentException>(() => StringComparer.FromComparison(minInvalid)); | ||
| Assert.Throw<ArgumentException>(() => StringComparer.FromComparison(maxInvalid)); |
There was a problem hiding this comment.
Throws. The name of the xunit method is "Throws" with an "s" at the end, not "Throw".
| {845D2B72-D8A4-42E5-9BE9-17639EC4FC1A}.netstandard1.7_Debug|Any CPU.ActiveCfg = net461_Release|Any CPU | ||
| {845D2B72-D8A4-42E5-9BE9-17639EC4FC1A}.netstandard1.7_Debug|Any CPU.Build.0 = net461_Release|Any CPU | ||
| {845D2B72-D8A4-42E5-9BE9-17639EC4FC1A}.netstandard1.7_Release|Any CPU.ActiveCfg = net461_Release|Any CPU | ||
| {845D2B72-D8A4-42E5-9BE9-17639EC4FC1A}.netstandard1.7_Release|Any CPU.Build.0 = net461_Release|Any CPU |
There was a problem hiding this comment.
Are the changes in this file necessary?
There was a problem hiding this comment.
I think No. I will remove this. My VS created this.
|
@dotnet-bot test this (logs gone) |
|
@mmitche the results all just have Any ideas? Perhaps it just didn't rerun them for some reason. I'll try open/close to do it. |
|
It says the type is not there. Maybe it just arrived. |
|
@dotnet-bot test this (logs gone) |
|
@mmitche the request to retest apparently didn't work as there are still no logs. |
|
@dotnet-bot test this please |
|
@danmosemsft Looks like the phrase is actually test this pease (spelled correctly) not just test this |
|
@dotnet-bot help |
|
Welcome to the dotnet/corefx Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories.
The following jobs are launched by default for each PR against dotnet/corefx:master.
The following optional jobs are available in PRs against dotnet/corefx:master.
Have a nice day! |
|
@mmitche oh - didn't realize as none of hte others need please. maybe better to take that off.. |
|
@danmosemsft Someone at some time had requested that it be added. But yeah we can remove it. |
|
@AlexRadch seems like you need to rebase the changes you made on System.Runtime.Extensions.Tests.csproj, do you mind doing it so that we can re-run the CI? |
|
@AlexRadch ping? |
|
The PR seems to be abandoned for 1 month. Closing for now. |
9 participants
Add API to convert a StringComparison to a StringComparer, to avoid bunch of switch-statements in user code (see 'Motivation' section below).
See https://github.com/dotnet/corefx/issues/13800 and dotnet/coreclr#8633