Changed all tests using random to use a seed in JIT/Methodical

[ADustyOldMuffin]

Addresses #7756

I found all of the tests inside of the JIT - Methodical folders using Random() without a seed and put the code referenced in the thread in place to put a seed based on the CORECLR_SEED environment variable, or to just use a default seed.

Please let me know if I should include more tests in this PR, I found some more in the JIT folder but they use Random() in a different way so I'm unsure.

This is my first PR to Dotnet so any and all feedback is welcome and appreciated!

[dnfadmin]

All CLA requirements met.

[BruceForstall]

@dotnet/jit-contrib

[JulieLeeMSFT]

@ADustyOldMuffin Thanks for your first contribution to dotnet!

[AndyAyersMS]

Runtime failures are unrelated .

[ADustyOldMuffin]

Runtime failures are unrelated .

Thank you, I had looked through the logs and didn't think I could find anything related but I wasn't 100% if I was interpreting them correctly.

[AndyAyersMS]

There are two failures.

The Linux arm64 checked failure is a long-standing failure in the xunit harness that orchestrates running the tests (#11063)

Unhandled exception. System.InvalidOperationException: Collection was modified after the enumerator was instantiated.
at System.Collections.Generic.Stack`1.Enumerator.MoveNext()
at Xunit.Sdk.DisposalTracker.Dispose() in C:\Dev\xunit\xunit\src\xunit.execution\Sdk\DisposalTracker.cs:line 26
at Xunit.Sdk.ExtensibilityPointFactory.Dispose() in C:\Dev\xunit\xunit\src\xunit.execution\Sdk\ExtensibilityPointFactory.cs:line 53
at Xunit.Sdk.TestFramework.Dispose() in C:\Dev\xunit\xunit\src\xunit.execution\Sdk\Frameworks\TestFramework.cs:line 52
at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__140_1(Object state)
at System.Threading.QueueUserWorkItemCallbackDefaultContext.Execute()
at System.Threading.ThreadPoolWorkQueue.Dispatch()
at System.Threading.PortableThreadPool.WorkerThread.WorkerThreadStart()

The windows arm64 failure is an infrastructure problem in the post-processing done after the tests finished (successfully)

 Retrying (Retry(total=4, connect=None, read=None, redirect=None, status=None)) after connection broken by 'NewConnectionError('<pip._vendor.urllib3.connection.VerifiedHTTPSConnection object at 0x7f8ee97cc0>: Failed to establish a new connection: [Errno -3] Temporary failure in name resolution',)': /simple/azure-devops/

I restarted the runtime-dev-innerloop failed runs since those CI legs got cancelled before they could finish.

[sandreenko]

The changes look good, thanks, Daniel.

I found all of the tests inside of the JIT - Methodical folders using Random() without a seed and put the code referenced in the thread in place to put a seed based on the CORECLR_SEED environment variable, or to just use a default seed.

Are you planning to fix the tests outside of Jit/Methodical? Grep for new Random() shows me 124 files but many of them are HardwareIntrinsics and are auto-generated, so you would need to change only one file in order to fix them but you will need to find out where the template definition for TestLibrary.Generator.Get*() is located, cc @tannergooding

runtime/src/tests/JIT/HardwareIntrinsics/X86/Avx/ConvertToVector128Int32.Double.cs

Line 102 in 01b7e73

varrandom=newRandom();

[tannergooding]

The templates for the hardware intrinsics live in:

• https://github.com/dotnet/runtime/tree/main/src/tests/JIT/HardwareIntrinsics/Arm/Shared
• https://github.com/dotnet/runtime/tree/main/src/tests/JIT/HardwareIntrinsics/General/Shared
• https://github.com/dotnet/runtime/tree/main/src/tests/JIT/HardwareIntrinsics/X86/Shared

As best as I can tell, ConvertToVector128Int32 is not actually being generated from a template. The metadata entry is missing from https://github.com/dotnet/runtime/blob/main/src/tests/JIT/HardwareIntrinsics/X86/Shared/GenerateTests.csx.
So I believe it was either removed, accidentally not checked in, or the file was copied and updated manually.

[ADustyOldMuffin]

Thanks I'll take a look and try and grab the rest then in JIT.

[ADustyOldMuffin]

@sandreenko I made changes to the rest of the tests I found in JIT.

For the template files I couldn't find any of the calls to random, but I have also never used templates for generation so it's quite possible I don't know what I'm looking for. I might need some help in regards to those.

[sandreenko]

looks like some changes are causing test failures, like you changed the seed in "src/tests/JIT/Performance/CodeQuality/Span/Indexer.cs" from 42 to 20010415 and now it fails with:

 Raw output file: C:\h\w\9FD90917\w\B9B10A34\e\JIT\Performance\Reports\JIT.Performance\CodeQuality\Span\Indexer\Indexer.output.txt
Raw output:
BEGIN EXECUTION
"C:\h\w\9FD90917\p\corerun.exe" Indexer.dll Running as correctness test: 1 iterations
pass -bench for benchmark mode w/default iterations
pass [#iterations] for benchmark mode w/iterations
**** Span known size bounds check elimination ****
KnownSizeCtor(1024): 0.05ms -- failed to validate, got 53 expected 70
KnownSizeCtor2(1024): 0.00ms -- failed to validate, got 53 expected 70

looks like these are bad written tests where the result depends on what Random.Next() gives us, could you please revert the changes there and try again?

JIT\Performance\CodeQuality\Span\Indexer\Indexer.cmd
JIT\SIMD\CircleInConvex_ro\CircleInConvex_ro.cmd

[ADustyOldMuffin]

Certainly, I've been slowly working through them sorry for the speed.

…

On Mon, Apr 19, 2021 at 7:46 PM Sergey Andreenko ***@***.***> wrote: looks like some changes are causing test failures, like you changed the seed in "src/tests/JIT/Performance/CodeQuality/Span/Indexer.cs" from 42 to 20010415 and now it fails with: Raw output file: C:\h\w\9FD90917\w\B9B10A34\e\JIT\Performance\Reports\JIT.Performance\CodeQuality\Span\Indexer\Indexer.output.txt Raw output: BEGIN EXECUTION "C:\h\w\9FD90917\p\corerun.exe" Indexer.dll Running as correctness test: 1 iterations pass -bench for benchmark mode w/default iterations pass [#iterations] for benchmark mode w/iterations **** Span known size bounds check elimination **** KnownSizeCtor(1024): 0.05ms -- failed to validate, got 53 expected 70 KnownSizeCtor2(1024): 0.00ms -- failed to validate, got 53 expected 70 looks like these are bad written tests where the result depends on what Random.Next() gives us, could you please revert the changes there and try again? JIT\Performance\CodeQuality\Span\Indexer\Indexer.cmd JIT\SIMD\CircleInConvex_ro\CircleInConvex_ro.cmd — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#50767 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJ3ZJWMVAJYPRDBZWPTV3HDTJTFHFANCNFSM42OACTWQ> .

[sandreenko]

Certainly, I've been slowly working through them sorry for the speed.

No worries, thank you for working on this issue!

[ADustyOldMuffin]

I made changes to the two tests that were failing, but I'm not sure if they're alright changes. I just check that the tests don't return 0 as they shouldn't be 0 at any time. Past that the only fix would be to not have them use random.

I also don't think that the build failures have anything to do with these changes.

[sandreenko]

/azp run runtime-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[sandreenko]

I made changes to the two tests that were failing, but I'm not sure if they're alright changes. I just check that the tests don't return 0 as they shouldn't be 0 at any time. Past that the only fix would be to not have them use random.

I would prefer to revert the changes in these 2 tests, I think it would align with the original issue:

It would be nice to use similar code in place of all tests using Random() without a seed.

these tests were using Random with a seed so they should not be changed.

[ADustyOldMuffin]

Ah apologies I misunderstood, reverted the changes to those tests.

[sandreenko]

/azp run runtime-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[sandreenko]

/azp run runtime-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[sandreenko]

LGTM, thanks @ADustyOldMuffin for your continuous effort to make this change! I will be glad to see more changes from you.