-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Add gRPC interop tests (again) #20069
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
Conversation
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.
If the internal build succeeds LGTM
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.
<Project> | ||
|
||
<ItemGroup> | ||
<ProjectReference Include="@(FunctionalTestAssetProjectReference)" ReferenceOutputAssembly="false" /> |
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 are the @(FunctionalTestAssetProjectReference)
items built at all on the build agents❔ When tests depend on newly-built AspNetCore.App bits, usual approach is to build them only on the Helix agents, adding @(Content)
items for the test assets so they're part of the work item payload automatically. If necessary (see project template tests), fallbacks for local testing are of course fine. But, this seems to be set up for CI testing
And, why is this infrastructure centralized (it's only used in the gRPC InteropTests
project)❔
@@ -0,0 +1,9 @@ | |||
<Project> | |||
<Target Name="CollectFunctionalTestPayload" DependsOnTargets="Publish" Returns="@(DependencyPayload)" > |
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.
Unclear how this works any better or more extensively than what's already in Helix.targets for all test projects. Shouldn't need to do anything more than remove ReferenceOutputAssembly="false"
to get the referenced test asset outputs into the work item payload.
RuntimePackNamePatterns="Microsoft.AspNetCore.App.Runtime.**RID**" | ||
DefaultRuntimeFrameworkVersion="$(SharedFxVersion)" | ||
LatestRuntimeFrameworkVersion="$(SharedFxVersion)" | ||
TargetingPackVersion="$(MicrosoftAspNetCoreAppRefPackageVersion)" |
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.
Doesn't work unless the targeting pack is building.
This is a fundamental break that could be left out completely because the $(TestDependsOnAspNetRuntime)
infrastructure handles everything for you on the Helix agents. If the point is to handle things on build agents, recommend using $(UpdateAspNetCoreKnownFramework)
property but ignoring the test asset projects except in very special cases.
Note: I'll admit I should have known better when signing off 1.5 years ago :shame: |
Re-implementing #17040. The first PR missed a restore step modification in the Daily Builds and Quarantine pipelines which is fixed by db585bd. I'm going to run the daily build pipeline to ensure this fix is sufficient.