Added Span.SequenceCompareTo tests for types bool, char, int, long and string by sputier · Pull Request #28320 · dotnet/corefx

sputier · 2018-03-21T16:18:12Z

This PR resolves #28118 by adding more unit tests for the MemoryExtensions.SequenceCompareTo function.
It includes new tests for the following types : bool, char, int, long and string.

Hope this first PR will match your expectations 😄

danmoseley · 2018-03-21T16:30:50Z

src/System.Memory/tests/Span/SequenceCompareTo.string.cs

+            {
+                for (int mismatchIndex = 0; mismatchIndex < length; mismatchIndex++)
+                {
+                    string[] first = new string[length];


Nit, it's optional, but we usually use var on the left side when the right side is a constructor.

Not a problem 😄 My last commit addresses this remark.

…ructor

ahsonkhan · 2018-03-22T11:39:13Z

src/System.Memory/tests/Span/SequenceCompareTo.int.cs

+    public static partial class SpanTests
+    {
+        [Fact]
+        public static void ZeroLengthSequenceCompareTo_Int()


We already have some T = int tests in this file:
https://github.com/dotnet/corefx/blob/master/src/System.Memory/tests/Span/SequenceCompareTo.T.cs#L12

Can you please consolidate these (i.e. remove the redundant integer specific tests from SequenceCompare.T.cs)?

ahsonkhan · 2018-03-22T11:40:43Z

src/System.Memory/tests/Span/SequenceCompareTo.string.cs

+    public static partial class SpanTests
+    {
+        [Fact]
+        public static void ZeroLengthSequenceCompareTo_String()


Can you move these tests (for T = string) into SequenceCompareTo.T.cs? That is where we tend to have tests for reference types like string, i.e. T != primitive numeric types (such as byte, int, long).

ahsonkhan · 2018-03-22T11:41:59Z

src/System.Memory/tests/System.Memory.Tests.csproj

  </ItemGroup>
  <ItemGroup>
    <Compile Include="AllocationHelper.cs" />
+    <Compile Include="Span\SequenceCompareTo.bool.cs" />


nit: Please sort these alphabetically and move them to the ItemGroup with the rest of the Span tests, here:

corefx/src/System.Memory/tests/System.Memory.Tests.csproj

Line 63 in 5f4e97d

<Compile Include="Span\SequenceCompareTo.byte.cs" />

ahsonkhan · 2018-03-22T11:47:19Z

src/System.Memory/tests/Span/SequenceCompareTo.bool.cs

+
+namespace System.SpanTests
+{
+    public static partial class SpanTests


We also need test coverage for ReadOnlySpan.

The tests should look similar to the ones you just added here, except call SequenceCompareTo on ReadOnlySpan<T> instead of Span<T>.

It also looks like we need to port over the existing byte specific tests from Span to ReadOnlySpan (copy and modify).

corefx/src/System.Memory/tests/Span/SequenceCompareTo.byte.cs

Line 12 in 5f4e97d

public static void ZeroLengthSequenceCompareTo_Byte()

corefx/src/System.Memory/tests/Span/SequenceCompareTo.T.cs

Line 12 in 5f4e97d

public static void ZeroLengthSequenceCompareTo()

See other ReadOnlySpanTests if you have any questions:

corefx/src/System.Memory/tests/ReadOnlySpan/StartsWith.byte.cs

Line 9 in 5f4e97d

public static partial class ReadOnlySpanTests

If you add the ROS tests, please add the cs files to the appropriate ItemGroup -

corefx/src/System.Memory/tests/System.Memory.Tests.csproj

Line 128 in 5f4e97d

<Compile Include="ReadOnlySpan\SequenceEqual.byte.cs" />

Thanks!

My last commit addresses the ROS tests. Hope everything will be alright !

ahsonkhan · 2018-03-22T11:53:35Z

src/System.Memory/tests/Span/SequenceCompareTo.bool.cs

+            var a = new bool[3];
+
+            var first = new Span<bool>(a, 1, 0);
+            var second = new Span<bool>(a, 2, 0);


nit: If you will notice, the second Span gets implicitly casted to a ReadOnlySpan when we call SequenceCompareTo. Consider changing the type to ReadOnlySpan explicitly:

var second = new ReadOnlySpan<bool>(a, 2, 0);

This way we are consistent with other tests, like SequenceCompareToWithSingleMismatch_Bool https://github.com/dotnet/corefx/pull/28320/files#diff-b162e342a6ea095c71071f76c985a098R91

ahsonkhan · 2018-03-22T22:21:50Z

src/System.Memory/tests/ReadOnlySpan/SequenceCompareTo.int.cs

+        [Fact]
+        public static void SameSpanSequenceCompareTo_Int()
+        {
+            int[] a = { 4, 5, 6 };


nit: I realize we have similar code elsewhere, but can we use values that are larger than byte (i.e. larger than 255), and that can only fit into an integer, going forward?

Random values somewhere between Int16.MaxValue (32767) and Int32.MaxValue (2147483647) would be best.

See recently added SequenceEqual.long as an example: https://github.com/dotnet/corefx/blob/master/src/System.Memory/tests/Span/SequenceEqual.long.cs#L34

ahsonkhan · 2018-03-22T22:22:07Z

src/System.Memory/tests/ReadOnlySpan/SequenceCompareTo.long.cs

+        [Fact]
+        public static void SameSpanSequenceCompareTo_Long()
+        {
+            long[] a = { 4, 5, 6 };


Same here. Use long[] a = { 488238291, 52498989823, 619890289890 };

ahsonkhan

Looks great. Thanks for taking this on and congrats on your first contribution :)

sputier · 2018-03-22T22:43:05Z

I just pushed the changes you requested regarding the test values. Thanks you for your valuable comments and help 😄 I hope I can contribute even more to this awesome project ! 🎉

ahsonkhan · 2018-03-22T23:06:24Z

src/System.Memory/tests/Span/SequenceCompareTo.int.cs

+        [Fact]
+        public static void SameSpanSequenceCompareTo_Int()
+        {
+            int[] a = { 4, 5, 6 };


I just pushed the changes you requested regarding the test values.

We should make the same changes to the Span tests too (for int/long), just like ROSpan.

Oh ! It seems I commited too quickly. It will be OK in a few minutes !

ahsonkhan · 2018-03-22T23:18:20Z

Unrelated test failure:

OSX.1012.Amd64.Open-x64-Debug
System.Net.NetworkInformation.Tests.PingTest/SendPingAsyncWithHostAndTimeoutAndBuffer_Unix
https://mc.dot.net/#/user/sputier/pr~2Fjenkins~2Fdotnet~2Fcorefx~2Fmaster~2F/test~2Ffunctional~2Fcli~2F/aed8e7936300a0290bb29844f967876525ca9ffb/workItem/System.Net.Ping.Functional.Tests/analysis/xunit/System.Net.NetworkInformation.Tests.PingTest~2FSendPingAsyncWithHostAndTimeoutAndBuffer_Unix

Unhandled Exception of Type System.Net.NetworkInformation.PingException
Message :
System.Net.NetworkInformation.PingException : An exception occurred during a Ping request.
---- System.Net.NetworkInformation.PingException : An exception occurred during a Ping request.
Stack Trace :
   at System.Net.NetworkInformation.Ping.GetAddressAndSendAsync(String hostNameOrAddress, Int32 timeout, Byte[] buffer, PingOptions options) in /Users/dotnet-bot/j/workspace/dotnet_corefx/master/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/src/System.Net.Ping/src/System/Net/NetworkInformation/Ping.cs:line 364
   at System.Net.NetworkInformation.Tests.PingTest.SendPingAsync(Func`2 sendPing, Action`1 pingResultValidator) in /Users/dotnet-bot/j/workspace/dotnet_corefx/master/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/src/System.Net.Ping/tests/FunctionalTests/PingTest.cs:line 464
   at System.Net.NetworkInformation.Tests.PingTest.SendPingAsyncWithHostAndTimeoutAndBuffer_Unix() in /Users/dotnet-bot/j/workspace/dotnet_corefx/master/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/src/System.Net.Ping/tests/FunctionalTests/PingTest.cs:line 278
--- End of stack trace from previous location where exception was thrown ---
----- Inner Stack Trace -----
   at System.Net.NetworkInformation.Ping.SendWithPingUtility(IPAddress address, Byte[] buffer, Int32 timeout, PingOptions options) in /Users/dotnet-bot/j/workspace/dotnet_corefx/master/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/src/System.Net.Ping/src/System/Net/NetworkInformation/Ping.Unix.cs:line 210
   at System.Net.NetworkInformation.Ping.SendPingAsyncCore(IPAddress address, Byte[] buffer, Int32 timeout, PingOptions options) in /Users/dotnet-bot/j/workspace/dotnet_corefx/master/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/src/System.Net.Ping/src/System/Net/NetworkInformation/Ping.Unix.cs:line 31
   at System.Net.NetworkInformation.Ping.GetAddressAndSendAsync(String hostNameOrAddress, Int32 timeout, Byte[] buffer, PingOptions options) in /Users/dotnet-bot/j/workspace/dotnet_corefx/master/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/src/System.Net.Ping/src/System/Net/NetworkInformation/Ping.cs:line 353

ahsonkhan · 2018-03-22T23:21:59Z

Filed an issue for this, potentially intermittent, CI failure: https://github.com/dotnet/corefx/issues/28390

I am sure it has nothing to do with this change/PR.

ahsonkhan · 2018-03-23T01:45:52Z

@dotnet-bot test OSX x64 Debug Build
@dotnet-bot test Windows x64 Debug Build

…nd string (dotnet/corefx#28320) * Added SequenceCompareTo tests for types bool, char, int, long and string * Removed a TraitAttribute * Changed explicit type names to 'var' when on the left side of a constructor * Removed redundants tests in SequenceCompareTo.T * Moved tests from SequenceCompareTo.String.cs to SequenceCompareTo.T.cs * Moved <Compile> items to the correct ItemGroup * Change some Span to ReadOnlySpan * Added ReadOnlySpan.SequenceCompareTo tests * Changed test values to match int & long types. * Changed test values to match int & long types. Commit migrated from dotnet/corefx@84e257e

sputier and others added 2 commits March 21, 2018 00:51

Added SequenceCompareTo tests for types bool, char, int, long and string

d881f87

Removed a TraitAttribute

9aa6ffb

danmoseley reviewed Mar 21, 2018

View reviewed changes

Changed explicit type names to 'var' when on the left side of a const…

7718fcd

…ructor

ahsonkhan added the area-System.Memory label Mar 22, 2018

ahsonkhan reviewed Mar 22, 2018

View reviewed changes

sputier added 5 commits March 22, 2018 21:42

Removed redundants tests in SequenceCompareTo.T

422c58b

Moved tests from SequenceCompareTo.String.cs to SequenceCompareTo.T.cs

b57a652

Moved <Compile> items to the correct ItemGroup

631333e

Change some Span to ReadOnlySpan

0fb040a

Added ReadOnlySpan.SequenceCompareTo tests

35e3fbd

ahsonkhan reviewed Mar 22, 2018

View reviewed changes

ahsonkhan approved these changes Mar 22, 2018

View reviewed changes

Changed test values to match int & long types.

aed8e79

ahsonkhan reviewed Mar 22, 2018

View reviewed changes

Changed test values to match int & long types.

4750388

ahsonkhan approved these changes Mar 22, 2018

View reviewed changes

stephentoub merged commit 84e257e into dotnet:master Mar 23, 2018

karelz added this to the 2.1.0 milestone Mar 27, 2018

This was referenced Jan 31, 2020

Test failed: System.Net.NetworkInformation.Tests.PingTest / SendPingAsyncWithHostAndTimeoutAndBuffer_Unix dotnet/runtime#25589

Closed

LoadedFiles_NonStaticallyReferencedAssembly fails in CI with NotEmptyException dotnet/runtime#25592

Closed

Conversation

sputier commented Mar 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Mar 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Mar 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Mar 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Mar 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ahsonkhan left a comment

Choose a reason for hiding this comment

Uh oh!

sputier commented Mar 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan commented Mar 22, 2018

Uh oh!

ahsonkhan commented Mar 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahsonkhan commented Mar 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ahsonkhan Mar 22, 2018 •

edited

Loading

ahsonkhan Mar 22, 2018 •

edited

Loading

ahsonkhan Mar 22, 2018 •

edited

Loading

ahsonkhan Mar 22, 2018 •

edited

Loading

ahsonkhan commented Mar 22, 2018 •

edited

Loading