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
fixes 92907 improves test error output #92908
fixes 92907 improves test error output #92908
Conversation
@@ -419,7 +419,7 @@ func verifyEvents(e coreclientset.EventInterface, options metav1.ListOptions, nu | |||
count += int(event.Count) | |||
} | |||
if count != num { | |||
return fmt.Errorf("expect event number %d, got %d: %v", num, count, events.Items) | |||
return fmt.Errorf("Expected %d events with reason set to %s and message set to %s\nbut %d actual events occured. Events : %v", num, reason, message, count, events.Items) |
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.
nitpick: I think convention is to not capitalize error message strings. Not sure if linter will actually check this or not
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.
Thanks Dan,let me do some surveying on this it would be good to know and follow convention
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.
Looks like the linter did catch the spelling of occurred
😅
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.
Survey complete, you are spot on, the only time fmt.Errorf
messages start with a capital letter is when the message starts with a exported type (I've only seen that in application code)
In test code, error messages always start with a lowercase letter ...
https://cs.k8s.io/?q=fmt.Errorf&i=nope&files=test&repos=kubernetes/kubernetes
/retest |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dashpole, RobertKielty 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:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Clarifies a misleading test error message
Which issue(s) this PR fixes:
Fixes #92907
Special notes for your reviewer:
Please see linked issue #92907
Does this PR introduce a user-facing change?: