-
Notifications
You must be signed in to change notification settings - Fork 228
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 query interactions with routing table #887
Conversation
There is an occasional test failure which is caused by a race in the routing table. This needs probe-lab/go-kademlia#115 to be completed (see probe-lab/go-kademlia#121) |
Test failure is a test from this PR. I will investigate. |
I've merged a version of #890 into this to get access to working builds. There should not be any conflicts but it would probably be better to merge this after #820 since that has had a couple of recent small changes plus #896 is about to be merged into it Requires probe-lab/go-kademlia#122 |
@dennis-tra should be ready for another look |
@@ -130,14 +130,18 @@ func (q *Query[K, N]) Advance(ctx context.Context, ev QueryEvent) QueryState { | |||
|
|||
switch tev := ev.(type) { | |||
case *EventQueryCancel: | |||
span.SetAttributes(tele.AttrEvent("EventQueryCancel")) |
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.
could have something like:
span.SetAttributes(tele.AttrEvent(fmt.Sprintf("%T", ev)))
at the top of this method. fmt
is already imported.
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.
or add it right to L122
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'm wary of adding allocations for tracing since they will affect performance
// MaybeTrace returns a context containing a new root span named after the test. It creates an new | ||
// tracing provider and installs it as the global provider, restoring the previous provider at the | ||
// end of the test. This function cannot be called from tests that are run in parallel. | ||
func MaybeTrace(t *testing.T, ctx context.Context) (context.Context, trace.TracerProvider) { |
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.
cool!
This change ports two tests from v1: TestRTAdditionOnSuccessfulQuery and TestRTEvictionOnFailedQuery
Part of #888