-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Run test template tests with Arcade #51020
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
base: release/10.0.1xx
Are you sure you want to change the base?
Conversation
0b13233
to
571fd37
Compare
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.
So, while this is a cleaner solution to the existing operation, I'm still confused about the existing operation. This change means that all builds, public PR builds and internal CI builds, will run this secondary build. Isn't the point to get CG to scan the acquired dependencies from the templates? If so, CG doesn't run externally on PRs.
You linked to where the jobs are defined by default for the PR run in the sdk-job-matrix.yml
. But there is a similar section in .vsts-ci.yml
:
Lines 111 to 143 in f684b50
${{ if and(eq(parameters.runTestBuild, false), ne(variables['Build.Reason'], 'PullRequest')) }}: | |
timeoutInMinutes: 180 | |
windowsJobParameterSets: | |
### OFFICIAL ### | |
- categoryName: Official | |
publishArgument: $(_publishArgument) | |
signArgument: $(_signArgument) | |
officialBuildProperties: $(_officialBuildProperties) /p:BuildWorkloads=true | |
enableDefaultArtifacts: true | |
runTests: false | |
publishRetryConfig: true | |
variables: | |
_SignType: real | |
- categoryName: Official | |
targetArchitecture: x86 | |
publishArgument: $(_publishArgument) | |
signArgument: $(_signArgument) | |
officialBuildProperties: $(_officialBuildProperties) | |
runTests: false | |
variables: | |
_SignType: real | |
dependsOn: Official_windows_x64 | |
downloadManifestMsiPackages: true | |
- categoryName: Official | |
targetArchitecture: arm64 | |
publishArgument: $(_publishArgument) | |
signArgument: $(_signArgument) | |
officialBuildProperties: $(_officialBuildProperties) | |
runTests: false | |
variables: | |
_SignType: real | |
dependsOn: Official_windows_x64 | |
downloadManifestMsiPackages: true |
Just make a new job in that section that only runs a build with the desired configuration.
Actually, I'm just gonna make a change to your PR so that we can get stuff moving.
… up invocation of populateInternalRuntimeVariables. Fixed Arcade template folder name paths. Minor formatting changes.
container: azureLinux30Amd64 | ||
helixTargetContainer: $(helixTargetContainerPrefix)ubuntu-24.04-helix-amd64 | ||
osProperties: /p:OSName=linux /p:BuildSdkDeb=true | ||
runTests: true |
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.
This was not necessary as runTests defaults to true.
-restore -test -ci -prepareMachine -nativeToolsOnMachine | ||
-configuration $(buildConfiguration) | ||
/p:Projects=\`"${{ replace(parameters.testProjects, ';', '`;') }}\`" | ||
/p:TestRunnerAdditionalArguments="${{ parameters.testRunnerAdditionalArguments }}" |
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.
Needed to add a way to pass this value. Since the arguments do not include ;
and ,
, but do contain spaces, the double doubles around the value should be sufficient.
### TestTemplatesCG ### | ||
# Note: This job is only used to allow the test templates to be built locally on the agent as opposed to Helix. | ||
# The tests acquire the templates' PackageReferences from NuGet, which allows them to be scanned by CG (component governance). | ||
# CG is only ran internally, so this job makes sense to only run alongside of the official jobs. | ||
- categoryName: TestTemplatesCG | ||
testProjects: $(Build.SourcesDirectory)/test/dotnet-new.IntegrationTests/dotnet-new.IntegrationTests.csproj | ||
testRunnerAdditionalArguments: -class Microsoft.DotNet.Cli.New.IntegrationTests.DotnetNewTestTemplatesTests | ||
publishXunitResults: true |
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.
This is the primary change. Let me know if this doesn't make sense. @Youssef1313
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.
It makes sense.
parameters: | ||
legacyCredential: $(dn-bot-dnceng-artifact-feeds-rw) | ||
- template: /eng/common/templates/steps/enable-internal-runtimes.yml | ||
- ${{ if eq(parameters.populateInternalRuntimeVariables, true) }}: |
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.
This also didn't need to run on every build. It only applies to internal builds. So, added the parameter to control that.
os: macOS | ||
helixTargetQueue: osx.13.arm64 | ||
populateInternalRuntimeVariables: true | ||
runtimeSourceProperties: /p:DotNetRuntimeSourceFeed=https://ci.dot.net/internal /p:DotNetRuntimeSourceFeedKey=$(dotnetbuilds-internal-container-read-token-base64) |
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'm fairly certain this is necessary but was missing. This job is only ran when the checkbox for enableArm64Job is used in a manual run. So, probably no one noticed.
@MiYanni Are we able to get this one merged soon 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.
Approving, but I'd be comfortable if someone else also reviewed, since I ended up doing a bunch of changes.
@agocke We're getting an AOT test failure:
|
Does it fail without the changes in this PR? |
@agocke The failure is unrelated to the changes as it was seen before. #50901 (comment) I restarted the job as it's likely flakiness and not consistent failure. |
@MiYanni It's green now. Can we get it in ASAP to try to understand why the merge of release/10.0.1xx to main is failing? Ping @dotnet/dotnet-cli for review. |
This is also blocking the flow of changes in 10.0.1xx that went post-RC2 to VMR. |
This provides better error reporting for test failures which is also unified with everything else in CI. See also #50946 (comment)