OCPBUGS-109657: Assert errors in TestGetPrimaryPoolForNode by amogh27-web · Pull Request #6482 · openshift/machine-config-operator

amogh27-web · 2026-09-02T09:52:43Z

Fixes: https://redhat.atlassian.net/browse/OCPBUGS-109657

- What I did

TestGetPrimaryPoolForNode only failed on unexpected errors. Cases with err: true could still pass when getPrimaryPoolForNode returned (nil, nil), so they did not protect the ambiguous-pool error path.

The test loop now asserts an error when test.err is true and no error otherwise. The duplicate-selector fixture used node-role: master, which does not match node-role/master, so it never hit that error path. The node labels now match the shared selector so both custom pools apply.

- How to verify it

go test ./pkg/controller/node/ -run TestGetPrimaryPoolForNode

- Description for the changelog
Assert errors in TestGetPrimaryPoolForNode so ambiguous-pool cases cannot pass on a nil error.

Made with Cursor

Summary by CodeRabbit

Tests
- Improved coverage for node pool selection when multiple custom pools share the master selector.
- Strengthened test assertions to verify both expected errors and successful outcomes.

Cases with err: true could pass when getPrimaryPoolForNode returned (nil, nil). Assert the error path and fix the duplicate-selector fixture so the node matches both custom pools. Co-authored-by: Cursor <cursoragent@cursor.com>

openshift-merge-bot · 2026-09-02T09:52:48Z

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

openshift-ci-robot · 2026-09-02T09:52:50Z

@amogh27-web: This pull request references Jira Issue OCPBUGS-109657, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (5.1.0) matches configured target version for branch (5.1.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

Fixes: https://redhat.atlassian.net/browse/OCPBUGS-109657

- What I did

TestGetPrimaryPoolForNode only failed on unexpected errors. Cases with err: true could still pass when getPrimaryPoolForNode returned (nil, nil), so they did not protect the ambiguous-pool error path.

The test loop now asserts an error when test.err is true and no error otherwise. The duplicate-selector fixture used node-role: master, which does not match node-role/master, so it never hit that error path. The node labels now match the shared selector so both custom pools apply.

- How to verify it
go test ./pkg/controller/node/ -run TestGetPrimaryPoolForNode
- Description for the changelog
Assert errors in TestGetPrimaryPoolForNode so ambiguous-pool cases cannot pass on a nil error.

Made with Cursor

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 · 2026-09-02T09:53:01Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

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

Review profile: CHILL

Plan: Team

Run ID: 968cc32f-390d-4594-8458-5f56230a6ba9

📥 Commits

Reviewing files that changed from the base of the PR and between 8f1ab28 and b8eee7e.

📒 Files selected for processing (1)

pkg/controller/node/node_controller_test.go

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.

Walkthrough

The node primary pool test now applies the correct master label format and validates both expected error and no-error outcomes.

Changes

Node primary pool test

Layer / File(s)	Summary
Test case and error assertions `pkg/controller/node/node_controller_test.go`	The shared master selector case uses `node-role/master` with an empty value. The test loop now asserts errors and successful results according to each test case.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Merge Risk: ⚪ Minimal · up to b8eee

This localized test-only change strengthens coverage for ambiguous pool errors without changing production behavior; no actionable merge-blocking risk remains.

Suggested reviewers: isabella-janssen

🚥 Pre-merge checks | ✅ 13 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Test Structure And Quality	⚠️ Warning	The PR introduces `assert.Error(t, err)` and `assert.NoError(t, err)` without failure messages in `TestGetPrimaryPoolForNode` (lines 466 and 468). These assertions do not identify the expected `getPri…	Add contextual messages to both new assertions, for example: `assert.Error(t, err, "expected getPrimaryPoolForNode to return an error for test case %d", idx)` and `assert.NoError(t, err, "expected getPrimaryPoolForNode to return no error fo…

✅ Passed checks (13 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: strengthening error assertions in TestGetPrimaryPoolForNode.
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	No failure condition is introduced. The PR changes `TestGetPrimaryPoolForNode`, a standard Go `testing` test, and adds no `It`, `Describe`, `Context`, or `When` title. Its existing subtest name `case#…
Microshift Test Compatibility	✅ Passed	PASS: The pull request changes only the Go unit test `pkg/controller/node/node_controller_test.go`. The diff adds no Ginkgo e2e tests (`It`, `Describe`, `Context`, or `When`) and introduces no MicroSh…
Single Node Openshift (Sno) Test Compatibility	✅ Passed	PASS: The pull request changes an existing Go unit test, `TestGetPrimaryPoolForNode`, in `pkg/controller/node/node_controller_test.go`. The test uses `testing.T` and fake clients, not Ginkgo e2e const…
Topology-Aware Scheduling Compatibility	✅ Passed	PASS: The PR changes only `pkg/controller/node/node_controller_test.go`. The diff updates test labels, comments, and error assertions. It does not modify deployment manifests, operator code, controlle…
Ote Binary Stdout Contract	✅ Passed	PASS. The pull request changes only `pkg/controller/node/node_controller_test.go`. The changes add a comment, correct test labels, and replace error checks with `assert.Error`/`assert.NoError`. No pro…
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	PASS: The pull request changes only `pkg/controller/node/node_controller_test.go`. The modified test is a standard Go `testing.T` unit test, not a new Ginkgo e2e test. It uses fake Kubernetes objects …
No-Weak-Crypto	✅ Passed	The pull request changes only `pkg/controller/node/node_controller_test.go`. The diff updates node labels, adds a test comment, and strengthens error assertions. It introduces no MD5, SHA1, DES, RC4, …
Container-Privileges	✅ Passed	PASS. The pull request changes only `pkg/controller/node/node_controller_test.go`. The diff updates test labels, comments, and error assertions. It introduces no container or Kubernetes manifest and n…
No-Sensitive-Data-In-Logs	✅ Passed	The PR changes only `pkg/controller/node/node_controller_test.go`. The added lines are a comment, static test labels, and `assert.Error`/`assert.NoError` checks. No logging call or sensitive value was…

Full details: Stable And Deterministic Test Names

Explanation

No failure condition is introduced. The PR changes TestGetPrimaryPoolForNode, a standard Go testing test, and adds no It, Describe, Context, or When title. Its existing subtest name case#%d remains unchanged and is deterministic from the fixed table index. The changed pool names and node labels are test-body data, not test titles.

Full details: Test Structure And Quality

Explanation

The PR introduces assert.Error(t, err) and assert.NoError(t, err) without failure messages in TestGetPrimaryPoolForNode (lines 466 and 468). These assertions do not identify the expected getPrimaryPoolForNode behavior or the table case, which violates the assertion-message requirement. The fixture uses fake clients and does not create persistent cluster resources. The test has no Eventually or Consistently calls, and the table covers one behavior.

Resolution

Add contextual messages to both new assertions, for example: assert.Error(t, err, "expected getPrimaryPoolForNode to return an error for test case %d", idx) and assert.NoError(t, err, "expected getPrimaryPoolForNode to return no error for test case %d", idx).

Full details: Microshift Test Compatibility

Explanation

PASS: The pull request changes only the Go unit test pkg/controller/node/node_controller_test.go. The diff adds no Ginkgo e2e tests (It, Describe, Context, or When) and introduces no MicroShift compatibility concern under this check.

Full details: Single Node Openshift (Sno) Test Compatibility

Explanation

PASS: The pull request changes an existing Go unit test, TestGetPrimaryPoolForNode, in pkg/controller/node/node_controller_test.go. The test uses testing.T and fake clients, not Ginkgo e2e constructs such as It, Describe, Context, or When. It introduces no SNO-relevant multi-node or HA assumption.

Full details: Topology-Aware Scheduling Compatibility

Explanation

PASS: The PR changes only pkg/controller/node/node_controller_test.go. The diff updates test labels, comments, and error assertions. It does not modify deployment manifests, operator code, controllers, replicas, affinities, topology spread constraints, node selectors, tolerations, or PDBs. Therefore, this topology-aware scheduling check is not applicable.

Full details: Ote Binary Stdout Contract

Explanation

PASS. The pull request changes only pkg/controller/node/node_controller_test.go. The changes add a comment, correct test labels, and replace error checks with assert.Error/assert.NoError. No process-level stdout writes or suite setup changes were introduced.

Full details: Ipv6 And Disconnected Network Test Compatibility

Explanation

PASS: The pull request changes only pkg/controller/node/node_controller_test.go. The modified test is a standard Go testing.T unit test, not a new Ginkgo e2e test. It uses fake Kubernetes objects and adds no IPv4 networking assumptions or external connectivity requirements.

Full details: No-Weak-Crypto

Explanation

The pull request changes only pkg/controller/node/node_controller_test.go. The diff updates node labels, adds a test comment, and strengthens error assertions. It introduces no MD5, SHA1, DES, RC4, 3DES, Blowfish, ECB usage, custom crypto, or non-constant-time secret comparison.

Full details: Container-Privileges

Explanation

PASS. The pull request changes only pkg/controller/node/node_controller_test.go. The diff updates test labels, comments, and error assertions. It introduces no container or Kubernetes manifest and no privileged, hostPID, hostNetwork, hostIPC, SYS_ADMIN, or allowPrivilegeEscalation setting.

Full details: No-Sensitive-Data-In-Logs

Explanation

The PR changes only pkg/controller/node/node_controller_test.go. The added lines are a comment, static test labels, and assert.Error/assert.NoError checks. No logging call or sensitive value was added. The test directly calls getPrimaryPoolForNode; it does not add a runtime logging path.

Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

_{Comment @coderabbitai help to get the list of available commands.}

openshift-ci · 2026-09-02T09:53:50Z

Hi @amogh27-web. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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 kubernetes-sigs/prow repository.

openshift-ci-robot · 2026-09-02T09:55:08Z

@amogh27-web: This pull request references Jira Issue OCPBUGS-109657, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (5.1.0) matches configured target version for branch (5.1.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Details

In response to this:

Fixes: https://redhat.atlassian.net/browse/OCPBUGS-109657

- What I did

TestGetPrimaryPoolForNode only failed on unexpected errors. Cases with err: true could still pass when getPrimaryPoolForNode returned (nil, nil), so they did not protect the ambiguous-pool error path.

The test loop now asserts an error when test.err is true and no error otherwise. The duplicate-selector fixture used node-role: master, which does not match node-role/master, so it never hit that error path. The node labels now match the shared selector so both custom pools apply.

- How to verify it
go test ./pkg/controller/node/ -run TestGetPrimaryPoolForNode
- Description for the changelog
Assert errors in TestGetPrimaryPoolForNode so ambiguous-pool cases cannot pass on a nil error.

Made with Cursor

Summary by CodeRabbit

Tests

Improved coverage for node pool selection when multiple custom pools share the master selector.

Strengthened test assertions to verify both expected errors and successful outcomes.

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.

isabella-janssen · 2026-09-02T14:04:35Z

/ok-to-test

isabella-janssen · 2026-09-02T18:55:06Z

/lgtm

openshift-merge-bot · 2026-09-02T18:55:32Z

Scheduling tests matching the pipeline_run_if_changed or not excluded by pipeline_skip_if_only_changed parameters:
/test e2e-aws-ovn
/test e2e-aws-ovn-upgrade
/test e2e-gcp-op-ocl-part1
/test e2e-gcp-op-ocl-part2
/test e2e-gcp-op-part1
/test e2e-gcp-op-part2
/test e2e-gcp-op-single-node
/test e2e-hypershift
/test tls-pqc-readiness

openshift-ci · 2026-09-02T18:55:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amogh27-web, isabella-janssen

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:

~~OWNERS~~ [isabella-janssen]

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

amogh27-web · 2026-09-03T07:40:04Z

/retest-required

isabella-janssen · 2026-09-03T12:38:45Z

/retest-required

isabella-janssen · 2026-09-03T18:53:19Z

/verified by CI

Since this is a test fix, there is not further verification needed.

openshift-ci-robot · 2026-09-03T18:53:31Z

@isabella-janssen: This PR has been marked as verified by CI.

Details

In response to this:

/verified by CI

Since this is a test fix, there is not further verification needed.

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.

isabella-janssen · 2026-09-03T19:45:44Z

/retest-required

openshift-merge-bot · 2026-09-03T21:12:14Z

/retest-required

Remaining retests: 0 against base HEAD 11fd0ff and 2 for PR HEAD b8eee7e in total

openshift-ci · 2026-09-03T21:37:59Z

@amogh27-web: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/perfscale-control-plane-6nodes	`b8eee7e`	link	false	`/test perfscale-control-plane-6nodes`

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

openshift-merge-bot · 2026-09-04T09:11:12Z

/retest-required

Remaining retests: 0 against base HEAD 91960cf and 1 for PR HEAD b8eee7e in total

openshift-ci-robot · 2026-09-04T10:13:44Z

@amogh27-web: Jira Issue Verification Checks: Jira Issue OCPBUGS-109657
✔️ This pull request was pre-merge verified.
✔️ All associated pull requests have merged.
✔️ All associated, merged pull requests were pre-merge verified.

Jira Issue OCPBUGS-109657 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓

Details

In response to this:

Fixes: https://redhat.atlassian.net/browse/OCPBUGS-109657

- What I did

TestGetPrimaryPoolForNode only failed on unexpected errors. Cases with err: true could still pass when getPrimaryPoolForNode returned (nil, nil), so they did not protect the ambiguous-pool error path.

The test loop now asserts an error when test.err is true and no error otherwise. The duplicate-selector fixture used node-role: master, which does not match node-role/master, so it never hit that error path. The node labels now match the shared selector so both custom pools apply.

- How to verify it
go test ./pkg/controller/node/ -run TestGetPrimaryPoolForNode
- Description for the changelog
Assert errors in TestGetPrimaryPoolForNode so ambiguous-pool cases cannot pass on a nil error.

Made with Cursor

Summary by CodeRabbit

Tests

Improved coverage for node pool selection when multiple custom pools share the master selector.

Strengthened test assertions to verify both expected errors and successful outcomes.

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.

openshift-merge-robot · 2026-09-05T02:42:07Z

Fix included in release 5.1.0-0.nightly-2026-09-04-193828

OCPBUGS-109657: Assert errors in TestGetPrimaryPoolForNode

b8eee7e

Cases with err: true could pass when getPrimaryPoolForNode returned (nil, nil). Assert the error path and fix the duplicate-selector fixture so the node matches both custom pools. Co-authored-by: Cursor <cursoragent@cursor.com>

openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. labels Sep 2, 2026

openshift-ci Bot requested review from dkhater-redhat and proietfb September 2, 2026 09:53

openshift-ci Bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 2, 2026

openshift-ci Bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 2, 2026

openshift-ci Bot assigned isabella-janssen Sep 2, 2026

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Sep 2, 2026

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 2, 2026

openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Sep 3, 2026

openshift-merge-bot Bot merged commit 25c2546 into openshift:main Sep 4, 2026
18 of 19 checks passed

Conversation

amogh27-web commented Sep 2, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

openshift-merge-bot Bot commented Sep 2, 2026

Uh oh!

openshift-ci-robot commented Sep 2, 2026

Uh oh!

coderabbitai Bot commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (2 warnings)

Uh oh!

openshift-ci Bot commented Sep 2, 2026

Uh oh!

openshift-ci-robot commented Sep 2, 2026

Summary by CodeRabbit

Uh oh!

isabella-janssen commented Sep 2, 2026

Uh oh!

isabella-janssen commented Sep 2, 2026

Uh oh!

openshift-merge-bot Bot commented Sep 2, 2026

Uh oh!

openshift-ci Bot commented Sep 2, 2026

Uh oh!

amogh27-web commented Sep 3, 2026

Uh oh!

isabella-janssen commented Sep 3, 2026

Uh oh!

isabella-janssen commented Sep 3, 2026

Uh oh!

openshift-ci-robot commented Sep 3, 2026

Uh oh!

isabella-janssen commented Sep 3, 2026

Uh oh!

openshift-merge-bot Bot commented Sep 3, 2026

Uh oh!

openshift-ci Bot commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-merge-bot Bot commented Sep 4, 2026

Uh oh!

Uh oh!

openshift-ci-robot commented Sep 4, 2026

Summary by CodeRabbit

Uh oh!

openshift-merge-robot commented Sep 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

amogh27-web commented Sep 2, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Sep 2, 2026 •

edited

Loading

openshift-ci Bot commented Sep 3, 2026 •

edited

Loading