Add smart UI test category detection to skip irrelevant test jobs - #33176
jfversluis wants to merge 9 commits into
Conversation
6a983cf to
f060cc5
Compare
jfversluis
added
the
p/0
There was a problem hiding this comment.
Pull request overview
This PR introduces automatic UI test category detection for pull requests to optimize test execution time. When a PR adds new UI tests with [Category(...)] attributes, the pipeline detects these categories and runs only the relevant test matrix jobs, while non-matching jobs complete quickly with 0 tests.
Key Changes:
- Adds a discovery stage that parses git diffs to detect new test categories in PRs
- Implements early filtering logic to skip provisioning and test execution for non-matching category groups
- Updates all test stages to depend on category discovery and use detected categories for filtering
- Includes temporary dummy tests in ButtonUITests and LabelUITests to validate the detection mechanism
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| eng/scripts/detect-ui-test-categories.ps1 | New PowerShell script that parses git diffs to extract Category attributes from added test code |
| eng/pipelines/common/ui-tests.yml | Adds discovery stage and updates test stage dependencies to use detected categories |
| eng/pipelines/common/ui-tests-steps.yml | Implements early category check and dynamic test filter calculation based on detected categories |
| eng/pipelines/common/provision.yml | Adds skip logic to bypass provisioning when no tests will run for a category group |
| src/Controls/tests/TestCases.Shared.Tests/Tests/ButtonUITests.cs | Adds dummy test to validate Button category detection |
| src/Controls/tests/TestCases.Shared.Tests/Tests/LabelUITests.cs | Adds dummy test to validate Label category detection |
| condition: | | ||
| or( | ||
| ne(variables['Build.Reason'], 'PullRequest'), | ||
| in(dependencies.discover_ui_test_categories.result, 'Succeeded', 'Skipped') | ||
| ) |
There was a problem hiding this comment.
The condition uses in(dependencies.discover_ui_test_categories.result, 'Succeeded', 'Skipped') which allows the build to proceed even if discovery was skipped. However, for non-PR builds, the discovery stage has condition: eq(variables['Build.Reason'], 'PullRequest') which means it will be skipped, not produce any output variables. The test stages depend on these output variables (DETECTED_CATEGORIES), so they need to handle the case where these variables are empty or undefined. Verify that the downstream logic correctly handles undefined DETECTED_CATEGORIES for non-PR builds.
| condition: | | |
| or( | |
| ne(variables['Build.Reason'], 'PullRequest'), | |
| in(dependencies.discover_ui_test_categories.result, 'Succeeded', 'Skipped') | |
| ) | |
| condition: succeeded() |
| foreach ($cat in $categoryList) { | ||
| $cat = $cat.Trim() | ||
| foreach ($det in $detectedList) { | ||
| $det = $det.Trim() | ||
| if ($cat -eq $det) { | ||
| $hasMatch = $true | ||
| Write-Host "Match found: '$cat'" | ||
| break | ||
| } | ||
| } | ||
| if ($hasMatch) { break } | ||
| } |
There was a problem hiding this comment.
The category comparison at lines 52 and 224 uses case-sensitive string equality ($cat -eq $det). However, the HashSet used for collecting categories in the PowerShell detection script uses case-insensitive comparison (line 57: [System.StringComparer]::OrdinalIgnoreCase). This inconsistency could cause matching failures if category names differ in case between the detection script output and the matrix CATEGORYGROUP values. Use case-insensitive comparison here as well with -ieq or .ToLower() for consistency.
|
|
||
| $testFilter = "" | ||
| $testConfigrationArgs = "${{ parameters.testConfigurationArgs }}" | ||
|
|
||
| "${{ parameters.testFilter }}".Split(",") | ForEach { | ||
| $testFilter += "TestCategory=" + $_ + "|" | ||
|
|
||
| # Get test filter from environment variable (passed from matrix via env block) | ||
| $testFilterParam = $env:CATEGORY_GROUP | ||
| $detectedCategories = $env:DETECTED_CATEGORIES | ||
| $isPR = $env:BUILD_REASON -eq "PullRequest" | ||
|
|
||
| Write-Host "Category Group from matrix: '$testFilterParam'" | ||
| Write-Host "Detected Categories: '$detectedCategories'" | ||
|
|
||
| # Fallback to parameter for non-matrix builds | ||
| if (-not $testFilterParam) { | ||
| $testFilterParam = "${{ parameters.testFilter }}" | ||
| Write-Host "Using testFilter parameter: '$testFilterParam'" | ||
| } | ||
|
|
||
| # For PRs with detected categories, use only matching categories | ||
| if ($isPR -and -not [string]::IsNullOrWhiteSpace($detectedCategories) -and -not [string]::IsNullOrWhiteSpace($testFilterParam)) { | ||
| $categoryList = $testFilterParam -split "," | ||
| $detectedList = $detectedCategories -split "," | ||
| $matchingCategories = @() | ||
|
|
||
| foreach ($cat in $categoryList) { | ||
| $cat = $cat.Trim() | ||
| foreach ($det in $detectedList) { | ||
| $det = $det.Trim() | ||
| if ($cat -eq $det) { | ||
| $matchingCategories += $cat | ||
| break | ||
| } | ||
| } | ||
| } | ||
|
|
||
| $testFilterParam = $matchingCategories -join "," | ||
| Write-Host "Running matching categories: $testFilterParam" | ||
| } | ||
|
|
||
| Write-Host "Final test filter: '$testFilterParam'" | ||
|
|
||
| if ($testFilterParam) { | ||
| $testFilterParam.Split(",") | ForEach { | ||
| $testFilter += "TestCategory=" + $_ + "|" | ||
| } | ||
| $testFilter = $testFilter.TrimEnd("|") | ||
| } | ||
|
|
||
| $testFilter = $testFilter.TrimEnd("|") | ||
|
|
There was a problem hiding this comment.
There is duplicated category matching logic between the early check step (lines 20-69) and the test filter calculation step (lines 200-233). Both steps perform the same category matching operation with identical logic. Consider extracting this into a shared function or removing one of the checks. Since the early check already determines whether tests should run and sets SHOULD_RUN_TESTS, the test filter calculation step could be simplified to only handle the filter string construction when SHOULD_RUN_TESTS is True.
| - pwsh: | | ||
| $testFilterParam = $env:CATEGORY_GROUP | ||
| $testFilterFallback = $env:TEST_FILTER_PARAM | ||
| $detectedCategories = $env:DETECTED_CATEGORIES | ||
| $isPR = $env:BUILD_REASON -eq "PullRequest" | ||
|
|
||
| Write-Host "=== Early Category Check ===" | ||
| Write-Host "Build Reason: $env:BUILD_REASON" | ||
| Write-Host "Category Group (from matrix): '$testFilterParam'" | ||
| Write-Host "Test Filter (from parameter): '$testFilterFallback'" | ||
| Write-Host "Detected Categories: '$detectedCategories'" | ||
|
|
||
| # Use testFilter parameter as fallback when CATEGORYGROUP is not set or not expanded | ||
| # When CATEGORYGROUP variable doesn't exist, it shows as literal '$(CATEGORYGROUP)' | ||
| $categoryGroupNotSet = [string]::IsNullOrWhiteSpace($testFilterParam) -or $testFilterParam -eq '$(CATEGORYGROUP)' | ||
| if ($categoryGroupNotSet -and -not [string]::IsNullOrWhiteSpace($testFilterFallback)) { | ||
| $testFilterParam = $testFilterFallback | ||
| Write-Host "Using testFilter parameter as category: '$testFilterParam'" | ||
| } | ||
|
|
||
| $shouldRun = $true | ||
|
|
||
| # For PRs with detected categories, check if this category group has any matches | ||
| if ($isPR -and -not [string]::IsNullOrWhiteSpace($detectedCategories) -and -not [string]::IsNullOrWhiteSpace($testFilterParam)) { | ||
| $categoryList = $testFilterParam -split "," | ||
| $detectedList = $detectedCategories -split "," | ||
| $hasMatch = $false | ||
|
|
||
| foreach ($cat in $categoryList) { | ||
| $cat = $cat.Trim() | ||
| foreach ($det in $detectedList) { | ||
| $det = $det.Trim() | ||
| if ($cat -eq $det) { | ||
| $hasMatch = $true | ||
| Write-Host "Match found: '$cat'" | ||
| break | ||
| } | ||
| } | ||
| if ($hasMatch) { break } | ||
| } | ||
|
|
||
| if (-not $hasMatch) { | ||
| $shouldRun = $false | ||
| Write-Host "##[warning]No matching categories - SKIPPING all steps for this job" | ||
| Write-Host "Category group '$testFilterParam' does not contain any detected categories: $detectedCategories" | ||
| } | ||
| } | ||
|
|
||
| Write-Host "Should run tests: $shouldRun" | ||
| Write-Host "##vso[task.setvariable variable=SHOULD_RUN_TESTS]$shouldRun" |
There was a problem hiding this comment.
The early check step could skip the job entirely if SHOULD_RUN_TESTS is False, but instead it sets a variable and forces all subsequent steps to check this condition. Azure Pipelines supports job-level conditions that could terminate the job earlier. Consider using a job-level condition expression or setting the job result to "Skipped" when no matching categories are found, rather than requiring every subsequent step to check SHOULD_RUN_TESTS.
f9ec697 to
5594765
Compare
|
/azp run maui-pr-uitests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
PureWeen
left a comment
PureWeen
left a comment
There was a problem hiding this comment.
Chatted with @jfversluis about some changes to be made here
github-project-automation
Bot
moved this from Todo
to Changes Requested
in MAUI SDK Ongoing
|
Added a
This lets reviewers force full coverage on a PR without having to push a no-op commit or hand-edit pipeline parameters. We'll need to create the |
|
Added a How to test:
Leaving the parameter empty/whitespace falls back to the normal PR-build behavior, so it's a no-op for real PR validation. |
🔍 Skill Validation Results❌ Static Checks FailedSkills checked: 15 | Agents checked: 0 Full validator output✅ LLM Evaluation Passed |
🔍 Multi-Model Code Review — PR #33176PR: Add PR category detection for UI tests
CI Status⏳ PENDING — ❌ Skill Validation is failing — this appears related to the PR's changes to Prior Review Status
🔴 CRITICAL1. Hardcoded
|
| Finding | Flagged By | Adversarial Result | Notes |
|---|---|---|---|
throw on unrecognized [Category] blocks all tests |
1 reviewer | Disputed — intentional fail-fast design, zero triggering instances in codebase, failure is visible (not silent) | Consider Write-Warning + fallback instead of throw for robustness |
Regex ^\+\s*\[Category\( misses multi-attribute lines |
1 reviewer | Disputed — only 1 instance in entire test suite uses combined pattern, guidelines explicitly forbid it | Theoretical edge case, not practical risk |
contains() substring match for CV1/CarV1 stage conditions |
1 reviewer | Not verified | Fragile if future category names overlap (e.g., CollectionViewLayout matches CollectionView) |
provision.yml skip-check runs for ALL pipelines, not just UI tests |
1 reviewer | Not verified | Harmless but adds noise to non-UI pipeline logs |
Shallow clone depth=200 silently falls back on long-lived PRs |
1 reviewer | Not verified | Fallback to run-all is correct behavior; could add ##[warning] |
Unauthenticated polling loop in trigger-uitest-pipeline.ps1 |
1 reviewer | Not verified | Works for public project; would fail silently if project goes private |
$matches shadows PowerShell automatic variable |
1 reviewer | Not verified | Scoping prevents bugs; cosmetic concern |
Missing newline at EOF in detect-ui-test-categories.ps1 |
1 reviewer | Not verified | POSIX compatibility nit |
Space ' ' defaults instead of '' for pipeline parameters |
1 reviewer | Not verified | Confusing UX in AzDO queue dialog |
Empty testFilter edge case when MATCHING_CATEGORIES is empty |
1 reviewer | Not verified | Falls back to running all categories in the group — correct but slower |
Test Coverage Assessment
This PR is primarily CI/CD pipeline infrastructure (YAML templates + PowerShell scripts). There are no automated tests for the detection logic itself (detect-ui-test-categories.ps1). The PR previously included dummy tests in ButtonUITests.cs and LabelUITests.cs for manual validation, which appear to have been removed. The prNumber queue-time parameter was added for end-to-end validation.
Recommendation: Consider adding Pester tests for detect-ui-test-categories.ps1 to validate regex matching, tier fallback logic, and edge cases (empty diff, no test files, combined attributes).
Recommended Action
- [Must fix] Change
$SourceBranchdefault from"feature/detect-uitest-categories"to"main"intrigger-uitest-pipeline.ps1— this is a post-merge time bomb - [Should fix] Add stage-level
NONEguards to all test stages (not just cv1/carv1) to avoid wasting CI agent allocations on docs-only PRs - [Should fix] Move PAT usage out of the Copilot agent container in
ci-copilot.yml— runtrigger-uitest-pipeline.ps1in a pre-agentsteps:block instead - [Should fix] Remove or wire up
UITestCategoryMatrix— dead output variables create confusion - [Nice to have] Stop logging PAT prefix characters in
trigger-uitest-pipeline.ps1
Items 1 is a blocking issue. Items 2-4 are strongly recommended before merge. Item 5 is a minor improvement.
551b9b3 to
42a78ef
Compare
…orkflow 3-tier category detection (test attrs, source paths, AI reasoning), AzDO pipeline filtering, result comments, and integration with the PR review workflow (trigger early, collect after try-fix). Includes: - eng/scripts/detect-ui-test-categories.ps1 (3-tier detection) - eng/pipelines/common/ui-tests-steps.yml (per-job filter gate) - eng/pipelines/common/ui-tests.yml (discovery stage) - eng/pipelines/ci-uitests.yml (prNumber + categories params) - eng/pipelines/ci-copilot.yml (DNCENG_PUBLIC_PAT for cross-org) - .github/scripts/trigger-uitest-pipeline.ps1 (orchestrator) - .github/scripts/post-uitest-categories-comment.ps1 (results comment) - .github/scripts/Review-PR.ps1 (Step 0.5 trigger, Step 2.5 collect) - .github/scripts/post-ai-summary-comment.ps1 (UI Tests section) - .github/pr-review/pr-preflight.md (AI category identification) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
42a78ef to
7437402
Compare
kubaflo
changed the title
Addressing review feedbackThe PR has been significantly rewritten since the original review. Here's how the current implementation addresses the feedback: Copilot review suggestions — status:
Major changes since original review:
@PureWeen @jfversluis — ready for re-review when you get a chance. |
Lightweight ubuntu job runs detect + queue independently from the main CopilotReview job, making it easy to test the PAT and pipeline integration without waiting for the full review. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This reverts commit fce147e.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Tests now run the PR's actual code (refs/pull/N/merge) instead of main or the feature branch. This ensures test results reflect the PR's changes, including new tests and snapshot baselines. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ENV ERROR (emulator timeout, ADB failure, app crash) now triggers automatic retry with 30s delay. Real failures (test logic) don't retry. Checks verification-report.md for 'ENV ERROR' pattern. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Step 0.5 now only detects categories (no pipeline trigger/monitor). Removed Step 2.5 (collect results) and -SkipUITests parameter. The actual UI test pipeline runs separately via PR CI. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Check verification-report.md for proper format before using it. If report has old/broken format (Passed: False, empty Total/Failed), generate a clean fallback instead. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
kubaflo
changed the title
|
Closing this PR — superseded by a new clean PR with the same branch (coming next). |
github-project-automation
Bot
moved this from Changes Requested
to Done
in MAUI SDK Ongoing
|
This PR has been superseded by #35133 which has a clean commit history. |
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
What this does
Before: Every PR runs ~24,000 UI tests across all categories (~2+ hours).
After: The pipeline detects which UI test categories a PR actually touches and only runs those. A PR that changes
Buttoncode only runs Button tests (~400 tests, ~30 min).How it works
When
maui-pr-uitestsruns for a PR:[Category(UITestCategories.X)]in changed test filesShell/→ Shell,Button*→ Button)Escape hatches
run-all-uitestslabel to force the full matrixcategoriespipeline parameterFiles changed
eng/scripts/detect-ui-test-categories.ps1eng/pipelines/common/ui-tests.ymleng/pipelines/common/ui-tests-steps.ymleng/pipelines/ci-uitests.ymlprNumberandcategoriesparameters for manual queueeng/pipelines/ci-copilot.ymlDNCENG_PUBLIC_PATfor cross-org build queuing.github/scripts/post-uitest-categories-comment.ps1.github/scripts/trigger-uitest-pipeline.ps1.github/scripts/Review-PR.ps1.github/scripts/post-ai-summary-comment.ps1.github/pr-review/pr-preflight.mdTested on
Validated on 30+ PRs. Examples of targeted runs vs full matrix: