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
test/e2e/common/node: enhance assertions #110127
Conversation
Welcome @jwtty! |
Hi @jwtty. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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 kubernetes/test-infra repository. |
/assign @pohly |
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.
/ok-to-test
Thanks for taking care of this. Some initial comments, still need to go through the entire diff...
test/e2e/common/node/lease.go
Outdated
@@ -89,7 +89,9 @@ var _ = SIGDescribe("Lease", func() { | |||
|
|||
readLease, err := leaseClient.Get(context.TODO(), name, metav1.GetOptions{}) | |||
framework.ExpectNoError(err, "couldn't read Lease") | |||
framework.ExpectEqual(apiequality.Semantic.DeepEqual(lease.Spec, readLease.Spec), true) | |||
if !apiequality.Semantic.DeepEqual(lease.Spec, readLease.Spec) { | |||
framework.Failf("expected lease spec: %#v, got %#v", lease.Spec, readLease.Spec) |
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.
We don't have good pretty-printing of API objects. Gomega's format.Object indents them nicely, but prints empty fields. There's a discussion going elsewhere about that.
I don't have a strong opinion about printf-style format strings. Can you perhaps try out which looks best (%s, %+v, %#v)?
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.
If we print as a single line, the printing the two objects on top of each other is easier to compare:
"Leases don't match. Expected:\n\t%s\nActual:\n\t%s"
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.
I wrote a piece of code to experiment:
s1 := spec{
HolderIdentity: sPtr("holder"),
LeaseDurationSeconds: iPtr(30),
AcquireTime: µTime{Time: time.Time{}.Add(2 * time.Second)},
RenewTime: µTime{Time: time.Time{}.Add(5 * time.Second)},
LeaseTransitions: iPtr(0),
}
fmt.Printf("%#v\n", s1)
fmt.Println("=====================")
fmt.Printf("%s\n", s1)
fmt.Println("=====================")
fmt.Printf("%+v", s1)
And result is:
main.spec{HolderIdentity:(*string)(0xc000010250), LeaseDurationSeconds:(*int32)(0xc000018030), AcquireTime:time.Date(1, time.January, 1, 0, 0, 2, 0, time.UTC), RenewTime:time.Date(1, time.January, 1, 0, 0, 5, 0, time.UTC), LeaseTransitions:(*int32)(0xc000018034)}
=====================
{%!s(*string=0xc000010250) %!s(*int32=0xc000018030) 0001-01-01 00:00:02 +0000 UTC 0001-01-01 00:00:05 +0000 UTC %!s(*int32=0xc000018034)}
=====================
{HolderIdentity:0xc000010250 LeaseDurationSeconds:0xc000018030 AcquireTime:0001-01-01 00:00:02 +0000 UTC RenewTime:0001-01-01 00:00:05 +0000 UTC LeaseTransitions:0xc000018034}
I personally prefer the %+v
output, informative enough and not too long. What do you think?
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.
I agree, let's use that.
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.
However, it won't help to diagnose the problem. It's better than before, but not good enough. For example, if HolderIdentity is different, then the output will just show the addresses, not the string.
Perhaps Failf should show the result of https://github.com/google/go-cmp?
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.
go-cmp would be much better there 👍
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.
I switched to use go-cmp.
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.
This is much uglier code for not a meaningful test output improvement but logic looks fine.
We should consider ExpectTrue
/ExpectFalse
helpers that can skip the slightly uglier comparison output, but not litter the code base with if THING { fail... }
.
/triage accepted |
I think that's subjective. I find the if check easier to read than the
It might not have more meaning, but it's at least less distracting because it is just the failure message without the "expected false to be true" boilerplate. FWIW, I agree with your suggestions to enhance the failure messages while touching the code. |
Thanks for the comments. I'm working on improving the error msgs. |
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.
/approve
/lgtm
thank you!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: endocrimes, jwtty 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 |
/retest |
1 similar comment
/retest |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Better output for developers when tests fail.
Which issue(s) this PR fixes:
Fixes part of #105678
Special notes for your reviewer:
After this PR:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: