[wip][poc] Add skills and test documentation models#1994
Open
anahas-redhat wants to merge 1 commit into
Open
Conversation
Add three Claude Code skills: - create-release-plan: generates release testing plans (full/light formats) from repo state, with OCP upgrade matrix, CI job derivation, DAST API list, and test case traceability - create-test-case: generates manual test case docs from Ginkgo e2e test files, mapping By() steps to action/verification with oc commands - create-test-code: generates e2e test code from manual test case documentation Add documentation models used by the skills: - Release test plans for Kueue 1.4 and 1.5 (full and light formats) serving as templates for future releases - Test case docs for admission fair sharing, TLS profile, and visibility on demand serving as reference examples Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: anahas-redhat The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughThis PR adds comprehensive release testing documentation and framework for the Kueue operator. It establishes command specifications for automating test artifact generation, documents four feature test suites (admission fair sharing, DRA Extended Resources, TLS profiles, visibility API) across 19 test scenarios, provides release testing plans for Kueue 1.4 and 1.5 with CI job mappings and upgrade matrices, and includes a working Ginkgo E2E test implementation for DRA behavior validation. ChangesKueue Release Testing Documentation and E2E Test Suite
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
.claude/commands/create-test-case.md (1)
9-13: 💤 Low valueConsider adding a language identifier to the fenced code block.
The usage examples block would be clearer with a language identifier (e.g.,
bashorplaintext).📝 Suggested improvement
## Usage -``` +```plaintext /create-test-case test/e2e/e2e_dra_test.go /create-test-case test/e2e/e2e_preemption_test.go /create-test-case🤖 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/commands/create-test-case.md around lines 9 - 13, The fenced code block showing usage examples in .claude/commands/create-test-case.md lacks a language identifier; update the triple-backtick fence to include a language like plaintext or bash so syntax/highlighting and accessibility improve (locate the block containing "/create-test-case test/e2e/e2e_dra_test.go" and change the opening "```" to "```plaintext" or "```bash")..claude/commands/create-test-code.md (1)
8-12: 💤 Low valueConsider adding a language identifier to the fenced code block.
The usage examples block would benefit from a language identifier for consistency with other command specifications.
📝 Suggested improvement
## Usage -``` +```plaintext /create-test-code test/docs/cases/e2e_dra.md /create-test-code test/docs/cases/e2e_preemption.md /create-test-code🤖 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/commands/create-test-code.md around lines 8 - 12, The fenced code block showing the usage examples for the /create-test-code command lacks a language identifier; update the block to include a language tag (e.g., plaintext) so it matches other command specifications — locate the fenced block containing the three lines that start with "/create-test-code" and change the opening ``` to ```plaintext to provide consistent syntax highlighting and styling..claude/commands/create-release-plan.md (1)
9-13: ⚡ Quick winAdd language identifiers to fenced code blocks for better rendering.
Multiple fenced code blocks throughout this file lack language identifiers, which affects syntax highlighting and accessibility.
📝 Suggested improvements
## Usage -``` +```plaintext /create-release-plan 1.5 /create-release-plan 1.5 full /create-release-plan 1.5 lightAnd similarly for the code blocks at lines 55, 63, 78, and 86 - they should use
plaintextortextas the language identifier.🤖 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/commands/create-release-plan.md around lines 9 - 13, Add a language identifier (e.g., plaintext) to the fenced code blocks that currently contain the command examples so they render with proper syntax highlighting; specifically update the blocks containing the lines "/create-release-plan 1.5", "/create-release-plan 1.5 full", and "/create-release-plan 1.5 light" (and the other similar blocks at the later occurrences around the snippets showing those commands) to use ```plaintext instead of plain ``` so all code fences include a language tag.test/docs/plans/kueue-1.4-light.md (1)
50-50: 💤 Low valueMinor naming inconsistency across plans.
The test case name varies between documents: "Managed Jobs NS Selector" (1.4-light, 1.5-light) vs "Managed Jobs Namespace Selector" (1.5-full). Consider standardizing to the full form for clarity.
🤖 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 `@test/docs/plans/kueue-1.4-light.md` at line 50, Rename the plan entry title "Managed Jobs NS Selector" to "Managed Jobs Namespace Selector" to match the naming used elsewhere; update the string in the document containing the table row that references the test file e2e_managed_jobs_namespace_selector_test.go so the label exactly reads "Managed Jobs Namespace Selector" (this ensures consistency across the plans 1.4-light, 1.5-light, and 1.5-full).test/e2e/e2e_dra_er_alpha2_test.go (3)
188-196: 💤 Low valueConsider extracting job creation into a helper function.
Following the guideline pattern from
e2e_dra_test.go, consider creating a small helper function in this file that calls the builder and layers on the DRA extended resource fields. This reduces duplication between the two test scenarios and improves readability.♻️ Example helper function
func newDRAExtendedResourceJob(builder *testutils.TestResourceBuilder, name, queueName string) *batchv1.Job { job := builder.NewJob() setDRAJobCPU(job) job.Name = name job.Labels[testutils.QueueLabel] = queueName job.Spec.Template.Spec.Containers[0].Resources.Requests[corev1.ResourceName(extendedResourceName)] = resource.MustParse("1") job.Spec.Template.Spec.Containers[0].Resources.Limits = corev1.ResourceList{ corev1.ResourceName(extendedResourceName): resource.MustParse("1"), } return job }Then use it at the call sites:
builder := testutils.NewTestResourceBuilder(ns.Name, erAlpha2QueueName) job := newDRAExtendedResourceJob(builder, "s1-no-deviceclass", erAlpha2QueueName)🤖 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 `@test/e2e/e2e_dra_er_alpha2_test.go` around lines 188 - 196, Extract the repeated job construction into a helper like newDRAExtendedResourceJob that accepts the testutils.TestResourceBuilder (from testutils.NewTestResourceBuilder), name and queueName, calls builder.NewJob(), invokes setDRAJobCPU(job), sets job.Name, job.Labels[testutils.QueueLabel], and applies the extended resource requests/limits using corev1.ResourceName(extendedResourceName) and resource.MustParse("1"); replace the inline construction in the test with a call to this helper to reduce duplication and improve readability.Source: Coding guidelines
310-318: 💤 Low valueInconsistent timeout for Consistently check.
Line 318 uses
testutils.ConsistentlyTimeout, while the similar check in the first scenario (line 225) usestestutils.ConsistentlyLongTimeout. If the shorter timeout is intentional (waiting less before DeviceClass creation), consider adding a comment to explain the difference. Otherwise, use the same timeout for consistency.🤖 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 `@test/e2e/e2e_dra_er_alpha2_test.go` around lines 310 - 318, The Consistently check using testutils.ConsistentlyTimeout in the Consistently(func(g Gomega) { ... }) call is inconsistent with the earlier scenario which uses testutils.ConsistentlyLongTimeout; either replace testutils.ConsistentlyTimeout with testutils.ConsistentlyLongTimeout to match the first scenario (so the Consistently invocation that checks pods for createdJob uses the longer timeout), or if the shorter timeout is intentional add a brief comment immediately above the Consistently call explaining why this case needs a shorter timeout (referencing the Consistently invocation and testutils.ConsistentlyTimeout/testutils.ConsistentlyLongTimeout to locate the change).
121-135: ⚡ Quick winConsider logging errors during DeviceClass restoration.
Lines 128 and 132 ignore errors during DeviceClass recreation/update. While cleanup failures can be tolerated, logging them helps with debugging when tests behave unexpectedly.
♻️ Proposed enhancement to log cleanup errors
if err != nil { recreated := originalDeviceClass.DeepCopy() recreated.ResourceVersion = "" - _, _ = kubeClient.ResourceV1().DeviceClasses().Create(ctx, recreated, metav1.CreateOptions{}) + if _, err := kubeClient.ResourceV1().DeviceClasses().Create(ctx, recreated, metav1.CreateOptions{}); err != nil { + GinkgoWriter.Printf("Warning: Failed to recreate DeviceClass during cleanup: %v\n", err) + } } else { dc.Spec.ExtendedResourceName = originalDeviceClass.Spec.ExtendedResourceName dc.Spec.Selectors = originalDeviceClass.Spec.Selectors - _, _ = kubeClient.ResourceV1().DeviceClasses().Update(ctx, dc, metav1.UpdateOptions{}) + if _, err := kubeClient.ResourceV1().DeviceClasses().Update(ctx, dc, metav1.UpdateOptions{}); err != nil { + GinkgoWriter.Printf("Warning: Failed to update DeviceClass during cleanup: %v\n", err) + } }🤖 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 `@test/e2e/e2e_dra_er_alpha2_test.go` around lines 121 - 135, The cleanup block in AfterAll that restores originalDeviceClass currently ignores errors from DeviceClasses.Create and DeviceClasses.Update; update the AfterAll closure to capture the returned errors from kubeClient.ResourceV1().DeviceClasses().Create(...) and .Update(...), and log them (including context like draDeviceClassName and whether it was a recreate vs update) using the test logging mechanism in this file (e.g., Ginkgo/Gomega logging or the existing test logger) so failures during restoration are recorded; reference the existing symbols originalDeviceClass, draDeviceClassName, kubeClient.ResourceV1().DeviceClasses().Get/Create/Update, and the local variables dc/err when adding the logging.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@test/docs/cases/e2e_admission_fair_sharing.md`:
- Around line 330-339: Update the cleanup step in the
e2e_admission_fair_sharing.md doc to clarify the Kueue config restore: replace
the literal filename reference `kueue-config-backup.yaml` with a clear
placeholder or restoration command and mention the saved-config variable used in
the Setup, e.g., use `<kueue-config-backup.yaml>` or a restoration instruction
like patching the cluster Kueue config with the saved config (refer to
"saved-config" from the Setup step) so readers know whether to apply a file or
reapply the saved config; adjust the final line accordingly in the markdown.
In `@test/e2e/e2e_dra_er_alpha2_test.go`:
- Around line 168-174: The namespace is being created directly with
kubeClient.CoreV1().Namespaces().Create(...) instead of using the shared helper;
replace the direct creation with testutils.CreateNamespace by constructing the
corev1.Namespace (with GenerateName erAlpha2NamespacePrefix and Labels using
testutils.OpenShiftManagedLabel), call cleanup, err :=
testutils.CreateNamespace(kubeClient, ns), assert
Expect(err).NotTo(HaveOccurred(), "Failed to create namespace"), and register
cleanup with DeferCleanup(cleanup) so the test uses consistent setup/teardown
patterns.
---
Nitpick comments:
In @.claude/commands/create-release-plan.md:
- Around line 9-13: Add a language identifier (e.g., plaintext) to the fenced
code blocks that currently contain the command examples so they render with
proper syntax highlighting; specifically update the blocks containing the lines
"/create-release-plan 1.5", "/create-release-plan 1.5 full", and
"/create-release-plan 1.5 light" (and the other similar blocks at the later
occurrences around the snippets showing those commands) to use ```plaintext
instead of plain ``` so all code fences include a language tag.
In @.claude/commands/create-test-case.md:
- Around line 9-13: The fenced code block showing usage examples in
.claude/commands/create-test-case.md lacks a language identifier; update the
triple-backtick fence to include a language like plaintext or bash so
syntax/highlighting and accessibility improve (locate the block containing
"/create-test-case test/e2e/e2e_dra_test.go" and change the opening "```" to
"```plaintext" or "```bash").
In @.claude/commands/create-test-code.md:
- Around line 8-12: The fenced code block showing the usage examples for the
/create-test-code command lacks a language identifier; update the block to
include a language tag (e.g., plaintext) so it matches other command
specifications — locate the fenced block containing the three lines that start
with "/create-test-code" and change the opening ``` to ```plaintext to provide
consistent syntax highlighting and styling.
In `@test/docs/plans/kueue-1.4-light.md`:
- Line 50: Rename the plan entry title "Managed Jobs NS Selector" to "Managed
Jobs Namespace Selector" to match the naming used elsewhere; update the string
in the document containing the table row that references the test file
e2e_managed_jobs_namespace_selector_test.go so the label exactly reads "Managed
Jobs Namespace Selector" (this ensures consistency across the plans 1.4-light,
1.5-light, and 1.5-full).
In `@test/e2e/e2e_dra_er_alpha2_test.go`:
- Around line 188-196: Extract the repeated job construction into a helper like
newDRAExtendedResourceJob that accepts the testutils.TestResourceBuilder (from
testutils.NewTestResourceBuilder), name and queueName, calls builder.NewJob(),
invokes setDRAJobCPU(job), sets job.Name, job.Labels[testutils.QueueLabel], and
applies the extended resource requests/limits using
corev1.ResourceName(extendedResourceName) and resource.MustParse("1"); replace
the inline construction in the test with a call to this helper to reduce
duplication and improve readability.
- Around line 310-318: The Consistently check using
testutils.ConsistentlyTimeout in the Consistently(func(g Gomega) { ... }) call
is inconsistent with the earlier scenario which uses
testutils.ConsistentlyLongTimeout; either replace testutils.ConsistentlyTimeout
with testutils.ConsistentlyLongTimeout to match the first scenario (so the
Consistently invocation that checks pods for createdJob uses the longer
timeout), or if the shorter timeout is intentional add a brief comment
immediately above the Consistently call explaining why this case needs a shorter
timeout (referencing the Consistently invocation and
testutils.ConsistentlyTimeout/testutils.ConsistentlyLongTimeout to locate the
change).
- Around line 121-135: The cleanup block in AfterAll that restores
originalDeviceClass currently ignores errors from DeviceClasses.Create and
DeviceClasses.Update; update the AfterAll closure to capture the returned errors
from kubeClient.ResourceV1().DeviceClasses().Create(...) and .Update(...), and
log them (including context like draDeviceClassName and whether it was a
recreate vs update) using the test logging mechanism in this file (e.g.,
Ginkgo/Gomega logging or the existing test logger) so failures during
restoration are recorded; reference the existing symbols originalDeviceClass,
draDeviceClassName, kubeClient.ResourceV1().DeviceClasses().Get/Create/Update,
and the local variables dc/err when adding the logging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 9ed91a3e-2854-4322-98b6-408199e36291
📒 Files selected for processing (12)
.claude/commands/create-release-plan.md.claude/commands/create-test-case.md.claude/commands/create-test-code.mdtest/docs/cases/e2e_admission_fair_sharing.mdtest/docs/cases/e2e_dra_er_alpha2.mdtest/docs/cases/e2e_tls_profile.mdtest/docs/cases/e2e_visibility_on_demand.mdtest/docs/plans/kueue-1.4-full.mdtest/docs/plans/kueue-1.4-light.mdtest/docs/plans/kueue-1.5-full.mdtest/docs/plans/kueue-1.5-light.mdtest/e2e/e2e_dra_er_alpha2_test.go
Comment on lines
+330
to
+339
| **Cleanup:** | ||
|
|
||
| ```bash | ||
| oc delete jobs --all -n <namespace> | ||
| oc delete localqueue lq-cpu-heavy lq-cpu-light -n <namespace> | ||
| oc delete clusterqueue <cq-name> | ||
| oc delete resourceflavor <rf-name> | ||
| oc delete namespace <namespace> | ||
| oc apply -f kueue-config-backup.yaml | ||
| ``` |
There was a problem hiding this comment.
Clarify or correct the cleanup file reference.
Line 338 references kueue-config-backup.yaml, which suggests a specific file must exist. Since this is a manual test case document, this should either:
- Be a placeholder like
<kueue-config-backup.yaml> - Show the actual command to restore from the saved config variable
- Reference the pattern used in automated tests
💡 Suggested clarification
**Cleanup:**
```bash
oc delete jobs --all -n <namespace>
oc delete localqueue lq-cpu-heavy lq-cpu-light -n <namespace>
oc delete clusterqueue <cq-name>
oc delete resourceflavor <rf-name>
oc delete namespace <namespace>
-oc apply -f kueue-config-backup.yaml
+# Restore the saved Kueue config (saved in Setup step 1)
+oc patch kueue cluster --type=merge -p '{"spec":{"config":<saved-config>}}'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| **Cleanup:** | |
| ```bash | |
| oc delete jobs --all -n <namespace> | |
| oc delete localqueue lq-cpu-heavy lq-cpu-light -n <namespace> | |
| oc delete clusterqueue <cq-name> | |
| oc delete resourceflavor <rf-name> | |
| oc delete namespace <namespace> | |
| oc apply -f kueue-config-backup.yaml | |
| ``` | |
| **Cleanup:** | |
🤖 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 `@test/docs/cases/e2e_admission_fair_sharing.md` around lines 330 - 339, Update
the cleanup step in the e2e_admission_fair_sharing.md doc to clarify the Kueue
config restore: replace the literal filename reference
`kueue-config-backup.yaml` with a clear placeholder or restoration command and
mention the saved-config variable used in the Setup, e.g., use
`<kueue-config-backup.yaml>` or a restoration instruction like patching the
cluster Kueue config with the saved config (refer to "saved-config" from the
Setup step) so readers know whether to apply a file or reapply the saved config;
adjust the final line accordingly in the markdown.
Comment on lines
+168
to
+174
| ns, err := kubeClient.CoreV1().Namespaces().Create(ctx, &corev1.Namespace{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| GenerateName: erAlpha2NamespacePrefix, | ||
| Labels: map[string]string{testutils.OpenShiftManagedLabel: "true"}, | ||
| }, | ||
| }, metav1.CreateOptions{}) | ||
| Expect(err).NotTo(HaveOccurred(), "Failed to create namespace") |
There was a problem hiding this comment.
Use testutils.CreateNamespace for all namespace creation in e2e tests.
Both test scenarios create namespaces directly instead of using the recommended testutils.CreateNamespace(kubeClient, ns) utility function. As per coding guidelines, all e2e tests should use the testutils helper for namespace creation to ensure consistent setup and cleanup patterns across the test suite.
🔧 Proposed fix pattern
Replace direct namespace creation:
ns, err := kubeClient.CoreV1().Namespaces().Create(ctx, &corev1.Namespace{...}, metav1.CreateOptions{})
// ... manual cleanup with DeferCleanupWith testutils helper:
ns := &corev1.Namespace{
ObjectMeta: metav1.ObjectMeta{
GenerateName: erAlpha2NamespacePrefix,
Labels: map[string]string{testutils.OpenShiftManagedLabel: "true"},
},
}
cleanup, err := testutils.CreateNamespace(kubeClient, ns)
Expect(err).NotTo(HaveOccurred(), "Failed to create namespace")
DeferCleanup(cleanup)🤖 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 `@test/e2e/e2e_dra_er_alpha2_test.go` around lines 168 - 174, The namespace is
being created directly with kubeClient.CoreV1().Namespaces().Create(...) instead
of using the shared helper; replace the direct creation with
testutils.CreateNamespace by constructing the corev1.Namespace (with
GenerateName erAlpha2NamespacePrefix and Labels using
testutils.OpenShiftManagedLabel), call cleanup, err :=
testutils.CreateNamespace(kubeClient, ns), assert
Expect(err).NotTo(HaveOccurred(), "Failed to create namespace"), and register
cleanup with DeferCleanup(cleanup) so the test uses consistent setup/teardown
patterns.
Source: Coding guidelines
|
@anahas-redhat: The following tests failed, say
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
POC for QE team
Add three Claude Code skills:
Add documentation models used by the skills:
Summary by CodeRabbit
Documentation
Tests