-
Notifications
You must be signed in to change notification settings - Fork 542
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
[tests] Add support for running EndToEnd tests on helix with the aspire
workload
#2534
Changes from 36 commits
9770c7d
f62530d
99f6a42
bf1bb7d
116f4d0
ce61861
39472c8
949294f
ef410ff
9aaf1ec
8649005
a976089
94a50c4
56c4f2d
800ed52
28430fd
e5afe1b
a2116d8
a57a478
149b6ff
2705b8c
c1b2b68
4199746
c6b9e56
3f8cd92
af1e0de
8dfe316
8072559
548a340
94e1d29
57bc07b
5be48e0
a600106
ffa8b33
8434d15
d951135
64a9eb7
e3e1a65
f49215a
bdb4081
0e79ba0
59085f9
5e4943b
aff6eb7
02f5cf0
4a0ea34
c40da44
b390a0c
56b7c61
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,7 +26,7 @@ | |
|
||
<Target Name="Build" /> | ||
|
||
<Target Name="BeforeBuild" BeforeTargets="Build"> | ||
<Target Name="BeforeBuild" BeforeTargets="_GetPackageFiles"> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need this change? In general, it's best to avoid depending on "private" tragets (Targets that start with an underscore) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wanted to limit this target to only when we are generating a nuget. Is this target needed only for generating the zip file? Is the zip file used for the dashboard docker image? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This target mainly accomplishes two things:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reverted. |
||
<MSBuild Projects="../../src/Aspire.Dashboard/Aspire.Dashboard.csproj" Targets="publish" Properties="Configuration=$(Configuration);Platform=$(Platform);TargetFramework=$(TargetFramework);RuntimeIdentifier=$(DashboardRuntime)" /> | ||
|
||
<!-- After publishing the project, we ensure that the published assets get packed in the nuspec. --> | ||
|
@@ -36,7 +36,7 @@ | |
</ItemGroup> | ||
|
||
<MakeDir Directories="$(DashboardPublishedArtifactsOutputDir)/$(DashboardRuntime)" /> | ||
<ZipDirectory SourceDirectory="$(DotNetOutputPath)Aspire.Dashboard/$(Configuration)/$(TargetFramework)/$(DashboardRuntime)/publish" DestinationFile="$(DashboardPublishedArtifactsOutputDir)/$(DashboardRuntime)/aspire-dashboard-$(DashboardRuntime).zip" /> | ||
<ZipDirectory SourceDirectory="$(DotNetOutputPath)Aspire.Dashboard/$(Configuration)/$(TargetFramework)/$(DashboardRuntime)/publish" DestinationFile="$(DashboardPublishedArtifactsOutputDir)/$(DashboardRuntime)/aspire-dashboard-$(DashboardRuntime).zip" Overwrite="true" /> | ||
joperezr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
<!-- Throw an error if _PublishItems is empty. --> | ||
<Error Condition="'@(_PublishItems)' == ''" Text="No files were found to pack. Ensure that the project being packed has a publish target defined." /> | ||
|
@@ -48,4 +48,4 @@ | |
<None Include="UnixFilePermissions.xml" Pack="true" PackagePath="data/" Condition=" '$(DashboardPlatformType)' == 'Unix' " /> | ||
</ItemGroup> | ||
|
||
</Project> | ||
</Project> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,7 @@ parameters: | |
steps: | ||
- script: ${{ parameters.buildScript }} | ||
-restore -build | ||
-pack | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are already running pack as a separate step (check line 98 in this file). Can we either change the ordering so that happens before the steps that run the tests, or alternatively just remove that whole step if it is no longer needed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. This pack could conflict with the subsequent pack, since that one is signed. I will investigate this in a separate PR. And it will really be needed only when we have this running on windows again. |
||
-configuration ${{ parameters.buildConfig }} | ||
/p:ArchiveTests=true | ||
/bl:${{ parameters.repoLogPath }}/build.binlog | ||
|
@@ -42,10 +43,20 @@ steps: | |
displayName: Publish non-helix TestResults | ||
condition: always() | ||
|
||
- ${{ if ne(parameters.isWindows, 'true') }}: | ||
# E2E tests are disabled on windows for now | ||
- script: ${{ parameters.buildScript }} | ||
-p:ContinuousIntegrationBuild=true | ||
-p:Configuration=${{ parameters.buildConfig }} | ||
-p:TriggerInstallWorkloadForTesting=true | ||
-bl:${{ parameters.repoLogPath }}/WorkloadInstallForTesting.binlog | ||
--projects $(Build.SourcesDirectory)/tests/Aspire.EndToEnd.Tests/Aspire.EndToEnd.Tests.csproj | ||
displayName: Install sdk+workload for testing | ||
|
||
- template: /eng/pipelines/templates/send-to-helix.yml | ||
parameters: | ||
HelixProjectPath: '$(Build.SourcesDirectory)/tests/send-to-helix.proj' | ||
HelixProjectArguments: /p:RunWithCodeCoverage=true /p:RepoTestResultsPath=${{ parameters.repoTestResultsPath }} | ||
HelixProjectArguments: /p:Configuration=${{ parameters.buildConfig }} /p:RunWithCodeCoverage=true /p:RepoTestResultsPath=${{ parameters.repoTestResultsPath }} | ||
|
||
${{ if eq(parameters.isWindows, 'true') }}: | ||
${{ if eq(variables['System.TeamProject'], 'public') }}: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,8 +10,6 @@ | |
<VSTemp>$(WorkloadIntermediateOutputPath)VS/</VSTemp> | ||
<WorkloadOutputPath>$(ArtifactsBinDir)workloads/</WorkloadOutputPath> | ||
<WorkloadOutputPath Condition="'$(workloadArtifactsPath)' != ''">$(workloadArtifactsPath)/</WorkloadOutputPath> | ||
<PackageSource>$(ArtifactsShippingPackagesDir)</PackageSource> | ||
<PackageSource Condition="'$(workloadPackagesPath)' != ''">$(workloadPackagesPath)/</PackageSource> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this mean that this cannot get overriden any longer? cc: @danegsta in case he is aware if we depend on this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be fine as long as the workload still builds correctly |
||
<RunAnalyzers>false</RunAnalyzers> | ||
</PropertyGroup> | ||
|
||
|
@@ -75,11 +73,11 @@ | |
</ShortNames> | ||
</ItemGroup> | ||
|
||
<MSBuild Projects="../dashboardpack/dashboardpack.csproj" Targets="restore;build" Properties="DashboardRuntime=%(_PackRuntimes.Identity);PackageOutputPath=$(PackageSource)" /> | ||
<MSBuild Projects="../dcppack/dcppack.csproj" Targets="restore;build" Properties="DcpRuntime=%(_PackRuntimes.Identity);PackageOutputPath=$(PackageSource)" /> | ||
<MSBuild Projects="../dashboardpack/dashboardpack.csproj" Targets="restore;build" Properties="DashboardRuntime=%(_PackRuntimes.Identity);PackageOutputPath=$(WorkloadsPackageSource)" /> | ||
<MSBuild Projects="../dcppack/dcppack.csproj" Targets="restore;build" Properties="DcpRuntime=%(_PackRuntimes.Identity);PackageOutputPath=$(WorkloadsPackageSource)" /> | ||
|
||
<ItemGroup> | ||
<ManifestPackages Include="$(PackageSource)Microsoft.NET.Sdk.Aspire.Manifest*.nupkg" | ||
<ManifestPackages Include="$(WorkloadsPackageSource)Microsoft.NET.Sdk.Aspire.Manifest*.nupkg" | ||
MsiVersion="$(MsiVersion)" | ||
SupportsMachineArch="true" /> | ||
</ItemGroup> | ||
|
@@ -89,7 +87,7 @@ | |
BaseOutputPath="$(WorkloadOutputPath)" | ||
EnableSideBySideManifests="true" | ||
ComponentResources="@(ComponentResources)" | ||
PackageSource="$(PackageSource)" | ||
PackageSource="$(WorkloadsPackageSource)" | ||
ShortNames="@(ShortNames)" | ||
WorkloadManifestPackageFiles="@(ManifestPackages)" | ||
WixToolsetPath="$(WixToolsetPath)" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,9 @@ | |
<PropertyGroup> | ||
<PackageId>$(PackageId).Manifest-$(DotNetAspireManifestVersionBand)</PackageId> | ||
<Description>.NET Aspire workload manifest</Description> | ||
|
||
<!-- this allows installing the manifest as a nuget, which is useful for testing --> | ||
<PackageType /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just Curious, does this change anything in the nuspec? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this removes: <packageTypes>
<packageType name="DotnetPlatform" />
</packageTypes> .. from the In a follow up for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. mmm, I suppose this property is set via Arcade. It would be good to check what the implications of that are with someone from coreeng team. |
||
</PropertyGroup> | ||
|
||
<ItemGroup> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
<Project> | ||
<PropertyGroup> | ||
<VersionBand Condition=" '$(VersionBand)' == '' ">$([System.Text.RegularExpressions.Regex]::Match($(MicrosoftDotnetSdkInternalPackageVersion), `^\d+\.\d+\.\d`))00</VersionBand> | ||
<!-- When we are ready to produce Workloads targeting the stable SDK band, set UseStableSdkBand to true. Otherwise, it will match the SDK preview band. --> | ||
<UseStableSdkBand>true</UseStableSdkBand> | ||
<DotNetVersionBand Condition=" '$(UseStableSdkBand)' != 'true' ">$(VersionBand)$([System.Text.RegularExpressions.Regex]::Match($(MicrosoftDotnetSdkInternalPackageVersion), `\-(preview|rc|alpha|rtm).\d+`))</DotNetVersionBand> | ||
<DotNetVersionBand Condition="'$(UseStableSdkBand)' == 'true'">$(VersionBand)</DotNetVersionBand> | ||
<DotNetAspireManifestVersionBand>$(DotNetVersionBand)</DotNetAspireManifestVersionBand> | ||
</PropertyGroup> | ||
</Project> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,6 @@ | ||
<Project> | ||
<Import Project="Workload.props" /> | ||
|
||
<PropertyGroup> | ||
<TargetFramework>net8.0</TargetFramework> | ||
<PackageType>DotnetPlatform</PackageType> | ||
|
@@ -12,13 +14,7 @@ | |
<DotNetOutputPath>$(RepoRoot)artifacts/bin/</DotNetOutputPath> | ||
<DotNetDirectory>$(DotNetOutputPath)dotnet/</DotNetDirectory> | ||
<DotNetPacksDirectory>$(DotNetDirectory)packs/</DotNetPacksDirectory> | ||
<VersionBand Condition=" '$(VersionBand)' == '' ">$([System.Text.RegularExpressions.Regex]::Match($(MicrosoftDotnetSdkInternalPackageVersion), `^\d+\.\d+\.\d`))00</VersionBand> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just curious, why extract only those properties and not the rest? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The extracted properties define the version, and band used for the workload. The remaining properties are relevant to the various projects that import it for building packages. Some of these common properties could still be moved to the props file, but not all. |
||
<!-- When we are ready to produce Workloads targeting the stable SDK band, set UseStableSdkBand to true. Otherwise, it will match the SDK preview band. --> | ||
<UseStableSdkBand>true</UseStableSdkBand> | ||
<DotNetVersionBand Condition=" '$(UseStableSdkBand)' != 'true' ">$(VersionBand)$([System.Text.RegularExpressions.Regex]::Match($(MicrosoftDotnetSdkInternalPackageVersion), `\-(preview|rc|alpha|rtm).\d+`))</DotNetVersionBand> | ||
<DotNetVersionBand Condition="'$(UseStableSdkBand)' == 'true'">$(VersionBand)</DotNetVersionBand> | ||
<DotNetSdkManifestsFolder>$(DotNetVersionBand)</DotNetSdkManifestsFolder> | ||
<DotNetAspireManifestVersionBand>$(DotNetVersionBand)</DotNetAspireManifestVersionBand> | ||
<_AspireDotNetVersionMajor>8</_AspireDotNetVersionMajor> | ||
<_AspireDotNetVersionMinor>0</_AspireDotNetVersionMinor> | ||
<_AspireDotNetVersion>$(_AspireDotNetVersionMajor).$(_AspireDotNetVersionMinor)</_AspireDotNetVersion> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
<?xml version="1.0" encoding="utf-8"?> | ||
<RunSettings> | ||
<RunConfiguration> | ||
<!-- Timeout in ms, 15 minutes --> | ||
<TestSessionTimeout>900000</TestSessionTimeout> | ||
<!-- Filter out failing (wrong framework, platform, runtime or activeissue) tests --> | ||
<TestCaseFilter>category!=failing</TestCaseFilter> | ||
</RunConfiguration> | ||
<LoggerRunSettings> | ||
<Loggers> | ||
<Logger friendlyName="trx"> | ||
<Configuration> | ||
<LogFileName>TestResults.trx</LogFileName> | ||
</Configuration> | ||
</Logger> | ||
<Logger friendlyName="console"> | ||
<Configuration> | ||
<Verbosity>normal</Verbosity> | ||
</Configuration> | ||
</Logger> | ||
</Loggers> | ||
</LoggerRunSettings> | ||
</RunSettings> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,10 +2,31 @@ | |
|
||
<PropertyGroup> | ||
<TargetFramework>$(NetCurrent)</TargetFramework> | ||
|
||
<!-- no docker support on helix/windows yet --> | ||
<RunTestsOnHelix Condition="'$(OS)' != 'Windows_NT'">true</RunTestsOnHelix> | ||
<SkipTests Condition="'$(OS)' == 'Windows_NT'">true</SkipTests> | ||
Comment on lines
+6
to
+8
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we just say There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes we can do it like that also. The reason for "also" is that if we don't have any tests that would get run, then not skipping the whole thing would mean:
I think we can add the attributes also, and for the specific case (windows) where nothing can run, we skip in the project file too. Thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How long until we are able to run these tests on Windows? |
||
|
||
<!-- no docker support on helix/windows yet --> | ||
<TestUsingWorkloads Condition="! ('$(ContinuousIntegrationBuild)' == 'true' and '$(OS)' == 'Windows_NT')">true</TestUsingWorkloads> | ||
<InstallWorkloadForTesting>$(TestUsingWorkloads)</InstallWorkloadForTesting> | ||
|
||
<BuiltNuGetsDir>$(ArtifactsShippingPackagesDir)</BuiltNuGetsDir> | ||
<PackageVersionForWorkloadManifests>$(PackageVersion)</PackageVersionForWorkloadManifests> | ||
<DefineConstants Condition="'$(TestsRunningOutsideOfRepo)' == 'true'">TESTS_RUNNING_OUTSIDE_OF_REPO;$(DefineConstants)</DefineConstants> | ||
|
||
<XunitRunnerJson>xunit.runner.json</XunitRunnerJson> | ||
</PropertyGroup> | ||
|
||
<ItemGroup> | ||
<Compile Include="..\testproject\Common\TestResourceNames.cs" /> | ||
<Compile Include="..\Shared\WorkloadTesting\*.cs" Link="WorkloadTestingCommon" /> | ||
|
||
<None Include="..\testproject\**\*" Link="testassets\testproject\%(RecursiveDir)%(FileName)%(Extension)" CopyToOutputDirectory="PreserveNewest" /> | ||
<None Include="$(PatchedNuGetConfigPath)" Link="testassets\testproject\nuget.config" CopyToOutputDirectory="PreserveNewest" /> | ||
<None Include="$(RepoRoot)Directory.Packages.props" Link="testassets\testproject\Directory.Packages.repo.props" CopyToOutputDirectory="PreserveNewest" /> | ||
|
||
<PackageReference Include="Microsoft.Extensions.Http.Resilience" /> | ||
<PackageReference Include="Microsoft.DotNet.XUnitExtensions" /> | ||
</ItemGroup> | ||
|
||
</Project> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
<Project> | ||
<PropertyGroup> | ||
<IsHelixOnlyTestProject>true</IsHelixOnlyTestProject> | ||
<IsWorkloadTestProject>true</IsWorkloadTestProject> | ||
<RunSettingsFilePath>$(MSBuildThisFileDirectory).runsettings</RunSettingsFilePath> | ||
</PropertyGroup> | ||
|
||
<Import Project="..\Directory.Build.props" /> | ||
</Project> |
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.
Where is this package source defined?
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.
https://github.com/dotnet/runtime/blob/f135699310bccfa83b3b441d2f4f1571fe8df480/src/tasks/WorkloadBuildTasks/InstallWorkloadFromArtifacts.cs#L262
I agree that it does seem odd that you can't directly see what this links up to in the repo. I can have the package source name be passed to the task, which would be in aspire repo.
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.
Does this throw a warning at all on the initial restore before the task has a chance to replace the source? Did we also consider manually passing in
-s <path>
to a manual invocation of restore instead of having this in the config?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.
No warnings. Wouldn't
-s path
only add a package source but not the mapping, and because of that restoring from the source would fail. That is what I hit when I removed this mapping.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.
Once dotnet/runtime#99277 is merged this entry can be removed, and it would be added at build time.