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

chore(client): basic query testing using mock net #83

Merged
merged 1 commit into from
Feb 10, 2023

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Feb 10, 2023

I've been working on some deeper testing as I iterate toward unixfs pathing (I really want to be able to do graph transfers to test that we're getting exactly the blocks we want). This is some of my initial forays into network-level testing.

  • I realised that we never gave Query or QueryResult the bindnode treatment so I did that in here but expect that to be extracted out to a go-markets-types repo in the near future; in the process I replaced the "rpc" stuff with some go-ipld-prime processing (using feat(dagcbor): mode to allow parsing undelimited streamed objects ipld/go-ipld-prime#490, hence the untagged dependency).
  • During testing I became concerned about messaging errors not properly resulting in a quick error turnaround; so you'll see some goroutine work in the client query to try and deal with that. It also tries to work in some additional context cancellation handling too because we use that a lot, although I hope that's largely dealt with by libp2p already. Maybe I've gone overboard here?

@codecov-commenter
Copy link

codecov-commenter commented Feb 10, 2023

Codecov Report

Merging #83 (7d3689c) into main (1bab381) will increase coverage by 3.94%.
The diff coverage is 77.96%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #83      +/-   ##
==========================================
+ Coverage   44.34%   48.29%   +3.94%     
==========================================
  Files          28       29       +1     
  Lines        2546     2578      +32     
==========================================
+ Hits         1129     1245     +116     
+ Misses       1371     1274      -97     
- Partials       46       59      +13     
Impacted Files Coverage Δ
pkg/client/client.go 25.88% <69.44%> (+25.88%) ⬆️
pkg/types/query.go 91.30% <91.30%> (ø)

Copy link
Collaborator

@hannahhoward hannahhoward left a comment

Choose a reason for hiding this comment

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

I would also argue that anything that runs on a mock net should live in an itest directory outside the hierarchy.

We should be ideally making the client have more abstract deps so it can be used independently.

I'm going to go ahead and merge cause it's certainly an improvement, but good feedback as you continue to make progress.

return err
}

mqn.server.SetStreamHandler(retrievalmarket.QueryProtocolID, func(s network.Stream) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

the complexity of this simple response points to a challenging long term issue when we build these -- I don't see how we respond to actual graphsync requests with filecoin retrieval negotiation happening on top without putting actual graphsync and the go-fil-markets retrieval server on top. Which don't get me wrong -- we can do, I just want to identify how hard that might end up being.

@@ -0,0 +1,236 @@
package client_test
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would also argue that anything that runs on a mock net should live in an itest directory outside the hierarchy. The solution for a test that lives in 'client' is to make it more injectable so we can unit test it.

I'm not religious about this though.

@hannahhoward hannahhoward merged commit 1623460 into main Feb 10, 2023
@rvagg rvagg deleted the rvagg/query-testing branch February 10, 2023 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants