e2e: Fix broken assertions - #1571
Conversation
Expect(bool) without .To() is a Gomega no-op; these tests never fail on these assertions.
PR Summary by QodoE2E: Fix no-op Gomega assertions in performance profile tests
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughPerformance profile end-to-end tests now use explicit Gomega assertions, remove conditional PPC error logging, correct mixed CPU diagnostics, and include expected-versus-actual details for PPC and offline CPU-set failures. ChangesPerformance test assertion diagnostics
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: oblau 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 |
Code Review by Qodo
1.
|
![qodo-for-rh-openshift[bot]](https://avatars.githubusercontent.com/u/792337?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/e2e/performanceprofile/functests/10_performance_ppc/ppc.go`:
- Around line 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.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ff43d9d3-90fb-489e-be11-f312db4d9f15
📒 Files selected for processing (3)
test/e2e/performanceprofile/functests/10_performance_ppc/ppc.gotest/e2e/performanceprofile/functests/11_mixedcpus/mixedcpus.gotest/e2e/performanceprofile/functests/2_performance_update/updating_profile.go
| 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) |
There was a problem hiding this comment.
🎯 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))
PYRepository: 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.
| 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.
There was a problem hiding this comment.
interesting catch @oblau worth looking into that
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
@oblau sounds good. if you're running it locally, it's worth printing string(output) and see the full error message.
|
/retest |
| 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 { |
There was a problem hiding this comment.
That's a change in the behavior. it makes the test more restrict. is that what we want here?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
Expect(Get(...)) without .ToNot(HaveOccurred()) is a Gomega no-op; API errors were silently swallowed. Split into err assignment + Expect(err) to match the prevailing style in the file.
The `if ok { testlog.Info(...) }` pattern after regexp.MatchString
never failed the test when the expected error was absent from output.
Replace with Expect(ok).To(BeTrue()) so a missing error message
actually fails the test case.
b138108 to
9c24e1d
Compare
|
@oblau: 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. |
2 participants
Split into 3 commits by bug pattern — each fixes a distinct class of broken assertion across different test files.
Add missing matcher to offlined CPU Expect() calls (5 sites)
Expect(cpuSet.Equals(other))without a matcher is a gomega no-op — always passes.Added
.To(BeTrue())so the comparison is actually asserted.Add missing error check to mixedcpus Get() calls (3 sites)
Expect(client.Get(...))without a matcher silently swallows errors.Captured the error and added
Expect(err).ToNot(HaveOccurred()).PPC: replace no-op log-on-match with actual assertions (4 sites)
if ok { testlog.Info(...) }afterregexp.MatchStringonly logged on success — never failed the test when the expected error was missing from output.Replaced with
Expect(ok).To(BeTrue()).Summary by CodeRabbit