-
Notifications
You must be signed in to change notification settings - Fork 531
OCPBUGS-99898: Add unit test for aggregateMachineMessages truncation logic #9128
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2305,6 +2305,66 @@ func TestTruncateReasons(t *testing.T) { | |
| } | ||
| } | ||
|
|
||
| func TestAggregateMachineMessages(t *testing.T) { | ||
| shortMsg := "machine is unhealthy\n" | ||
| // Build a message that is exactly (1000 - len(shortMsg)) chars so the total is exactly 1000. | ||
| padLen := maxMessageLength - len(shortMsg) | ||
| paddedMsg := strings.Repeat("x", padLen-1) + "\n" | ||
|
|
||
| for _, tc := range []struct { | ||
| name string | ||
| msgs []string | ||
| expect string | ||
| }{ | ||
| { | ||
| name: "When input is nil it should return empty string", | ||
| msgs: nil, | ||
| expect: "", | ||
| }, | ||
| { | ||
| name: "When input is empty it should return empty string", | ||
| msgs: []string{}, | ||
| expect: "", | ||
| }, | ||
| { | ||
| name: "When a single short message is given it should return it verbatim", | ||
| msgs: []string{shortMsg}, | ||
| expect: shortMsg, | ||
| }, | ||
| { | ||
| name: "When multiple messages fit within limit it should return them all concatenated", | ||
| msgs: []string{"error one\n", "error two\n", "error three\n"}, | ||
| expect: "error one\nerror two\nerror three\n", | ||
| }, | ||
| { | ||
| name: "When messages exactly hit the 1000-char boundary it should include all without truncation", | ||
| msgs: []string{paddedMsg, shortMsg}, | ||
| expect: paddedMsg + shortMsg, | ||
| }, | ||
| { | ||
| name: "When messages exceed the 1000-char limit it should truncate and append suffix", | ||
| msgs: []string{paddedMsg, shortMsg, "this overflows\n"}, | ||
| expect: paddedMsg + shortMsg + endOfMessage, | ||
| }, | ||
| { | ||
| name: "When a single message is exactly maxMessageLength it should be included", | ||
| msgs: []string{strings.Repeat("z", maxMessageLength)}, | ||
| expect: strings.Repeat("z", maxMessageLength), | ||
| }, | ||
| { | ||
| name: "When a single message exceeds the limit it should return only the truncation suffix", | ||
| msgs: []string{strings.Repeat("a", maxMessageLength+1)}, | ||
| expect: endOfMessage, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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),
},
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. Added test case for a single message of exactly AI-assisted response via Claude Code |
||
| }, | ||
| } { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| g := NewWithT(t) | ||
| result := aggregateMachineMessages(tc.msgs) | ||
| g.Expect(result).To(Equal(tc.expect)) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestAggregateMachineReasonsAndMessages(t *testing.T) { | ||
| g := NewWithT(t) | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Consider adding a
nilinput case alongside the empty slice. Go distinguishesnilfrom[]string{}andaggregateMachineMessageshandles 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Added a
nilinput test case before the empty slice case.AI-assisted response via Claude Code