Skip to content
Merged
Changes from all commits
Commits
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
25 changes: 25 additions & 0 deletions .claude/skills/code-formatting/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ description: "MANDATORY: When writing Go tests, you MUST use 'When...it should..

## Test Conventions - MANDATORY

### Test naming

**NON-NEGOTIABLE RULE**: Every Go test name MUST follow this exact format:
```go
name: "When <condition>, it should <expected behavior>"
Expand All @@ -35,7 +37,30 @@ name: "When encryption is enabled, it should configure disk encryption set"
- ✅ "When NodePool has ImageID, it should create basic Azure machine template"
- ✅ "When subnet ID is invalid, it should return error with subnet parse message"

### Unit test function naming

Unit test functions MUST be named after the function they test, using the standard Go `Test<FunctionName>` convention:

```go
// Testing function ReconcileNodePool
func TestReconcileNodePool(t *testing.T) { ... }

// Testing function buildAzureMachineTemplate
func TestBuildAzureMachineTemplate(t *testing.T) { ... }
```

**NEVER use generic or disconnected test function names like:**
- ❌ `TestNodePoolFeatures`
- ❌ `TestAzureIntegration`
- ❌ `TestVariousCases`

Each test function should map 1:1 to the function under test. If a function is complex enough to warrant many test cases, use table-driven tests within a single `Test<FunctionName>` function with the "When...it should..." naming for each case.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth to also mention the sub-test feature golang has here.
For reference: https://go.dev/blog/subtests#table-driven-tests-using-subtests


### Unit test placement

- **Always include unit tests** when creating new functions or modifying existing ones
- **Place unit tests next to the code they test** — in the same package directory, in a `_test.go` file
- **Do NOT place new tests in `test/integration/`** — this is a legacy directory. New unit tests go alongside the code under test. New integration-style tests that require a cluster should use `test/e2e/` or `test/envtest/`
Comment on lines +59 to +63

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Check for references to test/integration/ in documentation and guidance files

# Find documentation files that mention test/integration/
rg -n -C3 'test/integration' --type=md --glob '!.claude/**'

# Also check if test/integration/ directory exists and contains active tests
if [ -d "test/integration" ]; then
  echo "=== test/integration directory exists ==="
  fd -e go . test/integration | head -20
fi

Repository: openshift/hypershift

Length of output: 9461


🏁 Script executed:

#!/bin/bash
# Show the relevant lines in the skill file for exact wording/context
sed -n '45,90p' .claude/skills/code-formatting/SKILL.md | nl -ba | sed -n '1,120p'

Repository: openshift/hypershift

Length of output: 105


🏁 Script executed:

#!/bin/bash
# Show the relevant lines in the skill file for exact wording/context
sed -n '45,90p' .claude/skills/code-formatting/SKILL.md | nl -ba | sed -n '1,120p'

Repository: openshift/hypershift

Length of output: 105


🏁 Script executed:

#!/bin/bash
# Show the relevant lines in the skill file for exact wording/context
sed -n '45,90p' .claude/skills/code-formatting/SKILL.md | nl -ba | sed -n '1,120p'

Repository: openshift/hypershift

Length of output: 105


Resolve conflicting test/integration/ guidance for where to add integration coverage

.claude/skills/code-formatting/SKILL.md says test/integration/ is legacy and new integration-style tests should use test/e2e/ or test/envtest/, but several docs still direct contributors to test/integration/ (e.g., docs/content/getting-started/onboarding/development.md and .../reference.md, plus dr-cli.md/gcp-platform.md using go test .../test/integration/...). With test/integration/ containing active Go tests, this will create contradictory placement/run instructions—update/align the docs (or clarify the intended unit vs integration vs e2e split) to remove ambiguity.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.claude/skills/code-formatting/SKILL.md around lines 59 - 63, Docs conflict:
some guides point to legacy test/integration/ while SKILL.md declares it
deprecated. Update all docs and scripts to consistently treat test/integration/
as deprecated and direct integration-style tests to test/e2e/ or test/envtest/
(or explicitly document the unit vs integration vs e2e split). Concretely, edit
.claude/skills/code-formatting/SKILL.md to state the canonical locations, and
update docs/content/getting-started/onboarding/development.md,
docs/.../reference.md, dr-cli.md, gcp-platform.md and any CI/test invocation
lines (e.g., any go test .../test/integration/... commands) to point to
test/e2e/ or test/envtest/ (or add a clear note if legacy tests remain); ensure
the README/CONTRIBUTING includes the final placement policy.


## Quick Checklist

Expand Down