.github: introduce interop tests

[laurentsenta]

Description

Adds two workflows on push & PR:

• run-ping-interop-cross-version: runs a Testground interoperability test between multiple versions of rust-libp2p, including master, and the current branch (during a pull request)
• run-ping-interop-cross-implementation: runs a Testground interoperability test between go-libp2p and rust-libp2p, including master, and the current branch (during a pull request)

We rely on the https://github.com/libp2p/test-plans/ repository to retrieve and run the tests.

Demonstration:

• passing PR: demo/pass laurentsenta/rust-libp2p#1
• breaking PR: demo/break laurentsenta/rust-libp2p#2

Links to any relevant issues

Related PR in test-plans: libp2p/test-plans#26

Open Questions

Known issues:

• The tests take between 20 minutes and 30 minutes on failures

Change checklist

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• A changelog entry has been made in the appropriate crates

[mxinden]

https://github.com/libp2p/rust-libp2p/runs/7949749580?check_suite_focus=true failed at the Go build stage. I am rerunning the test.

[laurentsenta]

@mxinden that won't work, we need to patch the go tests,

I was planning on disabling the go tests temporarily (libp2p/test-plans#30)
But since we got an agreement between go and rust libp2p to try out the interop with a retry policy,
I'll fix the tests and then bump this PR. Sorry for the noise, it should take ~ a day.

[laurentsenta]

Here you go, tests have been fixed, please see the demo branches for results.

[laurentsenta]

@mxinden I'm worried this PR will go stale if we make a release and break the rust side of tests before we merge.
That will break the go side too,

is there anything I could do to help with reviewing/moving this forward?

[thomaseizinger]

I've not been involved in these test plans so please tell me if my idea has already been considered:

These inter-op tests seem like something that does not really belong into either repository (go / rust). I am also not sure whether running them on every PR is the most efficient way of dealing with inter-op problems. Having a failing build in one repository will take time from maintainers, even if them breaking was actually caused by the other repository.

I have an alternative idea for how we could set this up and I am curious to hear your thoughts:

1. Run the actual inter-op tests in the test-plans repository in a workflow with a repository_dispatch trigger.
2. Add a on: push branches: master workflow to the rust and go repositories that uses curl (or the GH CLI) to trigger the workflow by creating a repository dispatch event: https://docs.github.com/en/rest/repos/repos#create-a-repository-dispatch-event

This decouples the actual inter-op tests from PRs on individual repositories. The differences to the current solution are:

• We will only know about inter-op problems once a PR has been merged although I think this can also be a good thing. Many intermediary pushes on PRs are unfinished work and triggering inter-op tests for seems like a waste.
• Someone has to monitor the results of these inter-op tests or we need some kind of automation that reacts to the inter-op tests being broken.

An alternative to using repository_dispatch would be to run them on a schedule but that would make it harder to identify, which PR actually broke something.

[mxinden]

I am also not sure whether running them on every PR is the most efficient way of dealing with inter-op problems.

From a resource perspective? Given that they can run on a single machine, I don't think we have to worry about resource usage.

Having a failing build in one repository will take time from maintainers, even if them breaking was actually caused by the other repository.

If I understand correctly, with this patch, each rust-libp2p pull request would be tested against go-libp2p master. Is that correct @laurentsenta? I don't think that is a good idea. We can not expect the go-libp2p master branch to be clean at all times. Instead I would suggest testing each rust-libp2p pull request only against rust-libp2p master, rust-libp2p releases and go-libp2p releases, but not go-libp2p master.

Would you agree @laurentsenta and @thomaseizinger?

---

I have an alternative idea for how we could set this up and I am curious to hear your thoughts:

1. Run the actual inter-op tests in the `test-plans` repository in a workflow with a `repository_dispatch` trigger.

2. Add a `on: push branches: master` workflow to the rust and go repositories that uses `curl` (or the GH CLI) to trigger the workflow by creating a repository dispatch event: https://docs.github.com/en/rest/repos/repos#create-a-repository-dispatch-event

This decouples the actual inter-op tests from PRs on individual repositories. The differences to the current solution are:

* We will only know about inter-op problems once a PR has been merged although I think this can also be a good thing. Many intermediary pushes on PRs are unfinished work and triggering inter-op tests for seems like a waste.

* Someone has to monitor the results of these inter-op tests or we need some kind of automation that reacts to the inter-op tests being broken.

An alternative to using repository_dispatch would be to run them on a schedule but that would make it harder to identify, which PR actually broke something.

I don't think I understand what the benefit of this approach would be. I would prefer knowing whether a PR breaks compatibility before it was merged. I would like to not have to monitor any CI systems, but instead be warned before merging a pull request due to CI failing.

[mxinden]

Given that we are still in an early stage and thus will gain a lot from actually running these tests continuously, I am fine merging here and iterating on this in follow-up pull requests.

@laurentsenta I want to give @thomaseizinger a chance to reply. In case he doesn't have a strong opinion on blocking this, I will merge.

[galargh]

Having a failing build in one repository will take time from maintainers, even if them breaking was actually caused by the other repository.

I think @mxinden's suggestion to test only against go-libp2p releases is exactly what's needed to address this concern.

• We will only know about inter-op problems once a PR has been merged although I think this can also be a good thing. Many intermediary pushes on PRs are unfinished work and triggering inter-op tests for seems like a waste.

As long as we're not resource constrained, I don't we should worry about this. What we're generating here is not noise - one might decide to disregard if they know the work is unfinished but that's OK.

• Someone has to monitor the results of these inter-op tests or we need some kind of automation that reacts to the inter-op tests being broken.

That's why even if we didn't want to have the workflow run on every PR, I'd argue that simply changing the trigger to push: branches: - master instead of push + pull_request might be a better solution because the signal would already be where the maintainers are.

However, if we wanted to test master-master compatibility between the implementations, I think @thomaseizinger's idea would be perfect!

All in all, in my opinion, as long as we can afford running the workflow on every PR, we should.

[thomaseizinger]

An alternative to using repository_dispatch would be to run them on a schedule but that would make it harder to identify, which PR actually broke something.

I don't think I understand what the benefit of this approach would be. I would prefer knowing whether a PR breaks compatibility before it was merged. I would like to not have to monitor any CI systems, but instead be warned before merging a pull request due to CI failing.

Testing each merge into master would yield one run per pull request so we would be able to identify which PR broke something.

Regarding knowing before the PR is merged: In general, I think it is good to catch as many problems as possible as early as possible. But there is also a cost to that. For example, it would be good to catch more bugs at compile time rather than at runtime with tests but sometimes, the former design is too complicated or simply undesirable. Similarly, I think there is merit in running different tests at different stages.

Concretely, I think it makes sense to check things at PR merge time where we can say with high certainty that it is a bug in our software (rust-libp2p). I am not convinced that inter-op tests fall into this category, even if we check against go-libp2p's releases. The opposite is also true: A bug in rust-libp2p shouldn't stall development on the go-libp2p side or flag a PR as broken when it actually isn't.

Having to manually monitor CI system is not good but it is a problem that can be solved with automation. For example, the workflow run could open an issue on every failed run and tag the maintainers of both repositories or whoever is deemed to be responsible.

@laurentsenta I want to give @thomaseizinger a chance to reply. In case he doesn't have a strong opinion on blocking this, I will merge.

My concern is not blocking so feel free to merge.

[mxinden]

Concretely, I think it makes sense to check things at PR merge time where we can say with high certainty that it is a bug in our software (rust-libp2p). I am not convinced that inter-op tests fall into this category, even if we check against go-libp2p's releases. The opposite is also true: A bug in rust-libp2p shouldn't stall development on the go-libp2p side or flag a PR as broken when it actually isn't.

I would expect these tests to reveal mostly rust-libp2p issues when run against go-libp2p releases only. If that is not the case I think we should reconsider your proposal.

---

@laurentsenta would you mind doing the change to not run against go-libp2p master and accept the above suggestion by @galargh. I can then merge.

[thomaseizinger]

Concretely, I think it makes sense to check things at PR merge time where we can say with high certainty that it is a bug in our software (rust-libp2p). I am not convinced that inter-op tests fall into this category, even if we check against go-libp2p's releases. The opposite is also true: A bug in rust-libp2p shouldn't stall development on the go-libp2p side or flag a PR as broken when it actually isn't.

I would expect these tests to reveal mostly rust-libp2p issues when run against go-libp2p releases only. If that is not the case I think we should reconsider your proposal.

If our quest is to have interoperability with go-libp2p then yes. I hope that we will move towards all implementations being interoperable because they conform to the same spec, at which point I think the burden of running inter-op tests should be in neither repository but in some other place.

Only testing against releases actually makes this situation even more complicated. You have to assume that people depend on the behaviour of software that has been released. Discovering inter-op problems after you've released a software is one step too late because it is much easier to say "well, people already depend on this behaviour now so all other implementations will have to follow". If we want to move towards spec-driven compliance, I think this PR may be a step in the wrong direction.

Like I said above, my concern is not blocking but this step is worrying me a bit.

[laurentsenta]

Discovering inter-op problems after you've released a software is one step too late because it is much easier to say "well, people already depend on this behaviour now so all other implementations will have to follow".

But that's already the case right? with spec + conformance testing or not, if we release a broken behavior it's now a requirement for backward compatibility.

Some context to make sure we're on the same page:

• We have a single repository for interoperability tests, libp2p/test-plans. /every/ libp2p implementation will use that repo.
• go-libp2p is already running these interop tests on every PRs.

I get your point against releasing "broken" behaviors and turning them into de-facto specifications. And the worries against go-libp2p pushing a change to master that breaks the rust-libp2p CI.

But that's the core of this PR:

• Interop is a first-class citizen: you should not be able to break interop unknowingly.
• Interop is a shared problem: we assume other implementations are running the same tests, in exchange, we take the responsibility of running the tests ourselves. If "I" want to break interop, "I" have to sync with other implementations until it's not a breaking change anymore.

[thomaseizinger]

Some context to make sure we're on the same page:

* We have a single repository for interoperability tests, `libp2p/test-plans`. /every/ libp2p implementation will use that repo.

* `go-libp2p` is already running these interop tests on every PRs.

I didn't know that, thanks for clarifying!

I get your point against releasing "broken" behaviors and turning them into de-facto specifications. And the worries against go-libp2p pushing a change to master that breaks the rust-libp2p CI.

But that's the core of this PR:

* Interop is a first-class citizen: you should not be able to break interop unknowingly.

* Interop is a shared problem: we assume other implementations are running the same tests, in exchange, we take the responsibility of running the tests ourselves. If "I" want to break interop, "I" have to sync with other implementations until it's not a breaking change anymore.

This makes sense yes. I guess if every implementation is running the interop tests on every PR against each other's releases, you shouldn't be able to break (tested) interop.

Good to go from my end!

[mxinden]

For the record, not testing go-libp2p master on every rust-libp2p pull request is happening through libp2p/test-plans#37. Once that lands, I will merge here.

[laurentsenta]

Features merged, PR updated, and ready to get in.
Thanks a lot for the detailed feedback, reviews, and patches @thomaseizinger @mxinden @galargh!

Don't hesitate to tag me or the team team/ipdx with any issue or error you're encountering,
no doubt we'll iterate on these, as far as I know, this is the first time we're implementing such tests. Learning & improving is the goal here!

[mxinden]

🎉 I am glad that this is happening. Thanks everyone, especially @laurentsenta.