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

Rename multidim-interop to transport-interop #308

Merged
merged 5 commits into from
Oct 5, 2023

Conversation

thomaseizinger
Copy link
Contributor

@thomaseizinger thomaseizinger commented Sep 29, 2023

For now, I've left the old action in place to allow us updating all repositories separately. Once they are updated we should remove the old run-interop-ping-test action.

Resolves: #306.

@thomaseizinger
Copy link
Contributor Author

thomaseizinger commented Sep 29, 2023

cc @Menduist for nim-libp2p
cc @kevodwyer for java-libp2p

If you use the action provided in this repository in your own CI, you should update to the renamed one once this is merged.

@thomaseizinger
Copy link
Contributor Author

@maschad @achingbrain I saw that js-libp2p depends on a dedicated package that also still refers to multidim. No pressure to rename that too, I just wanted to point it out! :)

@maschad
Copy link
Member

maschad commented Oct 4, 2023

@thomaseizinger we can go through the process of deprecating the npm package and renaming it, as well as renaming the script which is current test causing the failure but if you don't want to be blocked by those tasks you should perhaps revert the js dockerfile changes in the interim and leave a TODO.

@thomaseizinger
Copy link
Contributor Author

@thomaseizinger we can go through the process of deprecating the npm package and renaming it, as well as renaming the script which is current test causing the failure but if you don't want to be blocked by those tasks you should perhaps revert the js dockerfile changes in the interim and leave a TODO.

Thanks for pointing that out! I've reverted that change and leave it up to you guys whether or not you want to go through the procedure of renaming that or leaving it as is.

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.

Works for me. Thank you!

@achingbrain
Copy link
Member

js-libp2p has an @libp2p/multidim-interop package that contains one interop test - ping.spec.ts.

The intention there was that it would one day include more than one test.

Is the expectation now that there would be multiple interop test modules each with one specific test? This seems like quite a lot of scaffolding if so.

If we keep it as one module you can filter which tests are run with something like:

$ aegir test -t node --grep 'ping'

or

$ aegir test -t node --grep 'holepunch'

or whatever, which should cover our requirements here?

@maschad
Copy link
Member

maschad commented Oct 6, 2023

That should work @achingbrain I agree it's not worth the extra scaffolding.

@thomaseizinger
Copy link
Contributor Author

@achingbrain At the moment there is a lot of duplication between the test runners in here, correct. I think the best way to fix this is to see how they evolve. The upcoming holepunch tests already use a slightly different structure in how things are emitted and it was easy because I just forked the multidim implementation.

I have some thoughts on rewriting all this to deno to remove some of the nodejs boilerplate and hopefully be able to more easily share scripts and modules across the various test runners. I don't expect us to add more "tests" to the multidim (now transport) interop. The relay tests will likely be yet another test runner.

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.

multidim-interop: rename to "transport"?
4 participants