OCPBUGS-62605: e2e: refactor GetSMTLevel to remove Gomega assertions #1399

SargunNarula · 2025-09-26T12:45:14Z

The GetSMTLevel util function previously used gomega assertions which could cause panics when called outside of Ginkgo It blocks.

This PR addresses:

Modified the GetSMTLevel signature to return (int, error)
Replaced ExpectWithOffset assertions with standard error handling
Updated all function refrences to handle the error return

SargunNarula · 2025-09-26T20:44:48Z

/retest

test/e2e/performanceprofile/functests/1_performance/cpu_management.go

The GetSMTLevel util function previously used gomega assertions which could cause panics when called outside of Ginkgo It blocks. This PR addresses: - Modified the GetSMTLevel signature to return (int, error) - Replaced ExpectWithOffset assertions with standard error handling - Updated all function refrences to handle the error return Signed-Off-by: Sargun Narula <snarula@redhat.com>

swatisehgal

/lgtm

Tal-or · 2025-09-29T10:22:34Z

test/e2e/performanceprofile/functests/utils/nodes/nodes.go

 // GetSMTLevel returns the SMT level on the node using the given cpuID as target
 // Use a random cpuID from the return value of GetOnlineCPUsSet if not sure
-func GetSMTLevel(ctx context.Context, cpuID int, node *corev1.Node) int {
+func GetSMTLevel(ctx context.Context, cpuID int, node *corev1.Node) (int, error) {


GinkgoHelper() in the first line will solve exactly that without having to change the function API
https://onsi.github.io/ginkgo/#mental-model-how-ginkgo-handles-failure

@Tal-or is very correct. In addition, that would allow us to remove the fragile WithOffset expectations (mostly my fault here).

That said, changing the function signature and return an explicit error LGTM and I slightly prefer that approach for library code.

@Tal-or If i understand correctly, if we used GinkgoHelper(), it would remove the panic and point the failure to Test itself, which originally is not the motive of the API change.

Considering a library function, it should work when called outside a test spec block, where it would panic. Please let me know if I’ve misunderstood anything here.

What is considered to be panic? a go assertion? because it's expected to abort a test with an assertion in Ginkgo.
All GinkgoHelper() is doing is point to the caller of the function (in the stack trace), it's basically equal to call Fail with offset.

Considering a library function, it should work when called outside a test spec block

this function scope is only for the NTO e2e test, it should not be imported into other projects and not even in the none-test code of this specific project, so assertion here is completely fine.

Bottom line, I don't mind whether this function return an errors or asserts, my initial comment was mainly for the sake of discussion and broadening the knowledge and expertise as part of the reviewing process.

this function scope is only for the NTO e2e test, it should not be imported into other projects and not even in the none-test code of this specific project, so assertion here is completely fine.

I fully agree with this sentence

I guess the remaining open point boils down to: where is this function planned to be used?

@ffromani Here is a use- case where this function was tend to be used - Link. Ignore the checkHyperthreding() for now as it is just a wrapper over getSMTLevel and would be removed once it itself gets fixed.

SargunNarula · 2025-09-29T14:47:41Z

/retest

SargunNarula · 2025-09-29T19:56:18Z

/retest

SargunNarula · 2025-09-29T22:18:28Z

/retest

SargunNarula · 2025-09-30T05:49:57Z

/retest

SargunNarula · 2025-09-30T09:26:25Z

/retest

ffromani · 2025-09-30T09:42:22Z

considering the proposed usage, which is still within the test code, I'm inclined to approve. @Tal-or any concern or comment?

Tal-or · 2025-09-30T12:49:04Z

considering the proposed usage, which is still within the test code, I'm inclined to approve. @Tal-or any concern or comment?

@ffromani Works for me

ffromani

/approve
/lgtm

openshift-ci · 2025-09-30T12:56:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ffromani, SargunNarula

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ffromani]

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

openshift-ci-robot · 2025-09-30T13:03:19Z

@SargunNarula: This pull request references CNF-19657 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target either version "4.21." or "openshift-4.21.", but it targets "openshift-4.20" instead.

In response to this:

The GetSMTLevel util function previously used gomega assertions which could cause panics when called outside of Ginkgo It blocks.

This PR addresses:

Modified the GetSMTLevel signature to return (int, error)

Replaced ExpectWithOffset assertions with standard error handling

Updated all function refrences to handle the error return

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.

SargunNarula · 2025-09-30T14:02:37Z

/retest

SargunNarula · 2025-09-30T15:15:47Z

/label verified

openshift-ci · 2025-09-30T15:15:50Z

@SargunNarula: The label(s) /label verified cannot be applied. These labels are supported: acknowledge-critical-fixes-only, platform/aws, platform/azure, platform/baremetal, platform/google, platform/libvirt, platform/openstack, ga, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, px-approved, docs-approved, qe-approved, ux-approved, no-qe, downstream-change-needed, rebase/manual, cluster-config-api-changed, run-integration-tests, approved, backport-risk-assessed, bugzilla/valid-bug, cherry-pick-approved, jira/valid-bug, ok-to-test, stability-fix-approved, staff-eng-approved. Is this label configured under labels -> additional_labels or labels -> restricted_labels in plugin.yaml?

In response to this:

/label verified

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.

mrniranjan · 2025-10-01T03:50:02Z

/verified by @mrniranjan

openshift-ci-robot · 2025-10-01T03:50:14Z

@mrniranjan: This PR has been marked as verified by @mrniranjan.

In response to this:

/verified by @mrniranjan

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-ci-robot · 2025-10-01T04:12:52Z

/retest-required

Remaining retests: 0 against base HEAD b5d66f8 and 2 for PR HEAD 0be8ef3 in total

openshift-ci-robot · 2025-10-01T07:35:41Z

@SargunNarula: This pull request references Jira Issue OCPBUGS-62605, which is invalid:

expected the bug to target the "4.21.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

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

In response to this:

The GetSMTLevel util function previously used gomega assertions which could cause panics when called outside of Ginkgo It blocks.

This PR addresses:

Modified the GetSMTLevel signature to return (int, error)

Replaced ExpectWithOffset assertions with standard error handling

Updated all function refrences to handle the error return

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.

SargunNarula · 2025-10-01T07:37:22Z

/jira refresh

openshift-ci-robot · 2025-10-01T07:37:30Z

@SargunNarula: This pull request references Jira Issue OCPBUGS-62605, 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 (4.21.0) matches configured target version for branch (4.21.0)
bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (liqcui@redhat.com), skipping review request.

In response to this:

/jira refresh

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-ci · 2025-10-01T10:44:00Z

@SargunNarula: 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/okd-scos-e2e-aws-ovn	`0be8ef3`	link	false	`/test okd-scos-e2e-aws-ovn`

Full PR test history. Your PR dashboard.

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-ci-robot · 2025-10-01T10:47:30Z

@SargunNarula: Jira Issue Verification Checks: Jira Issue OCPBUGS-62605
✔️ 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-62605 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓

In response to this:

The GetSMTLevel util function previously used gomega assertions which could cause panics when called outside of Ginkgo It blocks.

This PR addresses:

Modified the GetSMTLevel signature to return (int, error)

Replaced ExpectWithOffset assertions with standard error handling

Updated all function refrences to handle the error return

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 · 2025-10-02T13:23:05Z

Fix included in accepted release 4.21.0-0.nightly-2025-10-02-002727

SargunNarula changed the title ~~e2e: refactor: Remove gomega assertions from GetSMTLevel utility function~~ e2e: Remove gomega assertions from GetSMTLevel utility function Sep 26, 2025

openshift-ci bot requested review from jmencak and swatisehgal September 26, 2025 12:46

SargunNarula force-pushed the refactor_getsmtlevel branch from e6b9118 to 428c027 Compare September 26, 2025 13:37

SargunNarula changed the title ~~e2e: Remove gomega assertions from GetSMTLevel utility function~~ e2e: refactor GetSMTLevel to remove Gomega assertions and add multi arch support Sep 26, 2025

SargunNarula force-pushed the refactor_getsmtlevel branch from 428c027 to e6b9118 Compare September 29, 2025 09:53

SargunNarula changed the title ~~e2e: refactor GetSMTLevel to remove Gomega assertions and add multi arch support~~ e2e: refactor GetSMTLevel to remove Gomega assertions Sep 29, 2025

mrniranjan reviewed Sep 29, 2025

View reviewed changes

test/e2e/performanceprofile/functests/1_performance/cpu_management.go Outdated Show resolved Hide resolved

SargunNarula force-pushed the refactor_getsmtlevel branch from e6b9118 to 0be8ef3 Compare September 29, 2025 10:12

swatisehgal reviewed Sep 29, 2025

View reviewed changes

openshift-ci bot assigned swatisehgal Sep 29, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 29, 2025

Tal-or reviewed Sep 29, 2025

View reviewed changes

ffromani reviewed Sep 30, 2025

View reviewed changes

openshift-ci bot assigned ffromani Sep 30, 2025

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 30, 2025

SargunNarula changed the title ~~e2e: refactor GetSMTLevel to remove Gomega assertions~~ CNF-19657: e2e: refactor GetSMTLevel to remove Gomega assertions Sep 30, 2025

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Sep 30, 2025

openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Oct 1, 2025

SargunNarula changed the title ~~CNF-19657: e2e: refactor GetSMTLevel to remove Gomega assertions~~ OCPBUGS-62605: e2e: refactor GetSMTLevel to remove Gomega assertions Oct 1, 2025

openshift-ci-robot added the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Oct 1, 2025

openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Oct 1, 2025

openshift-merge-bot bot merged commit ccb071f into openshift:main Oct 1, 2025
18 of 19 checks passed

OCPBUGS-62605: e2e: refactor GetSMTLevel to remove Gomega assertions #1399

OCPBUGS-62605: e2e: refactor GetSMTLevel to remove Gomega assertions #1399

Uh oh!

Conversation

SargunNarula commented Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SargunNarula commented Sep 26, 2025

Uh oh!

Uh oh!

swatisehgal left a comment

Choose a reason for hiding this comment

Uh oh!

Tal-or Sep 29, 2025

Choose a reason for hiding this comment

Uh oh!

ffromani Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SargunNarula Sep 29, 2025

Choose a reason for hiding this comment

Uh oh!

Tal-or Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ffromani Sep 29, 2025

Choose a reason for hiding this comment

Uh oh!

ffromani Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

SargunNarula Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

SargunNarula commented Sep 29, 2025

Uh oh!

SargunNarula commented Sep 29, 2025

Uh oh!

SargunNarula commented Sep 29, 2025

Uh oh!

SargunNarula commented Sep 30, 2025

Uh oh!

SargunNarula commented Sep 30, 2025

Uh oh!

ffromani commented Sep 30, 2025

Uh oh!

Tal-or commented Sep 30, 2025

Uh oh!

ffromani left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Sep 30, 2025

Uh oh!

openshift-ci-robot commented Sep 30, 2025 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SargunNarula commented Sep 30, 2025

Uh oh!

SargunNarula commented Sep 30, 2025

Uh oh!

openshift-ci bot commented Sep 30, 2025

Uh oh!

mrniranjan commented Oct 1, 2025

Uh oh!

openshift-ci-robot commented Oct 1, 2025

Uh oh!

openshift-ci-robot commented Oct 1, 2025

Uh oh!

openshift-ci-robot commented Oct 1, 2025

Uh oh!

SargunNarula commented Oct 1, 2025

Uh oh!

openshift-ci-robot commented Oct 1, 2025

Uh oh!

openshift-ci bot commented Oct 1, 2025

Uh oh!

Uh oh!

openshift-ci-robot commented Oct 1, 2025

SargunNarula commented Sep 26, 2025 •

edited

Loading

ffromani Sep 29, 2025 •

edited

Loading

Tal-or Sep 29, 2025 •

edited

Loading

openshift-ci-robot commented Sep 30, 2025 •

edited by openshift-ci bot

Loading