Skip to content

[ci] Enable testing building previous#33961

Open
rmarinho wants to merge 1 commit into
net11.0from
enable-back-previous-tests
Open

[ci] Enable testing building previous#33961
rmarinho wants to merge 1 commit into
net11.0from
enable-back-previous-tests

Conversation

@rmarinho

@rmarinho rmarinho commented Feb 9, 2026

Copy link
Copy Markdown
Member

Description of Change

This pull request re-enables and adds several integration tests for .NET MAUI templates targeting the "DotNetPrevious" framework across iOS, macOS, and Windows, and updates the CI pipeline to run these tests in parallel for both ARM64 and x64 architectures. These changes help ensure better test coverage for previous .NET versions and improve CI reliability.

Test Coverage Improvements:

  • Re-enabled previously commented-out [InlineData] test cases for "DotNetPrevious" in SimpleTemplateTest.cs, covering maui, maui-blazor, and mauilib templates for build, special character, and bad image file scenarios. [1] [2] [3]
  • Re-enabled and added [InlineData] test cases for "DotNetPrevious" in WindowsTemplateTest.cs, covering both packaged and unpackaged publish scenarios for maui and maui-blazor templates. [1] [2]
  • Added dedicated test methods in AppleTemplateTests.cs for running iOS tests on the "DotNetPrevious" framework, enabling parallel execution in CI.

CI Pipeline Enhancements:

  • Added new jobs to the ci.yml pipeline to run iOS template tests for "DotNetPrevious" on both ARM64 (mac_runios_previous_arm64) and x64 (mac_runios_previous) architectures, supporting both public and internal MacOS pools and improving CI coverage for previous .NET versions. [1] [2]

Copilot AI review requested due to automatic review settings February 9, 2026 12:17
@rmarinho rmarinho added area-testing Unit tests, device tests testing-infrastructure Issue relating to testing infrastructure testing-reenable copilot labels Feb 9, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Re-enables .NET MAUI template integration tests against DotNetPrevious and extends CI to run the iOS “previous” lane on both ARM64 and x64 macOS pools.

Changes:

  • Re-enabled DotNetPrevious [InlineData] cases in SimpleTemplateTest and WindowsTemplateTest.
  • Added an iOS test entrypoint (RunOniOS_Previous) in AppleTemplateTests for CI to target.
  • Added two new CI jobs to run RunOniOS_Previous on ARM64 and x64 macOS pools.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
src/TestUtils/src/Microsoft.Maui.IntegrationTests/WindowsTemplateTest.cs Re-enables Windows publish tests for DotNetPrevious.
src/TestUtils/src/Microsoft.Maui.IntegrationTests/SimpleTemplateTest.cs Re-enables template build/pack scenarios for DotNetPrevious.
src/TestUtils/src/Microsoft.Maui.IntegrationTests/AppleTemplateTests.cs Adds RunOniOS_Previous test method for CI targeting.
eng/pipelines/ci.yml Adds new macOS jobs to run iOS previous tests on ARM64 and x64.

@@ -62,11 +62,14 @@ public AppleTemplateTests(IntegrationTestFixture fixture, ITestOutputHelper outp
if (true) return; // Skip: "Running Apple templates is only supported on macOS."

Copilot AI Feb 9, 2026

Copy link

Choose a reason for hiding this comment

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

if (true) return; disables the intended platform guard and does not actually skip tests (it just exits the constructor early). This can cause Apple tests to run on non-macOS environments (or proceed without proper setup) instead of being skipped. Replace this with the standard macOS guard used in the repo (e.g., if (!TestEnvironment.IsMacOS) Assert.Ignore("Running Apple templates is only supported on macOS.");) rather than returning unconditionally.

Copilot uses AI. Check for mistakes.
Comment on lines +65 to 73
// DotNetPrevious test methods
[Theory]
[InlineData("maui", "Debug", DotNetPrevious, RuntimeVariant.Mono, null)]
[InlineData("maui", "Release", DotNetPrevious, RuntimeVariant.Mono, null)]
[InlineData("maui-blazor", "Debug", DotNetPrevious, RuntimeVariant.Mono, null)]
[InlineData("maui-blazor", "Release", DotNetPrevious, RuntimeVariant.Mono, null)]
public void RunOniOS_Previous(string id, string config, string framework, RuntimeVariant runtimeVariant, string? trimMode)
=> RunOniOS(id, config, framework, runtimeVariant, trimMode);

Copilot AI Feb 9, 2026

Copy link

Choose a reason for hiding this comment

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

The new RunOniOS_Previous is a single [Theory] with multiple [InlineData] cases, but the comment immediately below states the CI model relies on “individual test methods” so each configuration can run in a separate job. With the current structure, the testName: RunOniOS_Previous job will execute all four cases serially in one job per arch. Consider splitting these into separate [Test] methods (or separate theory methods) per configuration (e.g., maui Debug, maui Release, maui-blazor Debug, maui-blazor Release) so the code matches the comment/CI strategy and keeps job runtimes predictable.

Suggested change
// DotNetPrevious test methods
[Theory]
[InlineData("maui", "Debug", DotNetPrevious, RuntimeVariant.Mono, null)]
[InlineData("maui", "Release", DotNetPrevious, RuntimeVariant.Mono, null)]
[InlineData("maui-blazor", "Debug", DotNetPrevious, RuntimeVariant.Mono, null)]
[InlineData("maui-blazor", "Release", DotNetPrevious, RuntimeVariant.Mono, null)]
public void RunOniOS_Previous(string id, string config, string framework, RuntimeVariant runtimeVariant, string? trimMode)
=> RunOniOS(id, config, framework, runtimeVariant, trimMode);
// DotNetPrevious test methods (individual methods per configuration for CI filtering)
[Fact]
public void RunOniOS_Previous_MauiDebug() => RunOniOS("maui", "Debug", DotNetPrevious, RuntimeVariant.Mono, null);
[Fact]
public void RunOniOS_Previous_MauiRelease() => RunOniOS("maui", "Release", DotNetPrevious, RuntimeVariant.Mono, null);
[Fact]
public void RunOniOS_Previous_BlazorDebug() => RunOniOS("maui-blazor", "Debug", DotNetPrevious, RuntimeVariant.Mono, null);
[Fact]
public void RunOniOS_Previous_BlazorRelease() => RunOniOS("maui-blazor", "Release", DotNetPrevious, RuntimeVariant.Mono, null);

Copilot uses AI. Check for mistakes.
Comment thread eng/pipelines/ci.yml
- name: mac_runios_previous_arm64
${{ if eq(variables['Build.DefinitionName'], 'maui-pr') }}:
pool: ${{ parameters.MacOSPool.public }}
${{ else }}:

Copilot AI Feb 9, 2026

Copy link

Choose a reason for hiding this comment

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

There are trailing spaces after ${{ else }}:. This is minor, but it can trip YAML linting or style checks. Consider removing the extra whitespace.

Copilot uses AI. Check for mistakes.
Comment thread eng/pipelines/ci.yml
- name: mac_runios_previous
${{ if eq(variables['Build.DefinitionName'], 'maui-pr') }}:
pool: ${{ parameters.MacOSPool.public }}
${{ else }}:

Copilot AI Feb 9, 2026

Copy link

Choose a reason for hiding this comment

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

There are trailing spaces after ${{ else }}:. This is minor, but it can trip YAML linting or style checks. Consider removing the extra whitespace.

Copilot uses AI. Check for mistakes.
@rmarinho

Copy link
Copy Markdown
Member Author

/rebase

@kubaflo

kubaflo commented Apr 17, 2026

Copy link
Copy Markdown
Contributor

🟡 .NET MAUI Code Review — Changes Suggested

👋 @rmarinho — new code review results are available. Please review the latest session below.

🟡 Review Session — Changes Suggestede8e499b · [ci] Enable testing building previous · 2026-04-17 09:11 UTC

Independent Assessment

What this changes:

  1. Re-enables commented-out [InlineData] cases for DotNetPrevious (net10.0) in SimpleTemplateTest.cs (Build, BuildWithMauiVersion, PackCoreLib) and WindowsTemplateTest.cs (PublishPackaged, PublishUnpackaged).
  2. Adds a new RunOniOS_Previous [Theory] in AppleTemplateTests.cs delegating to the private RunOniOS helper.
  3. Adds two new CI job pairs in ci.yml (mac_runios_previous_arm64, mac_runios_previous) to run that theory in both pool flavors.

Inferred motivation: Restore coverage that was disabled during the net11.0 branch bring-up, now that net10.0 is stable enough to build as the "previous" target.

Reconciliation with PR Narrative

Author's description matches the code. One wording note: the description says the AppleTemplateTests change enables "parallel execution in CI", but the implementation uses a single [Theory] method that CI targets with --filter "Name=RunOniOS_Previous", which runs all four InlineData cases serially in one job. The implementation doesn't actually deliver the claimed parallel fan-out.

Findings

❌ Error — Required CI check is red

maui-pr (Run Integration Tests RunOniOS_Previous ARM64) failed in ~8 min (no TRX output, so an early failure, not a timeout). The WindowsTemplates windows and AOT macOS integration lanes are also red. Since the whole purpose of this PR is to re-enable these tests, a red result on the exact lanes being re-enabled is a hard blocker. This needs to be investigated and either fixed, or the problematic InlineData rows must stay commented out with an issue link. Logs from build 1350355 have been purged — kicking a fresh run is the next step to capture the failure.

⚠️ Warning — [Theory] pattern contradicts the stated parallelism design

AppleTemplateTests.cs lines 71-72 explicitly document the design:

// Individual test methods for each configuration to enable parallel CI runs
// CI uses --filter "Name=TestMethodName" to run each test in a separate job

All existing DotNetCurrent entries follow that rule with four [Fact] wrappers (RunOniOS_MauiDebug, RunOniOS_MauiRelease, RunOniOS_BlazorDebug, RunOniOS_BlazorRelease). The new RunOniOS_Previous bundles four configurations into a single [Theory], and CI adds only one job pair that matches it — so Previous coverage runs ~4× the work serially in a lane with the same 45-min timeout.

Suggested fix: mirror the existing pattern with four [Fact] wrappers (e.g. RunOniOS_MauiDebug_Previous, …) and four CI job pairs. It also makes failure attribution per-config much clearer, which matters when reactivating long-disabled tests.

💡 Suggestion — Remove the now-stale commented block

AppleTemplateTests.cs:65-69 still has the old // [InlineData(...)] lines that this PR supersedes. Delete them to avoid confusion.

💡 Suggestion — Trailing whitespace in YAML

The new blocks in eng/pipelines/ci.yml have ${{ else }}: with trailing spaces. Minor, but the file is otherwise clean.

Devil's Advocate

  • Could [Theory] be intentional to minimize CI cost? Possibly, but then the PR description shouldn't claim "parallel execution" — the two statements are inconsistent. Either update the description or change the implementation.
  • Could the red checks be a flaky pre-existing lane? WindowsTemplates windows and AOT macOS sometimes flake, but RunOniOS_Previous ARM64 is a lane this PR just introduced — its failure is by definition caused by this PR.
  • Is re-enabling just "uncommenting" low-risk? Normally yes, but the red CI proves that at least some of the re-enabled configurations don't build/run cleanly with the current net10.0 SDK the CI image provides. Merging as-is would leave required checks red.

Verdict: NEEDS_CHANGES

Confidence: high
Summary: Mechanically straightforward PR, but (1) the CI lane this PR adds is failing, and (2) the new Apple iOS test uses a single [Theory] while the file's documented pattern — and all other Current-framework tests — use per-configuration [Fact] methods for parallel CI. Fix the failing lane (or scope down the re-enabled InlineData rows), and split the Apple test into four [Fact] wrappers with matching CI jobs to match the existing design.


@kubaflo

kubaflo commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

AI code review for net11.0 target

Verdict: Needs changes (re-enabled tests are currently failing in CI)

Independent review (diff-first, then reconciled with the PR narrative). This is not an approval — a human still needs to sign off.

What the PR does

Re-enables DotNetPrevious integration/template tests that were previously commented out (SimpleTemplateTest.Build, BuildWithMauiVersion, PackCoreLib, WindowsTemplateTest.Publish*), adds a new RunOniOS_Previous theory in AppleTemplateTests, and adds matching mac_runios_previous/mac_runios_previous_arm64 CI lanes in eng/pipelines/ci.yml.

Findings

  • The newly-enabled "previous" tests are red in CIRun Integration Tests RunOniOS_Previous ARM64, WindowsTemplates windows, AOT macOS, and Build macOS (Debug) are failing. Since the whole point of this PR is to turn these lanes on, they must be green (or have documented, tracked known-issues) before merge; otherwise this makes main/net11.0 CI permanently red. Please confirm whether the failures are real previous-SDK template breakages or infra, and link/justify any that are expected.
  • RunOniOS_Previous signature/parameters changed vs. the old commented-out data: the "iossimulator-x64" RID argument was dropped and the method now delegates to RunOniOS(id, config, framework, runtimeVariant, trimMode). I verified the call matches the existing RunOniOS overload (compiles), so this is fine — just flagging that previous runs targeted the simulator RID explicitly and now rely on the default; confirm that’s intended.
  • YAML hygiene: the two new lane blocks have trailing whitespace after ${{ else }}: (else }}: ). Cosmetic, but worth cleaning since the surrounding lanes don’t have it.
  • condition: ${{ parameters.macOSPoolLaneCondition }} on mac_runios_previous (x64) mirrors the sibling lanes — consistent. ✔️

CI

maui-pr and Build Analysis are red, driven by the previous-template lanes this PR enables (see finding #1). This is the gating item.

Confidence: high on the methodology; the gating question (real failures vs. infra) needs a maintainer to read the failing previous-template logs.

@kubaflo kubaflo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review summary — PR #33961: [ci] Enable testing building previous

Verdict: NEEDS_CHANGES (confidence: high)

All four models independently returned NEEDS_CHANGES. The PR correctly restores DotNetPrevious (net10.0) template coverage and wires up matching CI lanes, but the newly re-enabled lanes are red. claude-opus-4.8 pinpointed the likely root cause and it checks out against the code: the re-enabled Windows publish rows assert the legacy win10-x64 RID folder that net10.0 no longer produces.

Key findings:

  • Windows publish asserts a stale RID folder (error). PublishUnpackaged (lines 108/110) and PublishPackaged (lines 151/153) re-enable DotNetPrevious with usesRidGraph: true, so AssetExists looks under win10-x64/.... Post-.NET 8 the RID graph is off by default and the MAUI templates declare only win-x64 (no win10-x64 under src/Templates); BuildProps doesn't set UseRidGraph. So net10.0 publishes to win-x64 — like the passing DotNetCurrent rows that use false — and the lookups fail. Likely cause of the red WindowsTemplates lane.
  • iOS "previous" tests aren't actually parallelized (warning). RunOniOS_Previous is a single [Theory] with four configs; CI filters it as one job (FullyQualifiedName~RunOniOS_Previous), so all four build+run cycles run serially under one timeout: 45. This contradicts the file's per-[Fact] pattern and the PR's "parallel execution" claim, loses per-config attribution, and is tight on time (~55 min equivalent). Suggest splitting into four [Fact]s + four lanes.

CI: Build 1350355 is red on the two lanes this PR re-enables — WindowsTemplates and RunOniOS_Previous ARM64 — so these failures are PR-introduced and must be fixed (or the broken configs kept disabled with a tracking issue) before merge. (macOS Build (Debug)/AOT lanes are also red; their link to this PR is unclear and should be confirmed.)

[Theory]
[InlineData("maui", DotNetCurrent, "Release", false)]
//[InlineData("maui", DotNetPrevious, "Release", true)]
[InlineData("maui", DotNetPrevious, "Release", true)]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This re-enabled DotNetPrevious row passes usesRidGraph: true, so AssetExists looks under bin/Release/net10.0-windows10.0.19041.0/win10-x64/publish (line 132). But DotNetPrevious is now net10.0: post-.NET 8 the RID graph is off by default, and the MAUI Windows templates declare only the portable win-x64 RID (there is no win10-x64 anywhere under src/Templates). The publish output therefore lands in win-x64, exactly like the DotNetCurrent (net11.0) rows in this same [Theory] that pass false. As written, these checks look in a folder that won't exist — consistent with the red WindowsTemplates lane. BuildProps does not set UseRidGraph, so nothing re-enables the legacy RID here. The same stale true is on line 110 (maui-blazor).

Suggested change
[InlineData("maui", DotNetPrevious, "Release", true)]
[InlineData("maui", DotNetPrevious, "Release", false)]

[Theory]
[InlineData("maui", DotNetCurrent, "Release", false)]
//[InlineData("maui", DotNetPrevious, "Release", true)]
[InlineData("maui", DotNetPrevious, "Release", true)]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same stale-RID problem as PublishUnpackaged: this re-enabled net10.0 row passes usesRidGraph: true, so the expected path becomes bin/Release/net10.0-windows10.0.19041.0/win10-x64/AppPackages/... (line 177). Since net10.0 publishes with the portable win-x64 RID and produces no win10-x64 folder, this lookup fails. Note the packaged path is built differently here than in PublishUnpackaged (it concatenates prefix with the rid segment), so simply flipping to false yields bin/Release/net10.0-windows10.0.19041.0/AppPackages/... — please verify that matches where the MSIX actually lands for a non-RID-graph net10.0 packaged publish before settling on the value. Line 153 (maui-blazor) needs the same correction.

[InlineData("maui", "Release", DotNetPrevious, RuntimeVariant.Mono, null)]
[InlineData("maui-blazor", "Debug", DotNetPrevious, RuntimeVariant.Mono, null)]
[InlineData("maui-blazor", "Release", DotNetPrevious, RuntimeVariant.Mono, null)]
public void RunOniOS_Previous(string id, string config, string framework, RuntimeVariant runtimeVariant, string? trimMode)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

RunOniOS_Previous bundles all four net10.0 configurations into a single [Theory], but the surrounding DotNetCurrent cases are deliberately split into individual [Fact] methods (see the comment on line 74: "Individual test methods ... to enable parallel CI runs"). The CI lanes added to ci.yml map testName to --filter "FullyQualifiedName~RunOniOS_Previous" (see eng/pipelines/arcade/stage-integration-tests.yml), so one job runs all four build+run cycles serially under a single timeout: 45. That doesn't deliver the "parallel execution" the PR description claims, loses per-config failure attribution, and is tight on time — the equivalent DotNetCurrent ARM64 lanes took ~12/18/10/15 min individually (≈55 min combined). The RunOniOS_Previous ARM64 lane is currently red (failed in ~8 min). Consider splitting into four [Fact] wrappers plus four CI job pairs to match the existing pattern.

@kubaflo kubaflo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you please resolve conflicts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-testing Unit tests, device tests copilot testing-infrastructure Issue relating to testing infrastructure testing-reenable

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants