Add xcm-emulator for (at least some) testing of relay and system parachains runtimes
#103
Comments
|
P.S. This is not just me complaining, I am happy to add above ^^^ myself if fellows agree |
|
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. |
Porting existing |
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. |
|
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. |
|
Please check this comment: #114 (comment) |
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 Tests for these runtimes should be next to the code. Fine to get some infrastructure from |
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 I am not against of adding the tests here, but the two options have their benefits. Probably @joepetrowski to chip in here. |
|
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 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). |
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 #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.
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.
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. |
"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. |
|
Great! Looks like we're all aligned. @xlc @joepetrowski (as only fellows actively participating in discussion) please review #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! 😄 |
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.
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. |
|
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:
|
This PR adds XCM emulator tests for Relay chains and Asset Hub System parachains. It adds them under new `integration-tests/emulated/` dir which lives at the root of the repo and doesn't pollute the prod networks code/crates/etc in any way. As you can see from the `diff`, no other files outside `integration-tests` are touched by this PR (except workspace `Cargo.toml` and `Cargo.lock`). Tests added by this PR are rather old/limited, but I will update them when we bump polkadot-sdk crates deps to `v1.4` or `v1.5` release. This will bring in more refined tests for existing scenarios and many new tests for new scenarios such as complex asset-transfers and bridging - sending XCMs over bridge, and asset transfers between AHs on different relays using bridge. Also after bumping repo to sdk `v1.4` or `v1.5` release, we can also drop 90% of `integration-tests/emulated/common` and get it from `crates.io`. Fixes polkadot-fellows#103 --------- Co-authored-by: Bastian Köcher <[email protected]>
|
Can somebody, please, close this? |
@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. |
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, namelyxcm-emulator(and even somezombienettests if we already have ones that cover good or critical chunks of runtime functionality).The text was updated successfully, but these errors were encountered: