OCPBUGS-99898: Add unit test for aggregateMachineMessages truncation logic - #9128
OCPBUGS-99898: Add unit test for aggregateMachineMessages truncation logic#9128hypershift-jira-solve-ci[bot] wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/valid-reference
|
@hypershift-jira-solve-ci[bot]: This pull request references Jira Issue OCPBUGS-99898, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/hypershift-operator
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9128 +/- ##
==========================================
+ Coverage 44.51% 44.56% +0.04%
==========================================
Files 774 774
Lines 97003 97003
==========================================
+ Hits 43185 43228 +43
+ Misses 50830 50783 -47
- Partials 2988 2992 +4 see 4 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Nirshal
left a comment
Nirshal
left a comment
There was a problem hiding this comment.
Two suggestions — one correctness fix, one missing boundary case.
| } | ||
|
|
||
| func TestAggregateMachineMessages(t *testing.T) { | ||
| g := NewWithT(t) |
There was a problem hiding this comment.
g is bound to the outer t, not the subtest's t inside t.Run. If a subtest fails, g.Expect().To() calls Fatalf on the parent test, killing the entire function and skipping all remaining subtests.
TestTruncateReasons and TestAggregateMachineReasonsAndMessages nearby have the same issue, but that's not a reason to perpetuate it — the file itself uses the correct pattern in many other places (lines 88, 127, 176, 291, 386, 522, 673, etc.).
Move g := NewWithT(t) inside the t.Run closure so each subtest gets its own gomega instance. The full test function should look like this:
func TestAggregateMachineMessages(t *testing.T) {
shortMsg := "machine is unhealthy\n"
padLen := maxMessageLength - len(shortMsg)
paddedMsg := strings.Repeat("x", padLen-1) + "\n"
for _, tc := range []struct {
name string
msgs []string
expect string
}{
// ... test cases unchanged ...
} {
t.Run(tc.name, func(t *testing.T) {
g := NewWithT(t)
result := aggregateMachineMessages(tc.msgs)
g.Expect(result).To(Equal(tc.expect))
})
}
}Concretely: remove the g := NewWithT(t) on this line, and add g := NewWithT(t) as the first line inside the t.Run callback.
There was a problem hiding this comment.
Done. Moved g := NewWithT(t) inside the t.Run closure so each subtest gets its own gomega instance.
AI-assisted response via Claude Code
| { | ||
| name: "When a single message exceeds the limit it should return only the truncation suffix", | ||
| msgs: []string{strings.Repeat("a", maxMessageLength+1)}, | ||
| expect: endOfMessage, |
There was a problem hiding this comment.
The boundary between "fits" and "truncated" for a single message is not covered. The existing "exact boundary" case uses two messages summing to 1000, and this "single oversized" case uses 1001. But a single message of exactly 1000 chars is missing — this is the tightest boundary test for the > condition on the first loop iteration (0 + 1000 = 1000, NOT > 1000, so it should pass through).
Add this test case to the table (e.g. before this one):
{
name: "When a single message is exactly maxMessageLength it should be included",
msgs: []string{strings.Repeat("z", maxMessageLength)},
expect: strings.Repeat("z", maxMessageLength),
},There was a problem hiding this comment.
Done. Added test case for a single message of exactly maxMessageLength chars to cover the tight boundary of the > condition.
AI-assisted response via Claude Code
|
/test address-review-comments |
|
Review agent triggered. View job |
27a3682 to
001cc30
Compare
|
/lgtm |
|
Scheduling tests matching the |
|
/label acknowledge-critical-fixes-only |
openshift-ci
Bot
added
the
acknowledge-critical-fixes-only
jparrill
left a comment
jparrill
left a comment
There was a problem hiding this comment.
Dropped a comment. Thanks!
| expect string | ||
| }{ | ||
| { | ||
| name: "When input is empty it should return empty string", |
There was a problem hiding this comment.
nit: Consider adding a nil input case alongside the empty slice. Go distinguishes nil from []string{} and aggregateMachineMessages handles both, but documenting that contract explicitly makes the test more robust as a spec.
{
name: "When input is nil it should return empty string",
msgs: nil,
expect: "",
},There was a problem hiding this comment.
Done. Added a nil input test case before the empty slice case.
AI-assisted response via Claude Code
|
/test address-review-comments |
|
Review agent triggered. View job |
The aggregateMachineMessages function had zero test coverage despite having truncation boundary logic prone to off-by-one errors. This adds a table-driven TestAggregateMachineMessages covering empty input, single message, multiple messages under the limit, exact boundary at 1000 chars, overflow with truncation suffix, and a single oversized message. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The test images is failing legitimately, we have a fix in this PR to fix #9138 |
001cc30 to
b70bf93
Compare
|
New changes are detected. LGTM label has been removed. |
|
/verified by unit tests No justification needed, just adding unit tests |
openshift-ci-robot
added
the
verified
|
@jparrill: This PR has been marked as verified by DetailsIn response to this:
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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@jparrill: This pull request references Jira Issue OCPBUGS-99898, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
DetailsIn response to this:
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. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hypershift-jira-solve-ci[bot], jparrill The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
@hypershift-jira-solve-ci[bot]: 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. |
3 participants
What this PR does / why we need it:
Adds table-driven unit tests for the
aggregateMachineMessagesfunction, which previously had zero test coverage despite containing truncation boundary logic prone to off-by-one errors.The test covers the following cases:
Which issue(s) this PR fixes:
Fixes https://redhat.atlassian.net/browse/OCPBUGS-99898
Special notes for your reviewer:
Pure test addition — no production code changes.
Checklist:
Always review AI generated responses prior to use.
Generated with Claude Code via openshift-developer plugin
Summary by CodeRabbit