Skip to content
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

Merged
merged 30 commits into from
Sep 18, 2023
Merged

Test query interactions with routing table #887

merged 30 commits into from
Sep 18, 2023

Conversation

iand
Copy link

@iand iand commented Sep 8, 2023

This change ports two tests from v1: TestRTAdditionOnSuccessfulQuery and TestRTEvictionOnFailedQuery

Part of #888

@iand iand added the v2 All issues related to the v2 rewrite label Sep 11, 2023
@iand
Copy link
Author

iand commented Sep 12, 2023

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)

@iand
Copy link
Author

iand commented Sep 12, 2023

Test failure is a test from this PR. I will investigate.

@iand iand self-assigned this Sep 12, 2023
@iand
Copy link
Author

iand commented Sep 13, 2023

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

@iand
Copy link
Author

iand commented Sep 16, 2023

@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"))
Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Author

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool!

@iand iand merged commit 1d35505 into v2-develop Sep 18, 2023
11 checks passed
@dennis-tra dennis-tra deleted the v2-query branch September 18, 2023 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v2 All issues related to the v2 rewrite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants