NO-JIRA: chore: add unit test naming and placement conventions - #8722
Conversation
…ing skill - Unit test functions must be named after the function under test (TestFunctionName) - Unit tests go next to the code they test, not in test/integration/ (legacy) - Integration-style tests should use test/e2e/ or test/envtest/ Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
📝 WalkthroughWalkthroughThis PR updates the Code Formatting skill documentation to expand mandatory test conventions. It adds a "Test naming" subsection requiring Go test names to follow the Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
area/ai
openshift-ci
Bot
added
approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, enxebre The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Pipeline controller notification No second-stage tests were triggered for this PR. This can happen when:
Use |
|
/verified by @enxebre |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8722 +/- ##
=======================================
Coverage 41.59% 41.59%
=======================================
Files 758 758
Lines 93925 93925
=======================================
Hits 39066 39066
Misses 52113 52113
Partials 2746 2746
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
openshift-ci-robot
added
the
verified
|
@enxebre: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.claude/skills/code-formatting/SKILL.md (1)
16-38: ⚡ Quick winClarify "test name" refers to test case names within table-driven tests.
The section uses "test name" (line 18) without explicitly stating these are test case names within table-driven tests, not test function names. While line 57 later clarifies this, it would be clearer to specify upfront to avoid confusion.
📝 Suggested clarification
### Test naming -**NON-NEGOTIABLE RULE**: Every Go test name MUST follow this exact format: +**NON-NEGOTIABLE RULE**: Every Go test case name (within table-driven tests) MUST follow this exact format: ```go name: "When <condition>, it should <expected behavior>"</details> <details> <summary>🤖 Prompt for AI Agents</summary>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 16 - 38, Clarify that
"test name" in the "Test naming" section refers specifically to the name field
of individual test cases inside table-driven tests (not the Go test function
name); update the opening sentence under the "Test naming" heading to state this
explicitly and adjust the first example line (name: "When , it should
") to indicate it is the test case name in a table-driven
test, ensuring consistency with the later clarification currently present
elsewhere in the document.</details> <!-- cr-comment:v1:b434dcae15713e6191bc849c --> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Inline comments:
In @.claude/skills/code-formatting/SKILL.md:
- Around line 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.
Nitpick comments:
In @.claude/skills/code-formatting/SKILL.md:
- Around line 16-38: Clarify that "test name" in the "Test naming" section
refers specifically to the name field of individual test cases inside
table-driven tests (not the Go test function name); update the opening sentence
under the "Test naming" heading to state this explicitly and adjust the first
example line (name: "When , it should ") to
indicate it is the test case name in a table-driven test, ensuring consistency
with the later clarification currently present elsewhere in the document.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Repository YAML (base), Central YAML (inherited) **Review profile**: CHILL **Plan**: Enterprise **Run ID**: `69366dfc-4bb9-4454-b0f2-e7ec620199de` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 0b3300632d1314f6782feb49804eddd70a11d08e and 1fe40b7af8a69455e738938f42ea9ac22fba12ee. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `.claude/skills/code-formatting/SKILL.md` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| ### 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/` |
There was a problem hiding this comment.
🧩 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
fiRepository: 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.
| - ❌ `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. |
There was a problem hiding this comment.
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
enxebre
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@enxebre: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@enxebre: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
Summary
TestFunctionName)_test.go), not intest/integration/(legacy)test/e2e/ortest/envtest/Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit