Skip to content

e2e: Fix broken assertions #1571

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
oblau wants to merge 3 commits into
base: main
Choose a base branch
from
+13 −22
Open

e2e: Fix broken assertions #1571

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
258a375
E2E: add missing matcher to offlined CPU Expect() calls (5 sites)
oblau Jul 23, 2026
9bd37cd
E2E: add missing error check to mixedcpus Get() calls (3 sites)
oblau Jul 23, 2026
9c24e1d
E2E: PPC: replace no-op log-on-match with actual assertions (4 sites)
oblau Jul 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 4 additions & 13 deletions test/e2e/performanceprofile/functests/10_performance_ppc/ppc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import (
performancev2 "github.com/openshift/cluster-node-tuning-operator/pkg/apis/performanceprofile/v2"
testutils "github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/utils"
"github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/utils/label"
testlog "github.com/openshift/cluster-node-tuning-operator/test/e2e/performanceprofile/functests/utils/log"
"k8s.io/utils/cpuset"
"sigs.k8s.io/yaml"
)
Expand Down Expand Up @@ -120,9 +119,7 @@ var _ = Describe("[rfe_id: 38968] PerformanceProfile setup helper and platform a
errString := "Error: failed to obtain data from flags not appropriate to split reserved CPUs in case of topology-manager-policy: single-numa-node"
ok, err := regexp.MatchString(errString, string(output))
Expect(err).ToNot(HaveOccurred())
if ok {

@Tal-or Tal-or Jul 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That's a change in the behavior. it makes the test more restrict. is that what we want here?

@oblau oblau Jul 27, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is changing behavior - but isn't this the intention here?
If we are not asserting on ok then i fail to see why we need the errString
and regexp.MatchString(errString, string(output)) to begin with.
Maybe im missing something here.

Is the goal to make sure the correct error message appears, or that PPC script fails in general?

@Tal-or Tal-or Jul 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

After reevaluation I think this change is ok.
the only change I would add is in the message to make it more clear:

Expect(ok).To(BeTrue(), "expected error %q to be found in output: %s", errString, output)

testlog.Info(errString)
}
Expect(ok).To(BeTrue(), "expected error %q not found in output: %s", errString, output)
Eventually(session).Should(gexec.Exit(1))
})

Expand All @@ -144,9 +141,7 @@ var _ = Describe("[rfe_id: 38968] PerformanceProfile setup helper and platform a
errString := "Error: failed to compute the reserved and isolated CPUs: can't allocate odd number of CPUs from a NUMA Node"
ok, err := regexp.MatchString(errString, string(output))
Expect(err).ToNot(HaveOccurred(), "did not fail with Expected:%s failure", errString)
if ok {
testlog.Info(errString)
}
Expect(ok).To(BeTrue(), "expected error %q not found in output: %s", errString, output)
Eventually(session).Should(gexec.Exit(1))
})

Expand All @@ -169,9 +164,7 @@ var _ = Describe("[rfe_id: 38968] PerformanceProfile setup helper and platform a
errString := "Error: failed to compute the reserved and isolated CPUs: please specify the reserved CPU count in the range [1,3]"
ok, err := regexp.MatchString(errString, string(output))
Expect(err).ToNot(HaveOccurred(), "did not fail with Expected:%s failure", errString)
if ok {
testlog.Info(errString)
}
Expect(ok).To(BeTrue(), "expected error %q not found in output: %s", errString, output)
Comment on lines 164 to +167

@coderabbitai coderabbitai Bot Jul 23, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== locate file =="
git ls-files | grep -F 'test/e2e/performanceprofile/functests/10_performance_ppc/ppc.go' || true

echo "== relevant source =="
if [ -f test/e2e/performanceprofile/functests/10_performance_ppc/ppc.go ]; then
  sed -n '130,190p' test/e2e/performanceprofile/functests/10_performance_ppc/ppc.go
fi

echo "== deterministic regex behavior probe =="
python3 - <<'PY'
import re

errString = "Error: failed to compute the reserved and isolated CPUs: please specify the reserved CPU count in the range [1,3]"
output_same = errString
output_differed = "Error: failed to compute the reserved and isolated CPUs: please specify the reserved CPU count in the range [4,5]"
quoted = re.escape(errString)

for name, pat in [("errString", errString), ("regexp.QuoteMeta(errString)", quoted)]:
    print(name)
    try:
        print(" same matches:", bool(re.search(pat, output_same)))
        print(" differed matches:", bool(re.search(pat, output_differed)))
    except Exception as e:
        print(" regex error:", repr(e))
PY

Repository: openshift/cluster-node-tuning-operator

Length of output: 3438

Escape the literal CPU range before regex matching.

errString contains [1,3], but regexp.MatchString treats that as a character-class token, so it won't match the literal range: [1,3] text in the command output reliably.

Proposed fix 
-			ok, err := regexp.MatchString(errString, string(output))
+			ok, err := regexp.MatchString(regexp.QuoteMeta(errString), string(output))
📝 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
errString := "Error: failed to compute the reserved and isolated CPUs: please specify the reserved CPU count in the range [1,3]"
ok, err := regexp.MatchString(errString, string(output))
Expect(err).ToNot(HaveOccurred(), "did not fail with Expected:%s failure", errString)
if ok {
testlog.Info(errString)
}
Expect(ok).To(BeTrue(), "expected error %q not found in output: %s", errString, output)
errString := "Error: failed to compute the reserved and isolated CPUs: please specify the reserved CPU count in the range [1,3]"
ok, err := regexp.MatchString(regexp.QuoteMeta(errString), string(output))
Expect(err).ToNot(HaveOccurred(), "did not fail with Expected:%s failure", errString)
Expect(ok).To(BeTrue(), "expected error %q not found in output: %s", errString, output)
🤖 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/performanceprofile/functests/10_performance_ppc/ppc.go` around lines
164 - 167, Update the error matching in the test around errString and
regexp.MatchString to escape the literal CPU range before regex evaluation,
using regexp.QuoteMeta or an equivalent approach. Preserve the existing expected
message and assertions while ensuring the output comparison matches the literal
“[1,3]” text.

@Tal-or Tal-or Jul 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

interesting catch @oblau worth looking into that

@oblau oblau Jul 27, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Is the intention that the output will contain explicitly [1,3]?
If so im not sure regexp.MatchString is the right tool to begin with.
Would Expect(string(output)).To(ContainSubstring(errString)) be better here? we already use Expect(ppcErrorString).To(ContainSubstring("...") on test_id:41420

@coderabbitai coderabbitai Bot Jul 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

@Tal-or Tal-or Jul 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@oblau sounds good. if you're running it locally, it's worth printing string(output) and see the full error message.

Eventually(session).Should(gexec.Exit(1))
})

Expand Down Expand Up @@ -217,9 +210,7 @@ var _ = Describe("[rfe_id: 38968] PerformanceProfile setup helper and platform a
errString := `please use one of \[default low-latency\] power consumption modes together with the perPodPowerManagement`
ok, err := regexp.MatchString(errString, string(output))
Expect(err).ToNot(HaveOccurred(), "did not fail with Expected:%s failure", errString)
if ok {
testlog.Info(errString)
}
Expect(ok).To(BeTrue(), "expected error %q not found in output: %s", errString, output)
Eventually(session).Should(gexec.Exit(1))
})
})
Expand Down
8 changes: 4 additions & 4 deletions test/e2e/performanceprofile/functests/11_mixedcpus/mixedcpus.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,8 +320,8 @@ var _ = Describe("Mixedcpus", Ordered, Label(string(label.MixedCPUs)), func() {
By(fmt.Sprintf("Waiting when %s finishes updates", poolName))
profilesupdate.WaitForTuningUpdated(context.TODO(), profile)

Expect(testclient.ControlPlaneClient.Get(ctx, client.ObjectKeyFromObject(profile), profile))
testlog.Infof("new isolated CPU set=%q\nnew shared CPU set=%q", string(*profile.Spec.CPU.Isolated), string(*profile.Spec.CPU.Isolated))
Expect(testclient.ControlPlaneClient.Get(ctx, client.ObjectKeyFromObject(profile), profile)).To(Succeed())
testlog.Infof("new isolated CPU set=%q\nnew shared CPU set=%q", string(*profile.Spec.CPU.Isolated), string(*profile.Spec.CPU.Shared))
// we do not bother to revert the profile at the end of the test, since its irrelevant which of the cpus are shared
})

Expand Down Expand Up @@ -502,7 +502,7 @@ var _ = Describe("Mixedcpus", Ordered, Label(string(label.MixedCPUs)), func() {
Expect(pod.Status.Phase).To(Equal(corev1.PodPending), "Pod %s is not in the pending state", pod.Name)

By("Reverting the cluster to previous state")
Expect(testclient.ControlPlaneClient.Get(ctx, client.ObjectKeyFromObject(profile), profile))
Expect(testclient.ControlPlaneClient.Get(ctx, client.ObjectKeyFromObject(profile), profile)).To(Succeed())
profile.Spec.CPU.Shared = cpuSetToPerformanceCPUSet(ppShared)
profile.Spec.WorkloadHints.MixedCpus = ptr.To(true)
profiles.UpdateWithRetry(profile)
Expand Down Expand Up @@ -872,7 +872,7 @@ func setup(ctx context.Context) func(ctx2 context.Context) {

teardown := func(ctx2 context.Context) {
By(fmt.Sprintf("executing teardown - revert profile %q back to its initial state", profile.Name))
Expect(testclient.ControlPlaneClient.Get(ctx2, client.ObjectKeyFromObject(initialProfile), profile))
Expect(testclient.ControlPlaneClient.Get(ctx2, client.ObjectKeyFromObject(initialProfile), profile)).To(Succeed())
profiles.UpdateWithRetry(initialProfile)

// do not wait if nothing has changed
Expand Down
10 changes: 5 additions & 5 deletions test/e2e/performanceprofile/functests/2_performance_update/updating_profile.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -637,7 +637,7 @@ var _ = Describe("[rfe_id:28761][performance] Updating parameters in performance
Expect(err).ToNot(HaveOccurred())
offlinedCPUSetProfile, err := cpuset.Parse(string(offlined))
Expect(err).ToNot(HaveOccurred())
Expect(offlinedCPUSet.Equals(offlinedCPUSetProfile))
Expect(offlinedCPUSet.Equals(offlinedCPUSetProfile)).To(BeTrue(), "offlined CPUs mismatch: expected %q, got %q", offlinedCPUSetProfile, offlinedCPUSet)
}
})

Expand Down Expand Up @@ -707,7 +707,7 @@ var _ = Describe("[rfe_id:28761][performance] Updating parameters in performance
Expect(err).ToNot(HaveOccurred())
offlinedCPUSetProfile, err := cpuset.Parse(string(offlinedSet))
Expect(err).ToNot(HaveOccurred())
Expect(offlinedCPUSet.Equals(offlinedCPUSetProfile))
Expect(offlinedCPUSet.Equals(offlinedCPUSetProfile)).To(BeTrue(), "offlined CPUs mismatch: expected %q, got %q", offlinedCPUSetProfile, offlinedCPUSet)
}
})

Expand Down Expand Up @@ -770,7 +770,7 @@ var _ = Describe("[rfe_id:28761][performance] Updating parameters in performance
Expect(err).ToNot(HaveOccurred())
offlinedCPUSetProfile, err := cpuset.Parse(string(offlinedSet))
Expect(err).ToNot(HaveOccurred())
Expect(offlinedCPUSet.Equals(offlinedCPUSetProfile))
Expect(offlinedCPUSet.Equals(offlinedCPUSetProfile)).To(BeTrue(), "offlined CPUs mismatch: expected %q, got %q", offlinedCPUSetProfile, offlinedCPUSet)
}
})

Expand Down Expand Up @@ -840,7 +840,7 @@ var _ = Describe("[rfe_id:28761][performance] Updating parameters in performance
Expect(err).ToNot(HaveOccurred())
offlinedCPUSetProfile, err := cpuset.Parse(string(offlinedSet))
Expect(err).ToNot(HaveOccurred())
Expect(offlinedCPUSet.Equals(offlinedCPUSetProfile))
Expect(offlinedCPUSet.Equals(offlinedCPUSetProfile)).To(BeTrue(), "offlined CPUs mismatch: expected %q, got %q", offlinedCPUSetProfile, offlinedCPUSet)
}
})

Expand Down Expand Up @@ -966,7 +966,7 @@ var _ = Describe("[rfe_id:28761][performance] Updating parameters in performance
Expect(err).ToNot(HaveOccurred())
offlinedCPUSetProfile, err := cpuset.Parse(string(offlinedSet))
Expect(err).ToNot(HaveOccurred())
Expect(offlinedCPUSet.Equals(offlinedCPUSetProfile))
Expect(offlinedCPUSet.Equals(offlinedCPUSetProfile)).To(BeTrue(), "offlined CPUs mismatch: expected %q, got %q", offlinedCPUSetProfile, offlinedCPUSet)
}
})

Expand Down