Skip to content

Commit a12e081

Browse files
openshift-merge-bot[bot]openshift-merge-bot[bot]
authored
Merge pull request #10123 from bluesort/add-skill-writing-unit-tests
NO-ISSUE: Add assisted-service-writing-unit-tests agent skill
2 parents 9347728 + 2fa2c7d commit a12e081

File tree

3 files changed

+176
-0
lines changed

3 files changed

+176
-0
lines changed

‎.claude/skills/assisted-service-writing-unit-tests.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
../../.cursor/skills/assisted-service-writing-unit-tests/SKILL.md

‎.cursor/skills/assisted-service-writing-unit-tests/SKILL.md‎

Lines changed: 169 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,169 @@
1+
---
2+
name: assisted-service-writing-unit-tests
3+
description: Use when writing unit tests in assisted-service with gomock mocks, Ginkgo/Gomega, events, or database. Critical when tests pass but mocks aren't verified, or gomock.NewController appears in code.
4+
---
5+
6+
# Writing Unit Tests in Assisted-Service
7+
8+
## Overview
9+
10+
Unit tests in assisted-service use Ginkgo/Gomega BDD framework with gomock for mocking. **Core principle: Every gomock controller MUST call `ctrl.Finish()` in `AfterEach` to verify mock expectations.**
11+
12+
Without `ctrl.Finish()`, tests pass even when mocks aren't called correctly - silent failures that hide bugs. Tests run against real PostgreSQL (suite or per-test) with automatic cleanup.
13+
14+
## Quick Reference
15+
16+
| Scenario | Pattern |
17+
|----------|---------|
18+
| Testing with mocks | Create controller in `BeforeEach`, call `ctrl.Finish()` in `AfterEach` |
19+
| Testing without mocks | Use `It()` blocks directly or table-driven tests |
20+
| Database per test | `PrepareTestDB()` in `BeforeEach`, `DeleteTestDB()` in `AfterEach` |
21+
| Database for suite | `InitializeDBTest()` in `BeforeSuite`, `TerminateDBTest()` in `AfterSuite` |
22+
| Event verification | `eventstest.NewEventMatcher` with matchers |
23+
24+
## When to Use Patterns
25+
26+
```dot
27+
digraph mock_decision {
28+
"Testing interface deps?" [shape=diamond];
29+
"Need verify calls or control returns?" [shape=diamond];
30+
"Use gomock mocks" [shape=box];
31+
"Use real implementation" [shape=box];
32+
33+
"Testing interface deps?" -> "Need verify calls or control returns?" [label="yes"];
34+
"Testing interface deps?" -> "Use real implementation" [label="no"];
35+
"Need verify calls or control returns?" -> "Use gomock mocks" [label="yes"];
36+
"Need verify calls or control returns?" -> "Use real implementation" [label="no"];
37+
}
38+
```
39+
40+
```dot
41+
digraph db_decision {
42+
"Tests share data?" [shape=diamond];
43+
"Tests modify DB state?" [shape=diamond];
44+
"Suite-level DB" [shape=box];
45+
"Per-test DB" [shape=box];
46+
47+
"Tests share data?" -> "Suite-level DB" [label="yes"];
48+
"Tests share data?" -> "Tests modify DB state?" [label="no"];
49+
"Tests modify DB state?" -> "Per-test DB" [label="yes"];
50+
"Tests modify DB state?" -> "Suite-level DB" [label="no"];
51+
}
52+
```
53+
54+
## Scope
55+
56+
**Covers:** Unit tests in `internal/` and `pkg/` with gomock, Ginkgo/Gomega, database, events.
57+
58+
**Does NOT cover:** Subsystem tests, E2E, external service integration, performance tests.
59+
60+
**Test files:** `*_test.go` (same package). Suites: `*_suite_test.go`.
61+
62+
## Critical: Gomock Controllers MUST Call Finish()
63+
64+
**Every `Describe`/`Context` creating a gomock controller needs `ctrl.Finish()` in `AfterEach`. No exceptions.**
65+
66+
Without `ctrl.Finish()`:
67+
- `.Times(N)` - Expected call count NOT checked
68+
- `.MaxTimes(0)` - Unexpected calls NOT caught
69+
- `.Do()` / `.DoAndReturn()` - Mock behavior runs without verification
70+
71+
```go
72+
BeforeEach(func() {
73+
ctrl = gomock.NewController(GinkgoT())
74+
mockHandler = eventsapi.NewMockHandler(ctrl)
75+
})
76+
77+
AfterEach(func() {
78+
ctrl.Finish() // REQUIRED - verifies all EXPECT() constraints
79+
})
80+
```
81+
82+
**Why this matters:** Missing `ctrl.Finish()` causes silent test failures - tests pass even when mocks aren't called correctly. Real bugs found in commit `338133e05 MGMT-23548`.
83+
84+
**Letter = Spirit:** Following the pattern exactly IS following the spirit. This isn't ritual - it's how gomock verification works.
85+
86+
## Database Setup Patterns
87+
88+
**Suite-level** (shared DB, read-only tests):
89+
```go
90+
BeforeSuite(func() { common.InitializeDBTest() })
91+
AfterSuite(func() { common.TerminateDBTest() })
92+
```
93+
94+
**Per-test** (isolated DB, tests modify state):
95+
```go
96+
BeforeEach(func() { db, dbName = common.PrepareTestDB() })
97+
AfterEach(func() { common.DeleteTestDB(db, dbName) })
98+
```
99+
100+
## Event Testing
101+
102+
Use `eventstest.NewEventMatcher` with specific matchers:
103+
104+
```go
105+
mockEvents.EXPECT().SendHostEvent(gomock.Any(), eventstest.NewEventMatcher(
106+
eventstest.WithNameMatcher(eventgen.HostStatusUpdatedEventName),
107+
eventstest.WithHostIdMatcher(host.ID.String())))
108+
```
109+
110+
**Matchers:** `WithNameMatcher`, `WithHostIdMatcher`, `WithClusterIdMatcher`, `WithInfraEnvIdMatcher`, `WithSeverityMatcher`
111+
112+
## Table-Driven Tests
113+
114+
**DescribeTable** (simple cases):
115+
```go
116+
DescribeTable("FunctionName",
117+
func(input string, valid bool) { /* test logic */ },
118+
Entry("descriptive case 1", "value1", true),
119+
Entry("descriptive case 2", "value2", false))
120+
```
121+
122+
**Struct array** (complex scenarios):
123+
```go
124+
tests := []struct{name, input string; valid bool}{
125+
{name: "case 1", input: "val1", valid: true}}
126+
for _, t := range tests { It(t.name, func() { /* ... */ }) }
127+
```
128+
129+
## Test Structure
130+
131+
Ginkgo hierarchy: `Describe("Component")` → `Context("when X")` → `It("should Y")`
132+
133+
**Isolation:** No shared state between `It` blocks. Use `BeforeEach` for setup.
134+
135+
**Gomock matchers:** `.Times(N)`, `.MaxTimes(0)`, `.AnyTimes()`, `gomock.Any()`
136+
137+
## Red Flags - Stop and Fix
138+
139+
These thoughts mean STOP - fix the issue:
140+
141+
- "Tests are passing, mocks must be working" → Add `ctrl.Finish()` in `AfterEach`
142+
- "Too complex to set up properly" → Follow patterns, don't skip structure
143+
- "Minimum viable test to satisfy review" → Tests must verify behavior, not theater
144+
- "I'll add cleanup/verification later" → Do it now, tests will merge broken
145+
- Code has `gomock.NewController` but no `ctrl.Finish()` → Silent failures
146+
147+
**If you're rationalizing shortcuts due to time pressure, the test will be broken. No exceptions.**
148+
149+
## Common Mistakes
150+
151+
| Mistake | Symptom | Fix |
152+
|---------|---------|-----|
153+
| Missing `ctrl.Finish()` | Tests pass when mocks not called | Add `ctrl.Finish()` in `AfterEach` |
154+
| Shared variables between tests | Flaky tests, race conditions | Move initialization to `BeforeEach` |
155+
| Missing `GinkgoT()` | Controller doesn't report failures | Use `gomock.NewController(GinkgoT())` |
156+
| Generic Entry names | "test 1", "test 2" in output | Descriptive: "valid IPv4 CIDR" |
157+
| Wrong DB pattern | Pollution between tests | Suite-level for reads, per-test for writes |
158+
| No event matchers | Generic `gomock.Any()` for events | Use `eventstest.NewEventMatcher` with specific matchers |
159+
160+
## Before Submitting
161+
162+
**Check every test file:**
163+
- [ ] Every `gomock.NewController` has `ctrl.Finish()` in `AfterEach`
164+
- [ ] No shared state (all setup in `BeforeEach`)
165+
- [ ] Descriptive names ("valid IPv4 CIDR" not "test 1")
166+
- [ ] Database cleanup if using `PrepareTestDB()`
167+
- [ ] Tests pass: `go test -v ./path/to/package`
168+
169+
**Red flags:** Missing `ctrl.Finish()` → silent failures. Shared variables → flaky tests.

‎CLAUDE.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -368,6 +368,12 @@ The project has three main test categories:
368368
- Upstream: [assisted-test-infra](https://github.com/openshift/assisted-test-infra)
369369
- Downstream: QE maintained tests
370370

371+
### Test Patterns
372+
373+
**When writing unit tests in `internal/` or `pkg/` packages**, reference: `.claude/skills/assisted-service-writing-unit-tests.md`
374+
375+
**Note**: This skill covers unit tests only. Subsystem tests follow different patterns (documented separately).
376+
371377
### Environment Variables
372378

373379
#### Test Execution

0 commit comments

Comments
 (0)