[release/13.2] Make VerifyAspireCliVersionAsync resilient across branches#15561
[release/13.2] Make VerifyAspireCliVersionAsync resilient across branches#15561mitchdenny wants to merge 1 commit intorelease/13.2from
Conversation
Read VersionPrefix dynamically from eng/Versions.props instead of hardcoding a specific version. For non-stabilized builds (all normal PR builds), also verify the commit SHA suffix for exact build identity. This replaces the hardcoded '13.2.0' check that was added for the stabilized 13.2.0 release build. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 15561Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 15561" |
mitchdenny
changed the title
There was a problem hiding this comment.
Pull request overview
This PR updates Aspire CLI E2E test helpers to avoid hardcoding an expected CLI version, making VerifyAspireCliVersionAsync work across branches by deriving the version prefix from eng/Versions.props.
Changes:
- Add helpers to parse
MajorVersion/MinorVersion/PatchVersionfromeng/Versions.propsto build a dynamic version prefix. - Add a helper intended to detect whether builds are “stabilized” (no SHA suffix).
- Update
VerifyAspireCliVersionAsyncto validate the dynamic version prefix and (when non-stabilized) the commit SHA suffix.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/Aspire.Cli.EndToEnd.Tests/Helpers/CliE2ETestHelpers.cs | Adds XML parsing helpers for VersionPrefix and stabilized-build detection. |
| tests/Aspire.Cli.EndToEnd.Tests/Helpers/CliE2EAutomatorHelpers.cs | Switches CLI version verification from a hardcoded prefix to dynamic prefix + optional SHA suffix validation. |
| // The default value in Versions.props uses a Condition to default to "false", | ||
| // so we read the element's text directly. | ||
| var stabilize = doc.Descendants(ns + "StabilizePackageVersion") | ||
| .FirstOrDefault()?.Value; | ||
|
|
||
| return string.Equals(stabilize, "true", StringComparison.OrdinalIgnoreCase); |
There was a problem hiding this comment.
IsStabilizedBuild reads the raw element value from eng/Versions.props, but that property is conditionally assigned (and can be overridden via /p:StabilizePackageVersion=...). With the current Versions.props shape, this method can return a value that doesn’t match the effective build setting (e.g., PR builds overriding it to false), which will cause downstream version validation to skip/enable SHA checks incorrectly. Consider evaluating the property via MSBuild (same global properties as the build) or inferring stabilization from the actual aspire --version output (presence/absence of +g{sha} / prerelease label).
| var versionPrefix = CliE2ETestHelpers.GetVersionPrefix(); | ||
| var isStabilized = CliE2ETestHelpers.IsStabilizedBuild(); | ||
|
|
||
| await auto.TypeAsync("aspire --version"); | ||
| await auto.EnterAsync(); | ||
|
|
||
| // When the build is stabilized (StabilizePackageVersion=true), the CLI version | ||
| // is just "13.2.0" with no commit SHA suffix. When not stabilized, it includes | ||
| // the SHA (e.g., "13.2.0-preview.1.g<sha>"). In both cases, "13.2.0" is present. | ||
| // TODO: This change should be reverted on the integration to the main branch. | ||
| await auto.WaitUntilTextAsync("13.2.0", timeout: TimeSpan.FromSeconds(10)); | ||
| // Always verify the version prefix matches the branch's version (e.g., "13.3.0"). | ||
| await auto.WaitUntilTextAsync(versionPrefix, timeout: TimeSpan.FromSeconds(10)); | ||
|
|
||
| // For non-stabilized builds (all PR CI builds), also verify the commit SHA suffix | ||
| // to uniquely identify the exact build. Stabilized builds (official releases only) | ||
| // produce versions without SHA suffixes, so we skip this check. | ||
| if (!isStabilized && commitSha.Length == 40) | ||
| { | ||
| var shortCommitSha = commitSha[..8]; | ||
| var expectedVersionSuffix = $"g{shortCommitSha}"; | ||
| await auto.WaitUntilTextAsync(expectedVersionSuffix, timeout: TimeSpan.FromSeconds(10)); | ||
| } |
There was a problem hiding this comment.
VerifyAspireCliVersionAsync’s SHA-suffix validation depends on IsStabilizedBuild(), but IsStabilizedBuild currently parses Versions.props without evaluating MSBuild conditions/overrides. This can make the SHA check get skipped even in PR CI runs, reducing the test’s ability to prove the CLI came from the current PR build. Suggest basing the decision on the observed aspire --version output (e.g., wait until the screen contains the prefix and then, if it contains +g, require the matching g{shortSha}; otherwise require an exact stabilized version).
|
🎬 CLI E2E Test Recordings — 51 recordings uploaded (commit View recordings
📹 Recordings uploaded automatically from CI run #23520298644 |
JamesNK
left a comment
JamesNK
left a comment
There was a problem hiding this comment.
The approach of dynamically reading eng/Versions.props instead of hardcoding the version is a good idea, but IsStabilizedBuild() reads the source file on disk rather than the actual build-time value (which CI overrides via /p:StabilizePackageVersion=false). This means the SHA suffix check is dead code on release/13.2. See inline comments.
|
|
||
| // For non-stabilized builds (all PR CI builds), also verify the commit SHA suffix | ||
| // to uniquely identify the exact build. Stabilized builds (official releases only) | ||
| // produce versions without SHA suffixes, so we skip this check. |
There was a problem hiding this comment.
On the release/13.2 branch, eng/Versions.props has StabilizePackageVersion=true. This method reads that file value, so IsStabilizedBuild() always returns true on this branch — making the SHA verification block below dead code.
The PR description states "StabilizePackageVersion=false in all PR builds", but that's a CI pipeline override (/p:StabilizePackageVersion=false), not the file on disk. Since this reads the source file rather than querying the actual build configuration, the if (!isStabilized && ...) check never executes on release/13.2.
The test still passes (version prefix alone is sufficient), but the commit SHA check described in the PR's scenario table for release/13.2 PR builds won't actually run. Consider reading the stabilization state from the CLI's own version output (e.g., presence of + in the version string) instead of the source file.
| /// of "13.2.0-preview.1.25175.1+g{sha}"). This is only true for official release builds, | ||
| /// never for normal PR CI builds. | ||
| /// </summary> | ||
| internal static bool IsStabilizedBuild() |
There was a problem hiding this comment.
Nit: Both GetVersionPrefix() and IsStabilizedBuild() independently call GetRepoRoot(), build the same eng/Versions.props path, and parse it with XDocument.Load(). Since they're called back-to-back from VerifyAspireCliVersionAsync, this parses the same XML file twice. Consider a single helper that loads the document once and returns both values.
Description
Replaces the hardcoded
"13.2.0"version check inVerifyAspireCliVersionAsyncwith a dynamic approach that reads theVersionPrefixfromeng/Versions.propsat test time.Context: Why this fix exists
PR #15540 merges
release/13.2intomain. On therelease/13.2branch,VerifyAspireCliVersionAsyncwas changed to hardcode"13.2.0"(with a TODO: "This change should be reverted on the integration to the main branch") because stabilized release builds don't include the commit SHA suffix in the version string.When that merge PR reaches
main(where version is13.3.0-preview.1), the hardcoded"13.2.0"check fails. This pattern will repeat every timerelease/*merges intomain(or vice versa) if version strings are hardcoded.This PR fixes the root cause on
release/13.2so that future merges in either direction just work. The same fix was also pushed directly to PR #15540's branch (merge/release-13.2-to-main).Problem
The version check was hardcoded to
"13.2.0"on therelease/13.2branch because stabilized builds don't include the commit SHA suffix. This breaks when:main(where version is13.3.0)13.2.0→13.2.1)Solution
CliE2ETestHelpers.cs: AddedGetVersionPrefix()andIsStabilizedBuild()that parseeng/Versions.propsdynamicallyCliE2EAutomatorHelpers.cs: UpdatedVerifyAspireCliVersionAsyncto:VersionPrefix(adapts per branch)"13.2.0"and#pragma warningsuppressionWhy this is safe across branch merges
mainat 13.3.013.3.013.3.0-preview.1.xxx+g{sha}release/13.2at 13.2.013.2.013.2.0-pr.xxxrelease→main13.2.013.2.0NuGet package isolation
PR build packages remain isolated via the hive mechanism:
-pr.XXXX.shasuffixes (never match published GA versions)StabilizePackageVersion=falsein all PR buildsAspire*packages to local hive filesRelated PRs
release/13.2→main) by @joperezr that surfaced this issue. Same fix pushed to that branch.release/13.2so it's available for future merges.Checklist