Runtime: Consolidate the string and span tests for the string-like APIs

[ahsonkhan]

See https://github.com/dotnet/corefx/pull/27319#discussion_r170049466 for additional details and context.

This way, all the tests will be in one place making it easier to maintain, rather than duplicated between assemblies. Consider moving them to Common as well and reference from System.Memory.csproj so that running System.Memory tests will run all the span related tests.

cc @tarekgh

[tarekgh]

why didn't you mark this one for 2.1?

[ahsonkhan]

why didn't you mark this one for 2.1?

It did not seem very crucial, specifically for 2.1, given it is about moving tests around (doesn't bring up a test coverage gap, etc.). If you feel differently, please feel free to change it.

[ahsonkhan]

Partially fixed in https://github.com/dotnet/corefx/pull/28347

Leftover for future:

• Remove redundant span tests from System.Memory that were initially ported over from StringTests.
• Refactor the StringTests.cs to isolate these newly added span-based tests into a separate file (possibly moving them to Common) and reference them from System.Memory.Tests.csproj so that we can execute all relevant Span tests when running msbuild on S.M.T, rather than having to run tests from several places.
  
  
  
  • This will likely result in some code duplication.
  
  

[MarcoRossignoli]

@ahsonkhan the subject is the same?I could help with this.

[ahsonkhan]

Go for it. Let me know if you have any questions along the way.

This is what is required: https://github.com/dotnet/corefx/issues/28238#issuecomment-375163541

[MarcoRossignoli]

Refactor the StringTests.cs to isolate these newly added span-based tests into a separate file (possibly moving them to Common) and reference them from System.Memory.Tests.csproj so that we can execute all relevant Span tests when running msbuild on S.M.T, rather than having to run tests from several places.
This will likely result in some code duplication.

Do you mean create StringTests.Span.cs(public partial class StringTestsSpan : RemoteExecutorTestBase) under src\Commontests\Tests\System and add reference to System.Memory.Tests and System.Runtime.Tests?

Remove redundant span tests from System.Memory that were initially ported over from StringTests.

Do you mean remove tests from src\System.Memorytests\Span(Span<char>) that are already present on src\System.Runtimetests\System\StringTests.cs?If so, there are exactly same method name or i need to read every test code and try to find if there is same test on StringTests.cs?

cc: @ahsonkhan

[MarcoRossignoli]

@ahsonkhan ping for my questions https://github.com/dotnet/corefx/issues/28238#issuecomment-383275980

[ahsonkhan]

Do you mean create StringTests.Span.cs(public partial class StringTestsSpan : RemoteExecutorTestBase) under src\Commontests\Tests\System and add reference to System.Memory.Tests and System.Runtime.Tests?

Yes.

Do you mean remove tests from src\System.Memorytests\Span(Span<char>) that are already present on src\System.Runtimetests\System\StringTests.cs?If so, there are exactly same method name or i need to read every test code and try to find if there is same test on StringTests.cs?

Yes, remove the ones already moved to StringTestsSpan that you create in Common. Most have the same name, but some may not.

Let me give you an example. The following tests are essentially identical (and have the same name). The one in System.Memory is specific for spans only. The one in System.Runtime has both span and string. We should keep one copy of the input data and test (and move it to common) so that we only have to update it in one place.
https://github.com/dotnet/corefx/blob/master/src/System.Memory/tests/ReadOnlySpan/IndexOf.charSpan.cs#L62
https://github.com/dotnet/corefx/blob/master/src/System.Runtime/tests/System/StringTests.cs#L1359

Other examples: StartsWith, EndsWith, CompareTo, Contains, Equals, ToLower, ToUpper,

[ahsonkhan]

This is partially completed by https://github.com/dotnet/corefx/pull/29601. We still need to move some of the duplicated tests from S.M.T to StringTests.

[MarcoRossignoli]

@ahsonkhan do you like something like this?
I mean, move all possible Span<char> under ReadOnlySpan adding string test part, am I on the right track(before we go any further)?

[ahsonkhan]

@ahsonkhan do you like something like this?
I mean, move all possible Span<char> under ReadOnlySpan adding string test part, am I on the right track?

That looks good to me. In your example, this added a new string-based tests as well (borrowing from span), which is great. If in the process of moving the ReadOnlySpan<char> tests, you come across existing string-based tests, I would merge them together (just like you have it in your commit, and how we have existing stringtests today). Thanks.

[MarcoRossignoli]

let me know if there are other works to do on this!

[ahsonkhan]

Looks like this work item is done. All the duplicate span tests were moved to Common StringTests. Thanks @MarcoRossignoli

@tarekgh, can you confirm?

[tarekgh]

I think we are good here. we can revisit if we see anything missing later.