Skip to content

Add: AGENTS.md/README.md, Migrate more OTE tests#1516

Draft
jmencak wants to merge 1 commit into
openshift:mainfrom
jmencak:5.0-OTE-more-tests
Draft

Add: AGENTS.md/README.md, Migrate more OTE tests#1516
jmencak wants to merge 1 commit into
openshift:mainfrom
jmencak:5.0-OTE-more-tests

Conversation

@jmencak
Copy link
Copy Markdown
Contributor

@jmencak jmencak commented May 14, 2026

Summary by CodeRabbit

Release Notes

  • Chores

    • Updated Cluster Node Tuning Operator test suite configuration and qualifiers for improved test selection.
    • Refactored test setup skip logic for better handling when the Node Tuning Operator is not installed.
  • Documentation

    • Added comprehensive documentation for the Node Tuning Operator QE test extension, including test organization, migration guidelines, and development workflow instructions.

@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 14, 2026
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented May 14, 2026

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented May 14, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jmencak

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jmencak
Copy link
Copy Markdown
Contributor Author

jmencak commented May 14, 2026

/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-disruptive-longrunning

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 14, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 14, 2026

Walkthrough

This PR restructures the Cluster Node Tuning Operator test extension by removing ReleaseGate qualifiers from test suite selectors, refactoring test setup to use new skip utilities, and adding comprehensive documentation for NTO QE test development. New utility functions enable early test skipping and ROSA cluster detection.

Changes

NTO Test Suite Restructuring and Documentation

Layer / File(s) Summary
NTO test utility functions
test/extended/utils/nto_util.go
New SkipNoNTO function enables early test skipping when NTO is not installed; new IsRosaCluster function detects ROSA clusters via cluster claim inspection.
NTO test spec refactoring
test/extended/specs/nto.go
BeforeEach now calls SkipNoNTO instead of managing isNTO state variable; ReleaseGate label removed from disruptive test; empty AfterEach cleanup hook added.
Test suite qualifier configuration
cmd/cluster-node-tuning-operator-test-ext/main.go
Disruptive and slow suite selectors no longer gate on ReleaseGate; all-inclusive suite changed from ReleaseGate-gated to unqualified; comment label updated from [sig-node-tuning] to [sig-tuning-node].
Test framework documentation and guidance
test/extended/README.md, test/extended/AGENTS.md, test/extended/CLAUDE.md
Comprehensive documentation for NTO QE test extension defines scope, test organization, suite definitions, migration guide with labeling rules, code requirements, local development workflow, and AI agent guidance for test framework interaction.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 9 | ❌ 3

❌ Failed checks (3 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Test Structure And Quality ⚠️ Warning Most assertions lack messages (19+ in nto.go/nto_util.go). Main test violates single responsibility—100 lines with 10 unrelated g.By steps instead of one focused behavior. Add assertion messages throughout tests. Split large test into separate focused tests for each behavior (isolated_cores test vs irq_smp_affinity test).
Microshift Test Compatibility ⚠️ Warning Tests reference unsupported OpenShift APIs (Infrastructure, MachineSet, Scheduler, Tuned) without any MicroShift protection mechanisms like [apigroup] tags or [Skipped:MicroShift] labels. Add [apigroup:config.openshift.io] and [apigroup:machine.openshift.io] tags to test names, or add [Skipped:MicroShift], or guard BeforeEach with g.Skip() via IsMicroShiftCluster() check.
✅ Passed checks (9 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main changes: adding AGENTS.md and README.md documentation files and migrating additional OTE (OpenShift Test Extension) tests with updated test suite configuration and utility functions.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed All Ginkgo test titles use stable, deterministic strings. No dynamic content (pod names, timestamps, UUIDs, IPs) found in any test names across all modified files.
Single Node Openshift (Sno) Test Compatibility ✅ Passed PR adds 5 tests: 4 trivial tests and 1 real test with explicit SNO handling via IsSNOCluster check and GetFirstLinuxWorkerNode. No multi-node assumptions detected.
Topology-Aware Scheduling Compatibility ✅ Passed PR only modifies test files, documentation, and test utilities. No deployment manifests, operator code, or controllers were changed. This check is not applicable to test-only changes.
Ote Binary Stdout Contract ✅ Passed No stdout writes in process-level code. All Logf calls use fmt.Printf inside test blocks where Ginkgo v2 intercepts output. JSON contract with openshift-tests is safe.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed No IPv4 assumptions or external connectivity requirements detected in new tests. Tests work with cluster-internal Kubernetes APIs, local node resources (/proc/irq), and built-in test fixtures only.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: err: exit status 1: stderr: go: inconsistent vendoring in :\n\tgithub.com/RHsyseng/operator-utils@v1.4.13: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/coreos/go-systemd@v0.0.0-20191104093116-d3cd4ed1dbcf: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/coreos/ignition@v0.35.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/coreos/ignition/v2@v2.26.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/docker/go-units@v0.5.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/go-logr/stdr@v1.2.2: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/google/go-cmp@v0.7.0

... [truncated 19340 characters] ...

is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/legacy-cloud-providers: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/metrics: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/mount-utils: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/pod-security-admission: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/sample-apiserver: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tgithub.com/onsi/ginkgo/v2: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\n\tTo ignore the vendor directory, use -mod=readonly or -mod=mod.\n\tTo sync the vendor directory, run:\n\t\tgo mod vendor\n"


Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented May 14, 2026

@jmencak: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

  • periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-disruptive-longrunning

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/0e5325b0-4fae-11f1-8b60-ba5c504ebdf0-0

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
test/extended/utils/nto_util.go (1)

505-514: 💤 Low value

Consider more idiomatic string comparison.

The function uses strings.Compare(product, "ROSA") == 0, which is correct but verbose. Direct string comparison product == "ROSA" is more idiomatic and clearer in Go.

♻️ Suggested simplification
 func IsRosaCluster(oc *CLI) bool {
 	product, _ := oc.WithoutNamespace().AsAdmin().Run("get").Args("clusterclaims/product.open-cluster-management.io", "-o=jsonpath={.spec.value}").Output()
-	return strings.Compare(product, "ROSA") == 0
+	return product == "ROSA"
 }
🤖 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/extended/utils/nto_util.go` around lines 505 - 514, The IsRosaCluster
function uses strings.Compare(product, "ROSA") == 0 which is verbose; replace
that expression with the idiomatic direct string equality check product ==
"ROSA" in the IsRosaCluster function to simplify the code and improve
readability (keep the call that obtains product from oc unchanged).
🤖 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.

Nitpick comments:
In `@test/extended/utils/nto_util.go`:
- Around line 505-514: The IsRosaCluster function uses strings.Compare(product,
"ROSA") == 0 which is verbose; replace that expression with the idiomatic direct
string equality check product == "ROSA" in the IsRosaCluster function to
simplify the code and improve readability (keep the call that obtains product
from oc unchanged).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: cecf6633-5a5e-4a8a-a1d3-5b8d68fdad23

📥 Commits

Reviewing files that changed from the base of the PR and between 54a9ef7 and 64025ef.

📒 Files selected for processing (6)
  • cmd/cluster-node-tuning-operator-test-ext/main.go
  • test/extended/AGENTS.md
  • test/extended/CLAUDE.md
  • test/extended/README.md
  • test/extended/specs/nto.go
  • test/extended/utils/nto_util.go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant