Skip to content

Commit dacd4e7

Browse files
Copilot CICopilot
andcommitted
Reject a device category that selects the reproduction out
The published device selector is TestFilter=Category=Issue<N>, and DeviceTestSharedHelpers.GetExcludedTestCategories implements that by subtraction: it lists the public static string fields of TestCategory, removes the requested one, and excludes everything left. "Issue<N>" is deliberately not a TestCategory field, so the removal is a no-op and every conventional category ends up excluded -- including the one the test itself declares. The selector then selects the test out rather than in. The authoring guidance asked for exactly that shape, telling the agent to add [Category("Issue<N>")] "in addition to any conventional TestCategory". Reviewers measured the consequence twice: an Android reproduction reported 576 discovered / 3 passed / 573 ignored, and a Mac Catalyst one executed zero tests. Deleting only the broad category made each exact test run. Windows is exempt and stays exempt: its runner filters by discovered traits, so "Issue<N>" is a real category there. Corrects the guidance, rejects the shape in both the authoring guards and the publishing gate, and fixes the doc comment that asserted the opposite. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 735ac9a2-7bec-4baa-ad19-c298e5bc795a
1 parent 7695b20 commit dacd4e7

4 files changed

Lines changed: 195 additions & 3 deletions

File tree

.github/scripts/Replicate-Issue.Tests.ps1

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13595,3 +13595,94 @@ Describe 'A CS0104 ambiguity is resolved, not re-described' {
1359513595
}
1359613596
}
1359713597

13598+
13599+
Describe 'An issue-keyed device category is the only category on skip-filtered platforms' {
13600+
# DeviceTestSharedHelpers.GetExcludedTestCategories implements
13601+
# "Category=X" by subtraction over the public static string fields of
13602+
# TestCategory. "Issue<N>" is not one of those fields, so the removal is a
13603+
# no-op and every conventional category is excluded -- which excludes any
13604+
# test that also declares one. Measured on PR 533 (Android, Shape +
13605+
# Issue31330: 576 discovered, 3 passed, 573 ignored) and PR 515
13606+
# (Mac Catalyst, Accessibility + Issue37140: zero tests executed).
13607+
13608+
It 'reports the conventional category that hides the test on <platform>' -ForEach @(
13609+
@{ platform = 'android' }
13610+
@{ platform = 'ios' }
13611+
@{ platform = 'catalyst' }
13612+
) {
13613+
$source = "public class T`n{`n`t[Category(TestCategory.Shape)]`n`t[Category(`"Issue31330`")]`n`tpublic void M() { }`n}"
13614+
Assert-ReplicationDeviceCategoryIsExclusive `
13615+
-Content $source -Path 'a.cs' -Issue 31330 -Platform $platform |
13616+
Should -Be 'TestCategory.Shape'
13617+
}
13618+
13619+
It 'reports a broad category written as a string literal' {
13620+
# PR 515's shape, verbatim: Accessibility alongside Issue37140.
13621+
$source = "public class T`n{`n`t[Category(`"Accessibility`")]`n`t[Category(`"Issue37140`")]`n`tpublic void M() { }`n}"
13622+
Assert-ReplicationDeviceCategoryIsExclusive `
13623+
-Content $source -Path 'a.cs' -Issue 37140 -Platform 'catalyst' |
13624+
Should -Be '"Accessibility"'
13625+
}
13626+
13627+
It 'reports a conventional category combined into one attribute' {
13628+
# The exact shape the old guidance recommended.
13629+
$source = "public class T`n{`n`t[Category(TestCategory.Entry, `"Issue37275`")]`n`tpublic void M() { }`n}"
13630+
Assert-ReplicationDeviceCategoryIsExclusive `
13631+
-Content $source -Path 'a.cs' -Issue 37275 -Platform 'android' |
13632+
Should -Be 'TestCategory.Entry'
13633+
}
13634+
13635+
It 'accepts the issue-keyed category on its own' {
13636+
$source = "public class T`n{`n`t[Category(`"Issue31330`")]`n`tpublic void M() { }`n}"
13637+
Assert-ReplicationDeviceCategoryIsExclusive `
13638+
-Content $source -Path 'a.cs' -Issue 31330 -Platform 'android' |
13639+
Should -BeNullOrEmpty
13640+
}
13641+
13642+
It 'exempts windows, which selects from discovered traits' {
13643+
# ControlsHeadlessTestRunner collects tc.Traits["Category"], so
13644+
# "Issue31330" is a real category there. PR 525 selected its single
13645+
# Windows test correctly with a second category present.
13646+
$source = "public class T`n{`n`t[Category(TestCategory.Shape)]`n`t[Category(`"Issue31330`")]`n`tpublic void M() { }`n}"
13647+
Assert-ReplicationDeviceCategoryIsExclusive `
13648+
-Content $source -Path 'a.cs' -Issue 31330 -Platform 'windows' |
13649+
Should -BeNullOrEmpty
13650+
}
13651+
13652+
It 'ignores a commented-out conventional category' {
13653+
$source = "public class T`n{`n`t// [Category(TestCategory.Shape)]`n`t[Category(`"Issue31330`")]`n`tpublic void M() { }`n}"
13654+
Assert-ReplicationDeviceCategoryIsExclusive `
13655+
-Content $source -Path 'a.cs' -Issue 31330 -Platform 'android' |
13656+
Should -BeNullOrEmpty
13657+
}
13658+
13659+
It 'ignores a conventional category inside a block comment' {
13660+
# The line anchor alone cannot reject this one: the attribute does
13661+
# start its own line, so only comment stripping removes it.
13662+
$source = "public class T`n{`n/*`n[Category(TestCategory.Shape)]`n*/`n`t[Category(`"Issue31330`")]`n`tpublic void M() { }`n}"
13663+
Assert-ReplicationDeviceCategoryIsExclusive `
13664+
-Content $source -Path 'a.cs' -Issue 31330 -Platform 'android' |
13665+
Should -BeNullOrEmpty
13666+
}
13667+
13668+
It 'says nothing about empty content' {
13669+
Assert-ReplicationDeviceCategoryIsExclusive `
13670+
-Content '' -Path 'a.cs' -Issue 31330 -Platform 'android' |
13671+
Should -BeNullOrEmpty
13672+
}
13673+
13674+
It 'is wired into the device-test authoring guards' {
13675+
$orchestrator = Get-Content -LiteralPath (
13676+
Join-Path $PSScriptRoot 'Replicate-Issue.ps1') -Raw
13677+
$orchestrator | Should -Match 'Assert-ReplicationDeviceCategoryIsExclusive'
13678+
# The remedy has to name the offending category, or the author cannot
13679+
# act on it without guessing which attribute to delete.
13680+
$orchestrator | Should -Match 'Declare the issue-keyed category on its'
13681+
}
13682+
13683+
It 'tells the author to keep the category alone' {
13684+
$orchestrator = Get-Content -LiteralPath (
13685+
Join-Path $PSScriptRoot 'Replicate-Issue.ps1') -Raw
13686+
$orchestrator | Should -Match 'that must be the ONLY category the test carries'
13687+
}
13688+
}

.github/scripts/Replicate-Issue.ps1

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3703,6 +3703,24 @@ function Assert-GeneratedTestContent {
37033703
-Issue $Issue) {
37043704
$deviceTestIsSelectable = $true
37053705
}
3706+
$conflict = Assert-ReplicationDeviceCategoryIsExclusive `
3707+
-Content $content `
3708+
-Path $file `
3709+
-Issue $Issue `
3710+
-Platform $TargetPlatform
3711+
if ($conflict) {
3712+
$conflictMessage = (
3713+
"Generated device test '$file' declares [Category($conflict)] " +
3714+
"alongside [Category(`"Issue$Issue`")]. On $TargetPlatform the " +
3715+
'runner implements "Category=X" by excluding every other ' +
3716+
'TestCategory field, and "Issue' + $Issue + '" is not one of ' +
3717+
"them, so [Category($conflict)] lands in the excluded list and " +
3718+
'the test is skipped. Declare the issue-keyed category on its ' +
3719+
'own so the published selector selects it.')
3720+
if (-not $guardFailures.Contains($conflictMessage)) {
3721+
$guardFailures.Add($conflictMessage)
3722+
}
3723+
}
37063724
} catch {
37073725
$message = $_.Exception.Message
37083726
if (-not $guardFailures.Contains($message)) { $guardFailures.Add($message) }
@@ -5963,7 +5981,7 @@ If the issue requests a new public event, property, method, or other API that do
59635981
Use testFilter "Maui$IssueNumber" only for XAML; otherwise use "Issue$IssueNumber".
59645982
Never assert an environment precondition. A test that calls Assert.True(OperatingSystem.IsIOSVersionAtLeast(26), ...) turns red on every device below that floor before its oracle runs, so the failure reports the lane rather than the defect and survives a complete product fix. When the reported behavior needs an OS floor, skip instead: "if (!OperatingSystem.IsIOSVersionAtLeast(26)) return;" -- the shape this repository uses at 49 sites. The same applies to throwing or Assert.Fail from an unmet version gate.
59655983
5966-
A device test must also declare [Category("Issue$IssueNumber")] on its test class, in addition to any conventional TestCategory it already carries. CategoryAttribute takes params string[] and allows multiples, so this adds a category without editing the shared TestCategory file. The stock device-test runner honours only "Category=X" and "SkipCategories=X,Y", so without this category the published selector cannot isolate the reproduction and the whole suite runs instead.
5984+
A device test must also declare [Category("Issue$IssueNumber")] on its test class, and on android, ios and catalyst that must be the ONLY category the test carries. Do not add a conventional TestCategory next to it. The stock device-test runner honours only "Category=X" and "SkipCategories=X,Y", and it implements "Category=X" by subtraction: it lists the public static string fields of TestCategory, removes X, and excludes every one that remains. "Issue$IssueNumber" is not a TestCategory field, so removing it removes nothing and every conventional category ends up excluded -- a test that also declares [Category(TestCategory.Shape)] then carries an excluded category and is skipped. Reviewers measured this twice: an Android reproduction reported 576 discovered / 3 passed / 573 ignored, and a Mac Catalyst one executed zero tests; deleting only the broad category made each exact test run. Windows selects from discovered traits instead, so a second category is harmless there, but keep the issue-keyed category alone on every platform for one publishable selector. Without this category the published selector cannot isolate the reproduction and the whole suite runs instead.
59675985
List 1-10 exact new repository-relative .cs or .xaml files. Every filename must contain "$IssueNumber", every parent directory must already exist, and every path must be under one of these roots:
59685986
$approvedRoots
59695987
$existingIssueGuidance

.github/scripts/shared/Assert-ReplicationTestGuard.ps1

Lines changed: 61 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2217,9 +2217,11 @@ function Assert-ReplicationDeviceTestIsSelectable {
22172217
sibling from muddying the verdict.
22182218
22192219
CategoryAttribute takes params string[] and allows multiples, so an
2220-
issue-keyed category sits alongside the conventional
2220+
issue-keyed category can sit alongside the conventional
22212221
[Category(TestCategory.Entry)] without touching TestCategory -- which
2222-
matters, because editing that shared file is not add-only.
2222+
matters, because editing that shared file is not add-only. On the
2223+
skip-filtered platforms that second category makes the test
2224+
unselectable; Assert-ReplicationDeviceCategoryIsExclusive rejects it.
22232225
22242226
With [Category("Issue<N>")] present, the token names a real discovered
22252227
category, so the selector published in the pull request is one the
@@ -2242,6 +2244,63 @@ function Assert-ReplicationDeviceTestIsSelectable {
22422244
return [bool]([regex]::IsMatch($Content, $pattern))
22432245
}
22442246

2247+
function Assert-ReplicationDeviceCategoryIsExclusive {
2248+
<#
2249+
.SYNOPSIS
2250+
Reports a conventional category that makes an issue-keyed device test
2251+
unselectable on the skip-filtered platforms.
2252+
2253+
.DESCRIPTION
2254+
DeviceTestSharedHelpers.GetExcludedTestCategories implements
2255+
"TestFilter=Category=X" by *subtraction*: it lists the public static
2256+
string fields of TestCategory, removes X, and excludes everything that
2257+
is left. "Issue<N>" is deliberately not a TestCategory field, so
2258+
removing it removes nothing and every conventional category ends up in
2259+
the excluded list. A test that also declares [Category(TestCategory.Shape)]
2260+
therefore carries an excluded category and is skipped -- the published
2261+
selector selects it out rather than in.
2262+
2263+
Reviewers measured exactly this twice: PR 533 (Android, Shape +
2264+
Issue31330) reported 576 discovered / 3 passed / 573 ignored, and
2265+
PR 515 (Mac Catalyst, Accessibility + Issue37140) executed zero tests.
2266+
In both cases removing only the broad category made the exact test run.
2267+
2268+
Windows is exempt. Its runner filters by *discovered* traits
2269+
(ControlsHeadlessTestRunner collects tc.Traits["Category"]), so
2270+
"Issue<N>" is a real category there and a second one is harmless --
2271+
which is why PR 525 selected its single Windows test correctly.
2272+
2273+
Returns the offending category argument, or an empty string when the
2274+
issue-keyed category is the only one.
2275+
#>
2276+
param(
2277+
[Parameter(Mandatory = $true)][AllowEmptyString()][string]$Content,
2278+
[Parameter(Mandatory = $true)][string]$Path,
2279+
[Parameter(Mandatory = $true)][int]$Issue,
2280+
[Parameter(Mandatory = $true)][AllowEmptyString()][string]$Platform
2281+
)
2282+
2283+
if ([string]::IsNullOrWhiteSpace($Content)) { return '' }
2284+
if ($Platform -notin @('android', 'ios', 'catalyst')) { return '' }
2285+
2286+
# A commented-out attribute declares nothing.
2287+
$source = Get-ReplicationCommentFreeText -Text $Content -Path $Path
2288+
$token = '"Issue' + $Issue + '"'
2289+
2290+
foreach ($match in [regex]::Matches(
2291+
$source,
2292+
'(?m)^\s*\[\s*(?:(?:[A-Za-z_]\w*)\.)*Category\s*\(([^)]*)\)')) {
2293+
foreach ($argument in ($match.Groups[1].Value -split ',')) {
2294+
$trimmed = $argument.Trim()
2295+
if (-not $trimmed) { continue }
2296+
if ($trimmed -ceq $token) { continue }
2297+
return $trimmed
2298+
}
2299+
}
2300+
2301+
return ''
2302+
}
2303+
22452304
function Get-ReplicationEnvironmentCapabilityPattern {
22462305
# Calls that report which lane the test landed in, not what the product did.
22472306
return '(?:OperatingSystem\.Is[A-Za-z]*VersionAtLeast|UIDevice\.CurrentDevice\.CheckSystemVersion|Build\.VERSION\.SdkInt)'

.github/scripts/shared/Validate-ReplicationCandidate.ps1

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2056,6 +2056,30 @@ function Assert-ReplicationCandidateSources {
20562056
) {
20572057
$deviceTestIsSelectable = $true
20582058
}
2059+
if ($Manifest.TestType -ceq 'DeviceTest') {
2060+
# A second category does not merely dilute the selector, it
2061+
# inverts it: "Category=Issue<N>" is implemented by excluding
2062+
# every other TestCategory field, so the conventional category
2063+
# this test also carries lands in the excluded list and the
2064+
# test is skipped. Publishing that selector ships a
2065+
# reproduction nobody can run.
2066+
$categoryConflict = Assert-ReplicationDeviceCategoryIsExclusive `
2067+
-Content $file.Content `
2068+
-Path $file.Path `
2069+
-Issue ([int]$Manifest.IssueNumber) `
2070+
-Platform ([string]$Manifest.Platform)
2071+
if ($categoryConflict) {
2072+
throw (
2073+
"Candidate device test '$($file.Path)' declares " +
2074+
"[Category($categoryConflict)] alongside " +
2075+
"[Category(`"Issue$($Manifest.IssueNumber)`")]. On " +
2076+
"$($Manifest.Platform) the runner implements " +
2077+
'"Category=X" by excluding every other TestCategory ' +
2078+
'field, so that conventional category is excluded and ' +
2079+
'the test is skipped. Declare the issue-keyed ' +
2080+
'category on its own.')
2081+
}
2082+
}
20592083
}
20602084
Assert-SourceTextIsSafe `
20612085
-Content $file.Content `

0 commit comments

Comments
 (0)