Skip to content

Commit 909830b

Browse files
committed
Split validate-test-correctness into 3 focused skills
Split the combined skill into: - assess-test-type: Determines if tests should be UI or unit tests - validate-ui-tests: Validates UI tests with Appium/device - validate-unit-tests: Validates unit tests with dotnet test Updated pr-reviewer-detailed agent to orchestrate all 5 skills. Updated copilot-instructions.md with new skill references.
1 parent 556789d commit 909830b

6 files changed

Lines changed: 656 additions & 228 deletions

File tree

.github/agents/pr-reviewer-detailed.md

Lines changed: 45 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -142,10 +142,27 @@ Before looking at PR's diff, determine:
142142

143143
## Phase 2: Validation
144144

145-
### Step 6: Validate UI Tests (Skill: validate-uitest-correctness)
145+
### Step 6: Assess Test Type (Skill: assess-test-type)
146146

147-
Ensure the PR's tests actually test the right thing:
147+
First, determine if the PR's tests are the right type:
148148

149+
```bash
150+
# Find test files in PR
151+
git diff main --name-only | grep -E "Test|Issue"
152+
153+
# UI tests: TestCases.HostApp/, TestCases.Shared.Tests/
154+
# Unit tests: *.UnitTests/
155+
```
156+
157+
Ask:
158+
- Does the test require visual rendering? → UI test
159+
- Can it run without a device? → Unit test
160+
161+
### Step 7: Validate Tests (Skill: validate-ui-tests OR validate-unit-tests)
162+
163+
Based on test type, use the appropriate validation skill:
164+
165+
**For UI Tests:**
149166
```bash
150167
# Run tests WITH fix (should pass)
151168
pwsh .github/scripts/BuildAndRunHostApp.ps1 -Platform ios -TestFilter "IssueXXXXX"
@@ -158,7 +175,17 @@ pwsh .github/scripts/BuildAndRunHostApp.ps1 -Platform ios -TestFilter "IssueXXXX
158175
grep -A20 "Failed\|Assert" CustomAgentLogsTmp/UITests/test-output.log
159176
```
160177

161-
### Step 7-8: Test All Approaches (Skill: compare-fix-approaches)
178+
**For Unit Tests:**
179+
```bash
180+
# Run tests WITH fix (should pass)
181+
dotnet test path/to/UnitTests.csproj --filter "TestName"
182+
183+
# Revert fix, run tests (should fail)
184+
git checkout main -- path/to/fix/files
185+
dotnet test path/to/UnitTests.csproj --filter "TestName"
186+
```
187+
188+
### Step 8-9: Test All Approaches (Skill: compare-fix-approaches)
162189

163190
For each approach (yours AND the PR's):
164191

@@ -256,19 +283,27 @@ This review was conducted through an interactive deep-dive session:
256283

257284
## Skills Used by This Agent
258285

259-
This agent orchestrates three skills:
286+
This agent orchestrates five skills:
287+
288+
1. **assess-test-type** (`.github/skills/assess-test-type/SKILL.md`)
289+
- Determines if tests should be UI tests or unit tests
290+
- Provides decision framework based on what's being tested
291+
292+
2. **validate-ui-tests** (`.github/skills/validate-ui-tests/SKILL.md`)
293+
- Validates UI tests fail without fix and pass with fix
294+
- Uses BuildAndRunHostApp.ps1 and Appium
260295

261-
1. **validate-test-correctness** (`.github/skills/validate-test-correctness/SKILL.md`)
262-
- Validates test type appropriateness (UI test vs unit test)
263-
- Ensures tests correctly catch regressions (fail without fix, pass with fix)
296+
3. **validate-unit-tests** (`.github/skills/validate-unit-tests/SKILL.md`)
297+
- Validates unit tests fail without fix and pass with fix
298+
- Uses dotnet test (no device needed)
264299

265-
2. **independent-fix-analysis** (`.github/skills/independent-fix-analysis/`)
300+
4. **independent-fix-analysis** (`.github/skills/independent-fix-analysis/SKILL.md`)
266301
- Forms independent opinion before looking at PR
267302
- Researches git history to find root cause
268303
- Proposes alternative approaches
269304

270-
3. **compare-fix-approaches** (`.github/skills/compare-fix-approaches/`)
271-
- Tests all approaches against same UI tests
305+
5. **compare-fix-approaches** (`.github/skills/compare-fix-approaches/SKILL.md`)
306+
- Tests all approaches against same tests
272307
- Creates comparison table
273308
- Recommends best approach with justification
274309

.github/copilot-instructions.md

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,7 @@ The repository includes specialized custom agents for specific tasks. These agen
198198
- **Use when**: User wants thorough analysis with alternative fix proposals
199199
- **Capabilities**: Independent root cause analysis, alternative fix implementation, comparative testing
200200
- **Trigger phrases**: "deep review PR #XXXXX", "detailed review", "analyze and propose alternatives", "challenge this PR's approach"
201-
- **Skills used**: `validate-test-correctness`, `independent-fix-analysis`, `compare-fix-approaches`
201+
- **Skills used**: `assess-test-type`, `validate-ui-tests`, `validate-unit-tests`, `independent-fix-analysis`, `compare-fix-approaches`
202202

203203
4. **uitest-coding-agent** - Specialized agent for writing new UI tests for .NET MAUI with proper syntax, style, and conventions
204204
- **Use when**: Creating new UI tests or updating existing ones
@@ -215,22 +215,32 @@ The repository includes specialized custom agents for specific tasks. These agen
215215

216216
Skills are modular capabilities that agents can invoke. Located in `.github/skills/`:
217217

218-
1. **validate-test-correctness** (`.github/skills/validate-test-correctness/SKILL.md`)
219-
- **Purpose**: Validates test type appropriateness (UI test vs unit test) and ensures tests correctly catch regressions
220-
- **Trigger phrases**: "validate the tests", "should this be a UI test or unit test", "check test correctness"
218+
1. **assess-test-type** (`.github/skills/assess-test-type/SKILL.md`)
219+
- **Purpose**: Determines whether tests should be UI tests or unit tests
220+
- **Trigger phrases**: "should this be a UI test or unit test", "what type of test is appropriate"
221221
- **Used by**: `pr-reviewer-detailed` agent
222222

223-
2. **independent-fix-analysis** (`.github/skills/independent-fix-analysis/SKILL.md`)
223+
2. **validate-ui-tests** (`.github/skills/validate-ui-tests/SKILL.md`)
224+
- **Purpose**: Validates UI tests correctly fail without fix and pass with fix
225+
- **Trigger phrases**: "validate the UI tests", "check if UI tests catch the regression"
226+
- **Used by**: `pr-reviewer-detailed` agent
227+
228+
3. **validate-unit-tests** (`.github/skills/validate-unit-tests/SKILL.md`)
229+
- **Purpose**: Validates unit tests correctly fail without fix and pass with fix
230+
- **Trigger phrases**: "validate the unit tests", "check if unit tests catch the regression"
231+
- **Used by**: `pr-reviewer-detailed` agent
232+
233+
4. **independent-fix-analysis** (`.github/skills/independent-fix-analysis/SKILL.md`)
224234
- **Purpose**: Analyze issue and propose fixes independently before comparing with PR
225235
- **Trigger phrases**: "analyze this issue independently", "propose your own fix"
226236
- **Used by**: `pr-reviewer-detailed` agent
227237

228-
3. **compare-fix-approaches** (`.github/skills/compare-fix-approaches/SKILL.md`)
238+
5. **compare-fix-approaches** (`.github/skills/compare-fix-approaches/SKILL.md`)
229239
- **Purpose**: Compare multiple fix approaches by testing against same UI tests
230240
- **Trigger phrases**: "compare the approaches", "which fix is better"
231241
- **Used by**: `pr-reviewer-detailed` agent
232242

233-
4. **find-reviewable-pr** (`.github/skills/find-reviewable-pr/SKILL.md`)
243+
6. **find-reviewable-pr** (`.github/skills/find-reviewable-pr/SKILL.md`)
234244
- **Purpose**: Find open PRs that are ready for review based on platform, recency, complexity, and project status
235245
- **Trigger phrases**: "find a PR to review", "find an easy Android PR", "what PRs are available for review"
236246
- **Used by**: Any agent or direct invocation
Lines changed: 177 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,177 @@
1+
---
2+
name: assess-test-type
3+
description: Determines whether a test should be a UI test or unit test based on what behavior is being validated.
4+
metadata:
5+
author: dotnet-maui
6+
version: "1.0"
7+
compatibility: Requires ability to read and analyze C# test code.
8+
---
9+
10+
# Assess Test Type
11+
12+
This skill determines the appropriate test type (UI test vs unit test) for tests in a PR based on what behavior they're validating.
13+
14+
## When to Use
15+
16+
- "Should this be a UI test or unit test?"
17+
- "What type of test is appropriate here?"
18+
- "Assess the test type for this PR"
19+
- Before running test validation
20+
21+
## Test Type Decision Framework
22+
23+
### When to Use Unit Tests
24+
25+
Unit tests (xUnit) are appropriate when testing:
26+
27+
| Scenario | Example | Location |
28+
|----------|---------|----------|
29+
| Pure logic/algorithms | String parsing, math calculations | `*.UnitTests.csproj` |
30+
| Property change notifications | `INotifyPropertyChanged` behavior | `Controls.Core.UnitTests` |
31+
| Binding expressions | Binding path resolution | `Controls.Core.UnitTests` |
32+
| XAML parsing | XAML to object graph | `Controls.Xaml.UnitTests` |
33+
| View model behavior | Command execution, state changes | `*.UnitTests.csproj` |
34+
| Platform-agnostic handlers | Handler mapping logic | `Core.UnitTests` |
35+
36+
**Key indicators for unit tests:**
37+
- ✅ No visual rendering required
38+
- ✅ No platform-specific behavior being tested
39+
- ✅ Can mock dependencies
40+
- ✅ Tests run in milliseconds
41+
- ✅ No device/simulator needed
42+
43+
### When to Use UI Tests
44+
45+
UI tests (NUnit + Appium) are appropriate when testing:
46+
47+
| Scenario | Example | Location |
48+
|----------|---------|----------|
49+
| Visual rendering | Element appears on screen | `TestCases.HostApp` + `TestCases.Shared.Tests` |
50+
| User interaction | Tap, scroll, gesture response | `TestCases.Shared.Tests` |
51+
| Platform-specific behavior | iOS SafeArea, Android back button | `TestCases.Shared.Tests` |
52+
| Layout measurement | Element size, position | `TestCases.Shared.Tests` |
53+
| Navigation | Page transitions, Shell routing | `TestCases.Shared.Tests` |
54+
| Focus/keyboard | Entry focus, keyboard appearance | `TestCases.Shared.Tests` |
55+
56+
**Key indicators for UI tests:**
57+
- ✅ Requires visual tree to be rendered
58+
- ✅ Tests user-visible behavior
59+
- ✅ Platform-specific rendering matters
60+
- ✅ Interaction timing matters
61+
- ✅ Needs real device/simulator
62+
63+
## Instructions
64+
65+
### Step 1: Identify Test Files in PR
66+
67+
```bash
68+
# Find test files
69+
git diff main --name-only | grep -E "Test|Issue"
70+
71+
# Categorize by location
72+
# UI tests: TestCases.HostApp/, TestCases.Shared.Tests/
73+
# Unit tests: *.UnitTests/
74+
```
75+
76+
### Step 2: Analyze Each Test
77+
78+
For each test, answer these questions:
79+
80+
1. **Does it require rendering on screen?**
81+
- YES → Likely UI test
82+
- NO → Likely unit test
83+
84+
2. **Does it test user interaction?**
85+
- YES → UI test
86+
- NO → Could be either
87+
88+
3. **Is it testing platform-specific behavior?**
89+
- YES → UI test
90+
- NO → Likely unit test
91+
92+
4. **Can it run without a device/simulator?**
93+
- YES → Unit test
94+
- NO → UI test
95+
96+
### Step 3: Decision Flowchart
97+
98+
```
99+
Does it require rendering on screen?
100+
├── NO → Unit Test
101+
└── YES → Does it test user interaction?
102+
├── YES → UI Test
103+
└── NO → Does platform-specific rendering matter?
104+
├── YES → UI Test
105+
└── NO → Could be Unit Test (prefer Unit if possible)
106+
```
107+
108+
### Step 4: Check Current vs Recommended
109+
110+
Compare what the PR has vs what it should have:
111+
112+
```bash
113+
# Check current test type
114+
ls src/Controls/tests/TestCases.HostApp/Issues/Issue*.xaml 2>/dev/null && echo "Has UI test"
115+
ls src/Controls/tests/*UnitTests/**/Issue*.cs 2>/dev/null && echo "Has unit test"
116+
```
117+
118+
## Output Format
119+
120+
```markdown
121+
## Test Type Assessment
122+
123+
### Tests in PR
124+
125+
| Test | Current Type | Recommended | Reasoning |
126+
|------|--------------|-------------|-----------|
127+
| [TestName] | UI Test | UI Test ✅ | Requires user interaction |
128+
| [TestName] | UI Test | Unit Test ❌ | Pure logic, no rendering needed |
129+
| [TestName] | Unit Test | Unit Test ✅ | Tests binding resolution |
130+
131+
### Recommendation
132+
133+
- ✅ Test types are appropriate
134+
- OR
135+
- ⚠️ Consider changing [TestName] from [current] to [recommended] because [reason]
136+
137+
### Next Steps
138+
139+
Based on test types present:
140+
- For UI tests → Use `validate-ui-tests` skill
141+
- For unit tests → Use `validate-unit-tests` skill
142+
```
143+
144+
## Examples
145+
146+
### Example 1: RadioButton Binding in CollectionView
147+
148+
**Issue**: RadioButtonGroup.SelectedValue binding doesn't update when RadioButton is checked inside CollectionView
149+
150+
**Analysis**:
151+
- Requires visual rendering? YES (RadioButtons must appear)
152+
- Tests user interaction? YES (tapping RadioButton)
153+
- Platform-specific? NO (cross-platform binding issue)
154+
155+
**Verdict**: UI Test ✅
156+
157+
### Example 2: String Parsing Logic
158+
159+
**Issue**: Custom markup extension fails to parse certain string formats
160+
161+
**Analysis**:
162+
- Requires visual rendering? NO
163+
- Tests user interaction? NO
164+
- Platform-specific? NO
165+
166+
**Verdict**: Unit Test ✅
167+
168+
### Example 3: Layout Calculation
169+
170+
**Issue**: Grid column width incorrect when using star sizing
171+
172+
**Analysis**:
173+
- Requires visual rendering? YES (layout must be measured)
174+
- Tests user interaction? NO
175+
- Platform-specific? MAYBE (could differ per platform)
176+
177+
**Verdict**: UI Test ✅ (layout measurement requires rendering)

0 commit comments

Comments
 (0)