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

Unified node interface #32

Merged
merged 11 commits into from Feb 16, 2021

Conversation

hannahhoward
Copy link
Collaborator

Goals

Unify the node interface for bitswap & IPFS transfers, approach unification of testcases for IPFS/graphsync/bitswap

My hope here -- and I haven't tackled whether we want the nodes to all have a Fetch method -- is that we can get to the point where if you can satisfy the Node interface, you can run it against the test infrastructure

Implementation

  • Define a general node interface
  • Make sure nodes do not interact with runenv (unit testable should we want that)
  • Modify Bitswap & IPFS interfaces to match Node interface (rename bitswap interface from Node -> BitswapNode)
  • Separate out all common code
  • Bitswap test now follows IPFS test much more closely
  • I incorporated the fractionalSeed code so it runs on all tests if specified, thought I'm not sure it would really work for IPFS. Also I'm not sure we're ever going to use this.
  • I don't see a lot of value in the GenSeedSerial variable, so I just took it out (we could put it back in for all tests)

For Discussion

transfer.go, ipfsTransfer.go, and graphsyncTransfer.go now look nearly identical. The only major difference is the fetching code. Which could be put in the node likely (thought there are interesting challenges cause some need a targer peer while others don't)

A further refactor is to make a common executor, that maybe takes a fetch function if we don't want to put it in the node. Perhaps it all becomes a single test case except for TCP? It certainly would make the testplan easier to read.

build a unified node interface for bitswap & IPFS
@adlrocha
Copy link
Contributor

Just wanted to drop by to let you know that I love where this is going. I wish I had had more time to approach this refactor. One of the initial goals for the testbed was to have the environment for anyone to be able to come with her or his exchange protocol implementation for IPFS (or their improved Bitswap/Graphsync), plug it into the testbed, and evaluate their performance against existing protocols. This refactor is definitely a step towards that vision.

A further refactor is to make a common executor, that maybe takes a fetch function if we don't want to put it in the node. Perhaps it all becomes a single test case except for TCP? It certainly would make the testplan easier to read.

And I think this would be something awesome to have. It would make the testplan easier to read and the testbed way more flexible. Let me know if you need additional input or a quick review from my side.

Copy link
Contributor

@acruikshank acruikshank left a comment

Choose a reason for hiding this comment

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

LGTM

@acruikshank acruikshank merged commit ab7aecd into feat/testfiles-in-bitswap Feb 16, 2021
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.

None yet

3 participants