polkadot-fellows / runtimes

The various runtimes which make up the core subsystems of networks for which the Fellowship is represented.
GNU General Public License v3.0
139 stars 94 forks source link

Add `xcm-emulator` for (at least some) testing of relay and system parachains runtimes #103

Closed acatangiu closed 9 months ago

acatangiu commented 10 months ago

Other than try-runtime migrations and some very basic unit tests the code for these runtimes is really not tested (IMO dangerously under-tested).

For example, core misconfigurations can be easily introduced by accident and currently we're relying just on manual reviews to catch them - when they could easily be caught by (mostly existing) automated testing.

I heard about some plans of adding some chopsticks-based tests in the future, which sounds great! But in the meantime let's at least use what we have, namely xcm-emulator (and even some zombienet tests if we already have ones that cover good or critical chunks of runtime functionality).

acatangiu commented 10 months ago

P.S. This is not just me complaining, I am happy to add above ^^^ myself if fellows agree

xlc commented 10 months ago

To be honest, I just don't like xcm-emulator. The code is unstable, tests are not easy to write, it requires everything on a same version so can't be used to test compatibility across versions, which is pretty much the source of all the production bugs.

If it only takes say half day to reintroduce existing tests then I will be fine with that. Otherwise someone really need to start working on chopsticks based tests. I don't have permission to assign people but I would want to make it the top priority issue and qualified people cannot pick on other tasks until this is addressed.

acatangiu commented 10 months ago

If it only takes say half day to reintroduce existing tests then I will be fine with that.

Porting existingxcm-emulator tests from polkadot-sdk testnets to the prod runtimes here shouldn't take more than a day.

joepetrowski commented 10 months ago

To be honest, I just don't like xcm-emulator.

Nobody is making you use it. A lot of people do like it.

They are also not exclusive. I fully support a Chopsticks-based test suite.

xlc commented 10 months ago

The main issue I have is that I found people are keep repeating the mistake I made long time ago and I cannot stop it. I am the person proposed the idea and Acala is the original team implemented the simulator and emulator and the tests. By some definition I am more experienced on this topic than anyone. And then I decided to not using it anymore for many solid reasons. I could write all the reasons down one more time if people wanting to know. But anyway, I don’t have the authority to instruct people what to do so whatever. Also it could be me wrong and maybe you can overcome the blocks I had with emulator. So I will only just do what I always doing: make suggestions or complaints and no hard feelings if you decide to ignore them.

NachoPal commented 10 months ago

Please check this comment: https://github.com/polkadot-fellows/runtimes/pull/114#issuecomment-1853917368 We are planning to move the tests to its own repository

acatangiu commented 10 months ago

Please check this comment: #114 (comment) We are planning to move the tests to its own repository

Why would they be on a different repo? I mean I don't see any benefits, while seeing massive drawbacks. It's already hard enough to coordinate over polkadot-sdk and polkadot-fellows, why would we make it even harder?

Tests for these runtimes should be next to the code. Fine to get some infrastructure from polkadot-sdk. Definitely don't like yet another repo.

NachoPal commented 10 months ago

Why would they be on a different repo? I mean I don't see any benefits, while seeing massive drawbacks. It's already hard enough to coordinate over polkadot-sdk and polkadot-fellows, why would we make it even harder?

Tests for these runtimes should be next to the code. Fine to get some infrastructure from polkadot-sdk. Definitely don't like yet another repo.

I can give you one reason. If we keep them on its own repository we can create a space where other teams can add and reuse other's chains/networks definitions. The idea is not to add only Relay/System Parachains tests, it is to create a common place for others to add their tests agains Relay/System Parachains or other Parachains.

Another reason is to keep the runtimes repo as clean as possible, strictly keeping only the runtimes. Not having to maintain the xcm-emulator tests here will simplify and make more agile the release process.

I am not against of adding the tests here, but the two options have their benefits.

Probably @joepetrowski to chip in here.

acatangiu commented 10 months ago

I think that model is broken. Having all the tests of the ecosystem in one repo cannot work - you have to change the universe when changing some relay param.

Tests should follow code and follow same development workflow. Each project should have its tests next to its code, and it should bring in deps of other projects (whether prod or test) through cargo.

When chain BLA wants to test against Kusama relay, it takes a cargo crates.io or git dep to some locally-controlled version of kusama-relay-definitions module from fellowship runtimes repo and defines its tests next to its code. That way it controls both what version of relay to use, and how it uses it.


Regarding tests for https://github.com/polkadot-fellows/runtimes we should have tests next to code in order to run CI on them, we don't want to merge broken stuff, we want automated testing.

Local CI cannot (sanely) depend on another repo for tests - it's chicken and egg - you need the new feature/enhancement in prod code to define the test, and you need the test to merge the new feature/enhancement. I don't see any sane way to do it properly if the two live in separate repos, and cannot be changed/enhanced "atomically" (same single PR).

xlc commented 10 months ago

Please check this comment: #114 (comment) We are planning to move the tests to its own repository

Well, another example that someone making a decision without any public visible discussions and explanations. Can we stop doing it?

We absolutely need tests next to the code otherwise how can people work on features like https://github.com/polkadot-fellows/runtimes/pull/125

Yes I can see some valid concerns having the e2e tests in this repo. But there are multiple alternatives on top of a separate repo. e.g using submodule, cross repo CI setup, or just make the code better, etc. But we can't explore and discuss the alternatives if there are no public discussions.

I can give you one reason. If we keep them on its own repository we can create a space where other teams can add and reuse other's chains/networks definitions. The idea is not to add only Relay/System Parachains tests, it is to create a common place for others to add their tests agains Relay/System Parachains or other Parachains.

That's not a reason to not have ANY tests in this repo. Yeah a common place for ecosystem wide tests could be useful, but that's something else.

Another reason is to keep the runtimes repo as clean as possible, strictly keeping only the runtimes. Not having to maintain the xcm-emulator tests here will simplify and make more agile the release process.

You are basically saying not having tests can make everything simpler and faster. Yeah sure. I hope I don't need to tell you what's going to happen.

joepetrowski commented 10 months ago

Well, another example that someone making a decision without any public visible discussions and explanations. Can we stop doing it?

"Planning" to do something is not making a decision. This issue and PR are open. This is the public discussion. No Hollywood drama needed.

For the reasons @xlc and @acatangiu have said, I also prefer to have the tests here. Some people (@xlc included) have voiced opposition to having Emulator tests in the Fellowship repo. Therefore, @NachoPal and I planned to set up tests in a new repo, to keep those people happy and have our existing test coverage with a platform where other parachain teams can add tests using the same environment.

I really don't understand these seemingly dogmatic opinions on test tools. Emulator is not perfect. It's especially useful for fast development, like troubleshooting in adding new features and keeping a test to avoid regression. It has detected real issues and stopped bugs from going into production releases. Chopsticks is also useful in a different way. I fully support a large amount of tests either duplicated or moved fully to Chopsticks.

However, we actually have emulator tests right now, and I think it's insanity that people would prefer not to test runtimes at all than to bring in the test coverage we had in the Polkadot/Cumulus release process; instead we just continue to argue about whether Emulator is perfect or not (it's not) and make releases with zero test coverage.

acatangiu commented 10 months ago

Great! Looks like we're all aligned. @xlc @joepetrowski (as only fellows actively participating in discussion) please review https://github.com/polkadot-fellows/runtimes/pull/114 - it's pretty straightforward to review and it doesn't have to be perfect as long as we're always moving towards the right goal - take aim, right foot, left foot, we'll get there! 😄

NachoPal commented 10 months ago

Well, another example that someone making a decision without any public visible discussions and explanations. Can we stop doing it?

It has been openly discussed as part of the roadmap here. In this comment is where I mention about the POC to add the integration tests.

My opinion, since the beginning, has been always having the tests in this repo, but as @joepetrowski mentioned, some people voiced opposition to having Emulator tests in the Fellowship repo.

You are basically saying not having tests can make everything simpler and faster. Yeah sure. I hope I don't need to tell you what's going to happen.

No, I am not saying that. I was saying to have the tests (and run them) somewhere else. That repository would import the runtimes from this repository.

xlc commented 10 months ago

Sorry for overreacting and giving mixed signals. I can see I caused some confusion. Apologize and let me re-clarify my opinions.

If someone is going to write new tests from scratch, I would like this person to spend time with Chopsticks instead of emulator because Chopsticks can test everything that emulator can test, but emulator cannot test everything Chopsticks can test. For example, compatibility between different runtime versions. We had many XCM issue caused by V2 and V3 conversion, which is hard to test with emulator if both chains we using the latest XCM.

But, in this case, we already have a bunch of existing emulator tests. As I said in my previous comment, I don't mind people to spend a little time to integrate them. That's the most time effective task to do. Also have tests is better than no tests. And emulator tests are clearly catching bugs so it is better than nothing.

It is just that we clearly have more work on emulator done than Chopsticks, which to me isn't the right priority. I am very upset that it has been I don't know how many weeks and still no one is able to port existing Chopsticks tests to here.

The ultimate goal is that I want to make sure of all that parameter changes are properly tested. Right now they are not, at least not in the PR making the change. e.g. #125. This have multiple issues:

bkontur commented 9 months ago

Can somebody, please, close this? It is already merged: https://github.com/polkadot-fellows/runtimes/pull/114

serban300 commented 6 months ago

I am very upset that it has been I don't know how many weeks and still no one is able to port existing Chopsticks tests to here.

@xlc I'm not sure if this is still actual, but could you point me to the existing chopsticks tests please ? I'm working both on XCM and bridge testing and I think they could be very helpful. Maybe I could even try to port them.

xlc commented 6 months ago

https://github.com/AcalaNetwork/e2e-tests