Skip to content

Test decision guide #53

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
kubaflo wants to merge 2 commits into from
Closed

Test decision guide #53

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e5e5f02
Create test-type-decision-guide.md
kubaflo Dec 1, 2025
ddf1d90
Create Lessons-Learned-PR32939.md
kubaflo Dec 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
224 changes: 224 additions & 0 deletions .github/instructions/Lessons-Learned-PR32939.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,224 @@
# Lessons Learned: PR #32939 Testing Approach

## What Happened

**Task**: Review and test PR #32939 - Fix Slider and Stepper property order independence

**What I Did**:
1. ✅ Analyzed PR changes and understood the fix
2. ✅ Created comprehensive Sandbox test scenario
3. ✅ Wrote Appium automation scripts
4. ✅ Validated fix on Android device
5. ✅ Wrote detailed review document
6. ✅ Created PR with test scenario

**What I Did Wrong**:
❌ **Used Sandbox + UI Tests when Unit Tests were the right choice**

---

## Why This Was Wrong

### The PR Already Had Unit Tests

**PR #32939 included**:
- 98 comprehensive unit tests (39 Slider + 59 Stepper)
- Tests for all 6 property order permutations
- Tests for value preservation across range changes
- Tests for edge cases

**What this meant**:
- ✅ Author already chose unit tests as the appropriate test type
- ✅ Unit tests were sufficient to validate the fix
- ✅ No visual/layout verification needed
- ❌ My Sandbox/UI approach was redundant and time-consuming

### The Fix Was Pure Logic, Not Visual

**What the PR fixed**:
- Property initialization order independence
- Value preservation when range changes
- Event firing order

**What this means**:
- ✅ Can be tested with `new Slider()` and property assertions
- ✅ No rendering, layout, or platform handlers involved
- ✅ No visual appearance changes
- ❌ Doesn't require device/simulator
- ❌ Doesn't need Appium automation

### Sandbox Approach Had No Advantage

**What Sandbox testing provided**:
- ✅ Visual confirmation (nice to have)
- ✅ Device logs showing values
- ❌ No additional coverage beyond unit tests
- ❌ Much slower (build + deploy + run)
- ❌ More complex setup (Appium, device)
- ❌ More brittle (device dependencies)

**What unit tests provide**:
- ✅ Same validation (property values)
- ✅ Faster execution (milliseconds)
- ✅ Easier debugging
- ✅ Run in CI without devices
- ✅ Already written by PR author!

---

## What I Should Have Done

### Correct Approach for PR #32939

**Step 1**: Analyze PR
```bash
# Check PR files
gh pr diff 32939 --name-only

# Output shows:
src/Controls/src/Core/Slider/Slider.cs
src/Controls/src/Core/Stepper/Stepper.cs
src/Controls/tests/Core.UnitTests/SliderTests.cs [+112 tests]
src/Controls/tests/Core.UnitTests/StepperUnitTests.cs [+139 tests]
```

**Observation**: PR author added 251 lines of unit tests. This is a clear signal.

**Step 2**: Run existing unit tests
```bash
dotnet test src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj \
--filter "FullyQualifiedName~Slider" \
--verbosity normal
```

**Step 3**: Verify tests pass and cover the scenarios
```
✅ 98 tests passed
✅ All property orders tested
✅ Value preservation tested
✅ Edge cases covered
```

**Step 4**: Write review
```markdown
## PR Testing Summary

**PR**: #32939 - Fix Slider and Stepper property order independence
**Test Type**: Unit Tests (98 tests added by author)

### Test Validation

Ran comprehensive unit test suite:
- ✅ All 98 tests pass
- ✅ All 6 property order permutations covered
- ✅ Value preservation across range changes validated
- ✅ Edge cases (default values, multiple changes) tested

### Code Review

[Same technical analysis as I did]

### Verdict

✅ **APPROVE AND MERGE**
- Fix is well-designed and minimal
- Comprehensive unit test coverage
- All tests passing
- Low risk, high community impact
```

**Total Time**: ~10 minutes vs. ~45 minutes with Sandbox approach

---

## How to Avoid This in the Future

### Decision Framework

**When reviewing a PR, ask in this order**:

1. **Does PR already include tests?**
- Yes, unit tests → Run them, validate coverage, done ✅
- Yes, UI tests → Review them, run if possible
- No tests → Continue to step 2

2. **What is being changed?**
- Control properties/logic → Unit tests
- Visual appearance/layout → UI tests
- Platform handlers → UI tests
- Navigation/gestures → UI tests

3. **Can I test without UI?**
- Yes → Unit tests
- No → UI tests or Sandbox

### Red Flags I Missed

**Signals that unit tests were correct**:
- ✅ PR diff shows `Core.UnitTests/SliderTests.cs` [+112 lines]
- ✅ PR description focuses on "property order" and "value preservation"
- ✅ No mention of visual appearance or layout
- ✅ Only control source files modified (no handlers)
- ✅ Changes to `BindableProperty` definitions

**I should have stopped and thought**:
> "The PR author wrote 98 unit tests. They clearly believe unit tests are appropriate. Why am I creating a Sandbox UI test?"

---

## Updated Agent Instructions

I've created a new guide: `.github/instructions/test-type-decision-guide.md`

**Key additions**:
1. **Decision flowchart** - Visual guide for choosing test type
2. **Decision matrix** - Table of scenarios → test types
3. **Detailed decision rules** - When to use unit vs UI tests
4. **PR analysis clues** - How to recognize from PR changes
5. **Case study of PR #32939** - This exact scenario analyzed
6. **Updated decision process** - Step-by-step with examples

**Key rule added**:
> **Rule #1: Follow the PR Author's Lead**
> If PR includes unit tests → Add more unit tests
> If PR includes UI tests → Add more UI tests
> If PR includes no tests → Analyze and decide

---

## Positive Outcomes

**What I did well**:
- ✅ Thorough technical analysis of the fix
- ✅ Comprehensive review document
- ✅ Validated fix works correctly
- ✅ Clear documentation of test approach

**What the Sandbox work provided**:
- ✅ Visual confirmation for user's peace of mind
- ✅ Demonstrated fix in action
- ✅ Device logs showing property values
- ✅ Can be used for manual exploration if needed

**Not wasted, just not optimal**:
- The Sandbox scenario is still useful for manual testing
- The review document is comprehensive and valuable
- I validated the fix thoroughly (even if inefficiently)
- User can use Sandbox for additional exploration

---

## Summary

**Lesson**: When a PR includes 98 unit tests, that's a strong signal that unit tests are the right approach. Don't default to Sandbox/UI tests just because it's what I'm familiar with.

**Action**: Created comprehensive decision guide to avoid this in future sessions.

**Result**: Future PR reviews will be faster and more appropriate to the PR type.

---

**Related Documents**:
- `.github/instructions/test-type-decision-guide.md` - Complete decision framework
- `CustomAgentLogsTmp/PR32939-Review.md` - Technical review (still valuable)
- `CustomAgentLogsTmp/PR-Creation-Summary.md` - Sandbox PR details (optional artifact)
Loading