Skip to content

Download smoke test prereqs in prep script#16359

Closed
mthalman wants to merge 14 commits intodotnet:mainfrom
mthalman:sb3386-smoke-tests
Closed

Download smoke test prereqs in prep script#16359
mthalman wants to merge 14 commits intodotnet:mainfrom
mthalman:sb3386-smoke-tests

Conversation

@mthalman
Copy link
Copy Markdown
Member

Contributes to dotnet/source-build#3386

This provides a way for people to download the packages required to smoke tests in order to fulfill two scenarios:

  • Pre-downloading the packages allows the tests to be run offline.
  • Provides a way to download prerelease packages from a private NuGet feed.

The prep script is modified to include options that, when provided, will download the necessary packages required by the smoke tests. Unfortunately, becomes NuGet's restore logic only allows a project to reference a single version of a NuGet package. But some of the packages required by the smoke tests require multiple versions of a given package name. So a custom MSBuild target is needed which iteratively runs restore once for each individual package version.

The list of packages is defined in a prereqs.csproj file. The accuracy of this list is ensured by a newly added test. This test gets run after all the other smoke tests have finished execution as it is the execution of those tests which determines which packages are required. The packages which were loaded as a result of running the smoke tests is then validated against the list of packages defined in the prereqs.csproj to ensure its accuracy.

@mthalman mthalman requested a review from a team as a code owner May 10, 2023 14:49
Comment thread src/SourceBuild/content/prep.sh Outdated
Comment thread src/SourceBuild/content/build.proj
Comment thread src/SourceBuild/content/prep.sh Outdated
Comment thread src/SourceBuild/content/prep.sh
@mthalman mthalman requested a review from MichaelSimons May 25, 2023 19:24
OutputHelper = outputHelper;
}

public static void EnumerateTarball(string tarballPath, Func<TarEntry, bool> continueEnumeration)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was there a particular reason this wasn't put in the Utilities class? Seems like a better fit.

Comment thread eng/pipelines/templates/jobs/vmr-build.yml
Comment thread eng/pipelines/templates/jobs/vmr-build.yml
Comment thread src/SourceBuild/content/prep.sh Outdated
### --runtime-source-feed URL of a remote server or a local directory, from which SDKs and
### runtimes can be downloaded
### --runtime-source-feed-key Key for accessing the above server, if necessary
### --smoke-test-prereqs-path Directory where the smoke test prereqs packages should be downloaded to.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depending on the resolution to my previous comments, this may the the only new parameter. If that's the case I think this script would be more dev friendly if there was a 1. a flag to enable smoke test prereq gathering 2.the path was defaulted with an env override?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how we could have a prep script that didn't include a feed option. There has to be a way to specify the DSP feed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I still find the smoke-test-prereqs-path option to be a bit inconsistent - there is no equivalent option to specify the artifacts path today. Because of that, I towards this should be defaulted. This would require the addition of a n option to indicate you want the prereqs to be gathered.

@MichaelSimons
Copy link
Copy Markdown
Member

Regarding ad23764 and 4731319, what do you see as the frequency for these updates? Is this something we are going to need to update each release?

@mthalman
Copy link
Copy Markdown
Member Author

mthalman commented Jun 6, 2023

Latest updates:

  • Updates the smoke tests project so that when run offline, it will restore packages from the local prereqs feed location.
  • Updates prep script to use a get-smoke-test-prereqs option to be explicit about the action to take rather than implied from the path. This also removes the prereqs path location as an option and is now defaulted instead.
  • Treats errors of the prereqs as warnings so that we can evaluate how much churn is involved with keeping the baseline up-to-date before causing the tests to fail the build. This change, unfortunately, requires that we not run tests offline for now during this eval period because running offline would cause restore failures in scenario tests when baseline isn't correct.
  • Various bug fixes

@mthalman mthalman requested a review from MichaelSimons June 6, 2023 19:43
@mthalman
Copy link
Copy Markdown
Member Author

mthalman commented Jun 7, 2023

@MichaelSimons - This previous update is interesting. This dependency is defined by the project generated by the MSTest template and has this content:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>net8.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>

    <IsPackable>false</IsPackable>
    <IsTestProject>true</IsTestProject>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.6.0" />
    <PackageReference Include="MSTest.TestAdapter" Version="3.0.4" />
    <PackageReference Include="MSTest.TestFramework" Version="3.0.4" />
    <PackageReference Include="coverlet.collector" Version="6.0.0" />
  </ItemGroup>

</Project>

That version change was updated by dotnet/test-templates#307. Those dependencies make it tricky if we want to change things to derive the package list via static analysis. It may be that we'd need to use the SDK to create projects from the templates and parse the generated project file to get the packages that they reference.

@mthalman
Copy link
Copy Markdown
Member Author

mthalman commented Jun 7, 2023

It may be that we'd need to use the SDK to create projects from the templates and parse the generated project file to get the packages that they reference.

I guess that wouldn't really work when executing from the prep script since you don't yet have the SDK.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to re-use the existing NuGet.config files that already exist in the assets folder? These NuGet.configs get used by the test projects the tests themselves build. At a minimum we should utilize the same naming convention.


```bash
./prep.sh --no-artifacts --no-prebuilts \
--smoke-test-prereqs-output prereqs/packages/smoke-test-prereqs
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cmd and the one later on need to be updated to use the latest prep options. e.g. get-smoke-test-prereqs

if [[ '${{ parameters.runOnline }}' == 'False' ]]; then
smokeTestFeed="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet8/nuget/v3/index.json"
smokeTestFeedKeyArg=''
# Enable this when we have a feed for internal builds (when .NET 8 enters servicing)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can happen before servicing. It will be required at GoLive time whenever there is an MSRC.

I am a bit concerned about the maintainability of this. Ideally this would be able to share logic from eng/common/SetupNuGetSources.sh to be able to retrieve the feeds to add so that there is not a disconnect.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we at the point where we should have regular unit tests for the prep script? Those tests could verify the usage of the feed options and then here we could utilize the SetupNugetSources script to avoid introducing a maintenance item that needs to be updated for each release.

@mthalman
Copy link
Copy Markdown
Member Author

mthalman commented Jun 8, 2023

Abandoning these changes because of the issues that we're going to have with maintenance of the baseline file (see dotnet/source-build#3386 (comment)).

@mthalman mthalman closed this Jun 8, 2023
@mthalman mthalman deleted the sb3386-smoke-tests branch July 17, 2023 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants