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

feat(hole-punch): add hole-punch interoperability test suite #304

Merged
merged 88 commits into from
Oct 16, 2023

Conversation

thomaseizinger
Copy link
Contributor

@thomaseizinger thomaseizinger commented Sep 14, 2023

How to review this

  1. Read the README
  2. Look at src/generator.ts
  3. Look at testplans.ts

Most of the other files like helpers/cache.ts are a 1-to-1 copy from multidim-interop.

Follow-ups

  • Investigate why Rust test has double the RTT for the hole-punched connection on TCP in one direction
  • Assert RTT once above is fixed

@thomaseizinger
Copy link
Contributor Author

Don't be fooled by green CI, it is actually failing :D (But was working locally)

@thomaseizinger
Copy link
Contributor Author

Don't be fooled by green CI, it is actually failing :D (But was working locally)

Now we actually have passing tests!

@thomaseizinger
Copy link
Contributor Author

@marten-seemann This is ready to have a go-peer added! I am curious to learn whether the go-peer also observed a weird RTT via the TCP connection. That would tell us whether this is an issue with our network setup or a bug in rust-libp2p somewhere.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

🎉

➜  hole-punch-interop git:(feat/hole-punch-tests) npm run test

> libp2p hole-punch test@0.0.1 test
> ts-node src/*.test.ts && ts-node testplans.ts

Running 2 tests
Running test spec: rust-v0.52 x rust-v0.52 (quic)
Running test spec: rust-v0.52 x rust-v0.52 (tcp)
Expected RTT of direct connection to be ~200ms but was 401ms
0 failures:
Run complete

The amount of code duplication to the existing interop tests is unfortunate. That said, the duplication might just be easier here than a bad abstraction.

hole-punch-interop/README.md Outdated Show resolved Hide resolved
@thomaseizinger
Copy link
Contributor Author

The amount of code duplication to the existing interop tests is unfortunate. That said, the duplication might just be easier here than a bad abstraction.

I have two ideas to deal with this:

  1. Get rid of most of the code which just deals with caching: Re-consider container caching strategy to use registries #303
    • Makefiles
    • Cache helpers
    • Build shell-script
    • Version resolution
  2. Rewrite the rest to use deno:
    • Deno is much more a "convention over configuration" approach. It directly supports typescript for example.
    • Sharing utility functions across the various test runners should be easy as a result

@thomaseizinger
Copy link
Contributor Author

thomaseizinger commented Oct 9, 2023

@Menduist Would you have time to write a nim-libp2p client for this test suite so we can get a rough idea of whether it is working across implementations at this point?

@thomaseizinger
Copy link
Contributor Author

Hey everyone! This has been ready for additions of other languages for a while. Instead of having it sit here, my suggestion would be to merge it as is and activate it in rust-libp2p for now. That way, we get some insight on how stable the tests are once they are run on each PR.

Other languages are then free to add their implementation at their own pace.

Any objections @mxinden @marten-seemann @MarcoPolo @achingbrain @maschad @Menduist (not sure who else to tag?)?

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

I am in favor of moving forward here. What do others think?

@Menduist
Copy link
Contributor

Sorry, just seen the message, didn't realize this was already ready for integration
@diegomrsantos will add a nim implementation, but feel free to merge in the meantime :)

@thomaseizinger
Copy link
Contributor Author

I am in favor of moving forward here. What do others think?

I am planning to merge this in ~24h unless there are any concerns raised!

@thomaseizinger thomaseizinger merged commit 1e37b93 into libp2p:master Oct 16, 2023
1 check passed
mergify bot pushed a commit to libp2p/rust-libp2p that referenced this pull request Oct 25, 2023
Related: libp2p/test-plans#304.

Pull-Request: #4549.

Co-authored-by: diegomrsantos <diegomrsantos@gmail.com>
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.

add tests for hole punching
3 participants