Add more array.sort test combination

[DDolG]

Fixes: #41376

*Added tests for sorting arrays

*Separated tests for integer from string

*Added comments for integers and strings

EDIT (@krwq): Added Fixes ...

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

[dnfadmin]

All CLA requirements met.

[krwq]

@DDolG I assume this is fixing #41376 ? If so can you modify your PR description so that first line says "Fixes #41376" (this way merging PR automatically closes the issue)

[DDolG]

@DDolG I assume this is fixing #41376 ? If so can you modify your PR description so that first line says "Fixes #41376" (this way merging PR automatically closes the issue)

I assume this is fixing #41376 ? Yes.
Thank you very much.
Thank you very much for your last comment. Very concise and understandable. I am very grateful to You.

[DDolG]

@DDolG I assume this is fixing #41376 ? If so can you modify your PR description so that first line says "Fixes #41376" (this way merging PR automatically closes the issue)

Krzysztof I am very grateful to you for the feedback on the code.
Krzysztof, please tell me what these 6 spaces are for. I read the coding style guide, it doesn't say anything about it.

[krwq]

this was to align indentations of comments and code so they are the same across the file (in the very end of the coding style doc there is When editing files, keep new code and changes consistent with the style in the files. - not sure why it's in the very end considering it's probably one of the most important style rule 😄)

[DDolG]

Thanks a lot. I began to understand.

[krwq]

I was thinking of good way to check this PR and here are my conclusions:

• this PR does improve coverage
• there is still lots of room for improvement here: i.e. we never hit heap sort cases for large arrays

Here is how you can check what's covered and what's not locally (after running build.cmd -rc Release -s clr+libs at least once from root of the repo):

• go to src\libraries\System.Runtime\tests
• run dotnet test /p:Outerloop=true /p:Coverage=true
• after it's finished running somewhere in the bottom of the output you should see something along the lines of:

  2022-04-26T16:24:27: Writing report file 'd:\src\runtime\artifacts\bin\System.Runtime.Tests\Debug\net7.0-windows\TestResults\report\index.html'
  2022-04-26T16:24:28: Report generation took 8.5 seconds

• open the path from the log, in my case it's d:\src\runtime\artifacts\bin\System.Runtime.Tests\Debug\net7.0-windows\TestResults\report\index.html
• search for System.Array and click on link or alternatively directly open System.Private.CoreLib_Array.html from the same folder
• search for method you'd like to increase coverage for (scroll to the bottom/search for or click on the method name on the right)
• after you're done editing the test re-run the dotnet test .... command to re-generate report (might be a good idea to copy original report first for comparison) - make sure you've actually increased the coverage you planned

If you lack time I'm ok with merging this as is. Please let me know if you plan to further update the PR.

[DDolG]

I was thinking of good way to check this PR and here are my conclusions:

• this PR does improve coverage
• there is still lots of room for improvement here: i.e. we never hit heap sort cases for large arrays

Here is how you can check what's covered and what's not locally (after running build.cmd -rc Release -s clr+libs at least once from root of the repo):

• go to src\libraries\System.Runtime\tests
• run dotnet test /p:Outerloop=true /p:Coverage=true
• after it's finished running somewhere in the bottom of the output you should see something along the lines of:

  2022-04-26T16:24:27: Writing report file 'd:\src\runtime\artifacts\bin\System.Runtime.Tests\Debug\net7.0-windows\TestResults\report\index.html'
  2022-04-26T16:24:28: Report generation took 8.5 seconds

• open the path from the log, in my case it's d:\src\runtime\artifacts\bin\System.Runtime.Tests\Debug\net7.0-windows\TestResults\report\index.html
• search for System.Array and click on link or alternatively directly open System.Private.CoreLib_Array.html from the same folder
• search for method you'd like to increase coverage for (scroll to the bottom/search for or click on the method name on the right)
• after you're done editing the test re-run the dotnet test .... command to re-generate report (might be a good idea to copy original report first for comparison) - make sure you've actually increased the coverage you planned

If you lack time I'm ok with merging this as is. Please let me know if you plan to further update the PR.

I was thinking of good way to check this PR and here are my conclusions:

• this PR does improve coverage
• there is still lots of room for improvement here: i.e. we never hit heap sort cases for large arrays

Here is how you can check what's covered and what's not locally (after running build.cmd -rc Release -s clr+libs at least once from root of the repo):

• go to src\libraries\System.Runtime\tests
• run dotnet test /p:Outerloop=true /p:Coverage=true
• after it's finished running somewhere in the bottom of the output you should see something along the lines of:

  2022-04-26T16:24:27: Writing report file 'd:\src\runtime\artifacts\bin\System.Runtime.Tests\Debug\net7.0-windows\TestResults\report\index.html'
  2022-04-26T16:24:28: Report generation took 8.5 seconds

• open the path from the log, in my case it's d:\src\runtime\artifacts\bin\System.Runtime.Tests\Debug\net7.0-windows\TestResults\report\index.html
• search for System.Array and click on link or alternatively directly open System.Private.CoreLib_Array.html from the same folder
• search for method you'd like to increase coverage for (scroll to the bottom/search for or click on the method name on the right)
• after you're done editing the test re-run the dotnet test .... command to re-generate report (might be a good idea to copy original report first for comparison) - make sure you've actually increased the coverage you planned

If you lack time I'm ok with merging this as is. Please let me know if you plan to further update the PR.

Krzysztof, you are absolutely right.
I'm confused by the moment that I can't see exactly how the sorting goes. I wanted to ask a question about how to check HeapSort. But I hesitated to ask a question. In my opinion, it is desirable to make maximum coverage with tests. I thought for a very long time how to write a test in order to check the HeapSort method. Didn't come up with anything. So I am very grateful to you for your help.

[DDolG]

Unable to start testing with log generation.
I think to allocate sorting in a separate program to add logging there. And then form such a sequence that the HeapSort method is involved. Unfortunately, nothing better comes to mind.
Please tell me, if the sequence is large enough, will it need to be placed in another place?

[krwq]

@DDolG you;re seeing two issues there:

• test failure - most likely caused by timeout (I cannot see the full log to tell which test) - it's possible this is preventing coverage report from being generated - I think this problem might be related to your local changes - try fixing it (or temporarily disabling failing test) and see if report gets generated before you try anything else
• Html Logger Error - I'm not sure what that is but I think I've sometimes seen that locally and I believe it can be ignored

Can you save your local changes try doing git clean -fdx from the root of the repo, rebuild everything (build -rc Release -s clr+libs) and run test again with /p:Coverage=true and see if that still happens?

Please tell me, if the sequence is large enough, will it need to be placed in another place?

I'm not sure what sequence are you referring to. In general debugging through test might help you figure out how to hit specific place in the code (it might not be super trivial because of all of the if statements in the sorting code but it likely will be easier than guessing) - simply open project in VS and on the test explorer window search for test and right click and click Debug (make sure to put a breakpoint in the test first). Also if test is [Theory] you might want to comment out all cases but one since they run in parallel and that might make debugging much harder otherwise

[DDolG]

Thank a lot. Awesome way to check test coverage.
Krzysztof I saw that there are methods that are not covered by tests.
I will try to cover these methods with tests.
Thank you very much for teaching me this technology.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Fixes: #41376

*Added tests for sorting arrays

*Separated tests for integer from string

*Added comments for integers and strings

EDIT (@krwq): Added Fixes ...

Author:	DDolG
Assignees:	-
Labels:	area-System.Runtime, community-contribution
Milestone:	-

[DDolG]

Hello. Help me please. I have the impression that I'm not testing correctly.
I do testing according to the method written by Krzysztof. The output is a report. This report shows that the ArraySortHelper class is not covered by tests. Although if you separately debug each test, you can see that it enters the methods of this class.

System.Private.CoreLib_ArraySortHelper_1.zip

[jeffhandley]

Hello. Help me please. I have the impression that I'm not testing correctly.

Sorry we've not been able to jump back in and help here, @DDolG. Per @krwq's earlier comment, it's OK to merge this as-is without adding further coverage at this time though.

[jeffhandley]

Thanks for the contribution, @DDolG!

[DDolG]

@jeffhandley thanks a lot. I am very grateful to you for the trust you have placed in me.