-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Add GCStress testing for libraries tests on checked CoreCLR #38235
Add GCStress testing for libraries tests on checked CoreCLR #38235
Conversation
I couldn't figure out the best area label to add to this PR. Please help me learn by adding exactly one area label. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
5ae7c1d
to
a0a379b
Compare
This comment has been minimized.
This comment has been minimized.
a0a379b
to
6b18fa0
Compare
This comment has been minimized.
This comment has been minimized.
Triggered |
ping can anyone take a look? |
ping Maybe someone on @dotnet/jit-contrib can sign off? |
@@ -144,3 +159,18 @@ jobs: | |||
- jitstress2_jitstressregs0x10 | |||
- jitstress2_jitstressregs0x80 | |||
- jitstress2_jitstressregs0x1000 | |||
${{ if in(parameters.coreclrTestGroup, 'gcstress0x3-gcstress0xc') }}: | |||
scenarios: | |||
# Disable gcstress0x3 for now; it causes lots of test timeouts. Investigate this after |
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.
Should we link this to an issue?
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.
Make sense. I'll add one.
<TimeoutInSeconds Condition="'$(Scenario)' == '' and ('$(TargetArchitecture)' == 'arm64' or '$(TargetArchitecture)' == 'arm')">1800</TimeoutInSeconds> | ||
<TimeoutInSeconds Condition="'$(Scenario)' != '' and ('$(TargetArchitecture)' == 'arm64' or '$(TargetArchitecture)' == 'arm')">3600</TimeoutInSeconds> | ||
<TimeoutInSeconds Condition=" | ||
'$(Scenario)' == 'gcstress0x3' or |
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 not use $(Scenario.StartsWith('gcstress')) or '$(Scenario)' == 'heapverify1'
?
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'd prefer to leave it explicit, even through it is more verbose, as it makes it easier to 'grep' for a full scenario name and find every file that references it.
Sorry I missed this. Just reviewed. |
@@ -16,7 +16,7 @@ function print_usage { | |||
echo '' | |||
echo 'Command line:' | |||
echo '' | |||
echo './setup-gcstress.sh --arch=<TargetArch> --outputDir=<coredistools_lib_install_path>' | |||
echo './setup-stress-dependencies.sh --arch=<TargetArch> --outputDir=<coredistools_lib_install_path>' |
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,
echo "$0 --arch=<TargetArch> --outputDir=<coredistools_lib_install_path>"
Disable GCStress0x3 for now, as that causes many timeouts in these tests.
There are currently too many failures in these jobs so we don't want to run them on a schedule yet.
d5c7565
to
9bdf668
Compare
Any final objections? Or sign-off? |
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.
LGTM, thanks Bruce for making these improvements!
Part of this is downloading and copying the stress dependencies (namely, coredistools) into the
testhost.
There are still many issues to address with failures in these tests, so the schedule
is disabled.
Disable GCStress0x3 for now, as that causes many timeouts in these tests.
Fixes #38233