-
Notifications
You must be signed in to change notification settings - Fork 59
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
Make tablet test provide more details on failure #209
Make tablet test provide more details on failure #209
Conversation
tablet_integration_test.go
Outdated
@@ -69,7 +69,9 @@ func TestTablets(t *testing.T) { | |||
} | |||
} | |||
|
|||
assertEqual(t, "queriedHosts", 1, queriedHosts) | |||
if queriedHosts != 1 { | |||
t.Errorf("trace should show only once host from the list %s, but it showed it %d, trace:\n%s", hostAddresses, queriedHosts, buf.String()) |
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.
you mean "one host" and "but it showed %d"? It is not about how many times it showed given host, but how many hosts it showed
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.
and I would consider using Fatalf
to fail on first unsuccessful query
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 have changed language, take a look.
286f98d
to
29f1112
Compare
tablet_integration_test.go
Outdated
@@ -69,7 +69,9 @@ func TestTablets(t *testing.T) { | |||
} | |||
} | |||
|
|||
assertEqual(t, "queriedHosts", 1, queriedHosts) | |||
if queriedHosts != 1 { | |||
t.Fatalf("trace should show host only once but it showed it %d, hosts: %s trace:\n%s", queriedHosts, hostAddresses, buf.String()) |
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 meant: "trace should show only one host, but it showed %d, hosts: %s trace: \n%s"
Host can be present in the trace multiple times but it has to be the same host
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, changed language again, take a look.
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.
It would be nice to have similar change in the second test in this file, could you add it?
29f1112
to
5a0d8be
Compare
Time to time this test fails due to not obviouse reason To make it possible investigate the issue we need to drop more details when it happens
5a0d8be
to
1d1ee11
Compare
Time to time this test fails due to not obviouse reason
To make it possible investigate the issue we need to drop more details when it happens
Failure example: