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

port go tests #159

Merged
merged 13 commits into from
Dec 15, 2017
Merged

port go tests #159

merged 13 commits into from
Dec 15, 2017

Conversation

pgte
Copy link
Contributor

@pgte pgte commented Nov 29, 2017

Addresses #9, porting go-ipfs bitswap tests into here.

Also, because of making it harder to test, this PR proposes a simplification of the getMany implementation. Besides being simpler to track the algorithm, it also guarantees the ordering of returns, i.e., the order of the CIDs passed in is the same order of the respective blocks getting out.
This has the downside of the result array having holes in it because of cancelations.
(If this last is a serious breaking change, I could always filter the output before returning).

@ghost ghost assigned pgte Nov 29, 2017
@ghost ghost added the status/in-progress In progress label Nov 29, 2017
@daviddias
Copy link
Member

Besides being simpler to track the algorithm, it also guarantees the ordering of returns, i.e., the order of the CIDs passed in is the same order of the respective blocks getting out.

Given that some blocks will be local and others remote, it is better to start returning as soon as possible. For testing, you can always sort it there.

@pgte
Copy link
Contributor Author

pgte commented Nov 29, 2017

@diasdavid as far as I can tell, the previous algorithm was doing the same, no?

@daviddias
Copy link
Member

@pgte you seem to be correct. This is a perf optimization that bitswap needs then.

@daviddias
Copy link
Member

Before merging this one, could you make sure that this doesn't create any breaking change by going to js-ipfs, npm linking and running unit tests and interop tests?

@pgte
Copy link
Contributor Author

pgte commented Dec 14, 2017

@diasdavid interop tests run ok.
Unit tests fail, but at the same 3 places they're failing when using ipfs-bitswap master (screenshots attached)

1__bash

1__bash

BTW, this PR is still not done: I was awaiting feedback and then got side-tracked. Back on this!

@pgte pgte changed the title WIP: port go tests port go tests Dec 14, 2017
@pgte
Copy link
Contributor Author

pgte commented Dec 14, 2017

@diasdavid except for the rebroadcastTimeout tests, all possible tests in https://github.com/ipfs/go-ipfs/blob/master/exchange/bitswap/bitswap_test.go were replicated.

@pgte
Copy link
Contributor Author

pgte commented Dec 14, 2017

Also, depends on this for tests to run: libp2p/js-libp2p#138

@daviddias
Copy link
Member

Trying to release libp2p/js-libp2p#138 but npm is not cooperating. Will try again later (or ping me on IRC when you are around)

@daviddias
Copy link
Member

released. kicked off CI to start again :)

@ghost ghost assigned daviddias Dec 15, 2017
@daviddias daviddias merged commit 519e2a9 into master Dec 15, 2017
@ghost ghost removed the status/in-progress In progress label Dec 15, 2017
@daviddias daviddias deleted the chore/port-go-tests branch December 15, 2017 09:47
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.

2 participants