-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Modify the test case Runtime_40444.cs (release/5.0) #45431
Conversation
* Modify the test case Runtime_40444.cs 1) Increase the maximum loop count for failure to 1000 million 2) Added additional computation inside of loop 3) Add addition message when the loop executes to the limit and the test case fails Updated test case and insured that it fails on JITS without the fix Test now checks it the other thread set t2_finished * Re-enable the Runtime_40444 test * Fix typos
@@ -1,66 +1,154 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Extra new line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to just leave this matching the existing test in master.
Port of #40951 to 5.0. @danmosemsft since this just is a test fix, any extra scrutiny needed before a merge? |
Manual run of jit-stress confirms that it is green: https://dev.azure.com/dnceng/public/_build/results?buildId=906223&view=results |
@jkotas @AndyAyersMS @danmosemsft @jeffschwMSFT So now I need the approval to check this into release/5.0 |
@@ -76,9 +76,6 @@ | |||
<ExcludeList Include="$(XunitTestBinBase)/GC/Scenarios/muldimjagary/muldimjagary/*"> | |||
<Issue>https://github.com/dotnet/runtime/issues/5933</Issue> | |||
</ExcludeList> | |||
<ExcludeList Include="$(XunitTestBinBase)/JIT/Regression/JitBlue/Runtime_40444/*"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to fix and re-enable this specific test in servicing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I see that people are seeing the test failing in release/5.0: #44831 (comment)
The test should not be running in release/5.0 in the first place because it is disabled. Why is the test running in release/5.0?
I think we should rather fix the infrastructure problem that is causing this test to run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That line is incorrect and didn't actually disable the test.
This is what the line would need to be changed to actually disable the test:
<ExcludeList Include="$(XunitTestBinBase)/JIT/Regression/JitBlue/Runtime_40444/Runtime_40444/*">
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved. Please work through cr feedback and then we can take this as part of tell mode.
@jkotas Because Jit Stress testing was turned in release/5.0 on sometime after the fork. Originally when I learned of this issue shortly after the fork, I verified that we were not running Jit stress testing jobs in the release/5.0 branch. But since then these jobs have since been turned on and this occasionally test fails. So an issue was opened to fix it and I ported this change over from master. |
Updated test case and insured that it fails on JITS without the fix
Test now checks it the other thread set t2_finished
Re-enable the Runtime_40444 test
Fix typos