-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Conversation
@dotnet-bot test ci please |
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.
Looks good to me, did have a few comments/questions.
netci.groovy
Outdated
@@ -1741,6 +1742,11 @@ def static addTriggers(def job, def branch, def isPR, def architecture, def os, | |||
case 'illink': | |||
Utilities.addGithubPRTriggerForBranch(job, branch, "${os} ${architecture} ${configuration} via ILLink", "(?i).*test\\W+${os}\\W+${architecture}\\W+${configuration}\\W+${scenario}.*") | |||
break | |||
case 'corefx_innerloop': | |||
if (configuration == 'Release') { |
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.
Do we want this for release? Generally we prefer checked.
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.
|
||
buildCommands += "python -u %WORKSPACE%\\tests\\scripts\\run-corefx-tests.py -arch ${arch} -ci_arch ${architecture} -build_type ${configuration} -fx_root ${absoluteFxRoot} -fx_branch ${fxBranch} -env_script ${envScriptPath}" | ||
buildCommands += "python -u %WORKSPACE%\\tests\\scripts\\run-corefx-tests.py -arch ${arch} -ci_arch ${architecture} -build_type ${configuration} -fx_root ${absoluteFxRoot} -fx_branch ${fxBranch} -env_script ${envScriptPath}" |
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.
What is the difference between the outerloop and innerloop jobs? Why are we opting for two different ways to run corefx tests?
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.
CoreFx tests have a notion of innerloop and outerloop built into them as well. We should probably only run corefx's innerloop tests in coreclr's innerloop. This naming seems appropriate in case we want to add corefx outerloop jobs at some point. It's unclear right now (to me, at least).
In reply to: 196827178 [](ancestors = 196827178)
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.
Yep, @RussKeldorph was spot on. The CI configuration will only run innerloop (marked as such) CoreFX tests during innerloop CoreCLR jobs.
netci.groovy
Outdated
@@ -2571,6 +2586,12 @@ def static shouldGenerateJob(def scenario, def isPR, def architecture, def confi | |||
return false | |||
} | |||
|
|||
// Run basic corefx tests only on PR-triggered jobs | |||
// Runs only under Release - running tests under Debug is slow |
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.
Same comment about release vs checked
netci.groovy
Outdated
if (os != 'Windows_NT'|| architecture != 'x64') { | ||
return false | ||
} | ||
if(configuration != 'Release') { |
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.
Same comment about release vs checked
@@ -2831,6 +2860,8 @@ Constants.allScenarios.each { scenario -> | |||
|
|||
// Create the new job | |||
def newJob = job(Utilities.getFullJobName(project, jobName, isPR, folderName)) {} | |||
|
|||
// Should we add corefx_innerloop to views? |
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.
Will let @BruceForstall comment
@dotnet-bot test ci please |
* Netci * Add fake test switch * Add branch PR trigger * Add Checked corefx_innerloop jobs Commit migrated from dotnet/coreclr@b575607
Connected to #18365
This updates the ci definition and adds a mock runtest switch.