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

Fixes: nomenclature confusion in XCM tests #7808

Closed
wants to merge 2 commits into from

Conversation

xeodus
Copy link

@xeodus xeodus commented Mar 5, 2025

Refactor XCM Nomenclature

This PR resolves confusing naming conventions in XCM emulator tests. The key changes address a common pain point where runtime implementations were incorrectly named "pallets" in macro-generate code.

Core Changes

  • Renamed [<$name ParaPallet>] to [<$name ParaRuntime>] in decl_test_parachains! macro
  • Fixed the naming confusion in XCM emulator tests from <Rococo as RococoPallet>::XcmPallet::force_xcm_version to <Rococo as RococoRuntime>::XcmPallet::force_xcm_version
  • Documentation for how the decl_test_parachains macro works with emulated chains for both substrate devs and internal developers

@xeodus xeodus marked this pull request as ready for review March 5, 2025 16:18
@bkchr bkchr requested review from bkontur and acatangiu March 5, 2025 22:44
@acatangiu
Copy link
Contributor

Not sure if this rename is worth it as it introduces a lot of changes everywhere it's used: all runtime tests. It will require the name changes to https://github.com/polkadot-fellows/runtimes/ too.

It introduces a lot of noise (changes) for not much reason. I would not do it.

Even so, @xeodus if you insist on it and want to spend the time doing it all the way, I will not block it. You can start by making all emulated tests compile/pass to get an idea of all the required changes.

cargo test -p asset-hub-rococo-integration-tests -p asset-hub-westend-integration-tests -p bridge-hub-rococo-integration-tests -p bridge-hub-westend-integration-tests -p collectives-westend-integration-tests -p coretime-rococo-integration-tests -p coretime-westend-integration-tests -p people-rococo-integration-tests -p people-westend-integration-tests

@bkontur
Copy link
Contributor

bkontur commented Mar 6, 2025

Not sure if this rename is worth it as it introduces a lot of changes everywhere it's used: all runtime tests. It will require the name changes to https://github.com/polkadot-fellows/runtimes/ too.

It introduces a lot of noise (changes) for not much reason. I would not do it.

Even so, @xeodus if you insist on it and want to spend the time doing it all the way, I will not block it. You can start by making all emulated tests compile/pass to get an idea of all the required changes.

cargo test -p asset-hub-rococo-integration-tests -p asset-hub-westend-integration-tests -p bridge-hub-rococo-integration-tests -p bridge-hub-westend-integration-tests -p collectives-westend-integration-tests -p coretime-rococo-integration-tests -p coretime-westend-integration-tests -p people-rococo-integration-tests -p people-westend-integration-tests

Exactly 👆

@xeodus I see you basically picked this #2416 issue.

If you want to help with integration emulated tests, maybe much more useful/helpful would be de-duplicate and simplify existing tests themself, so we could easier maintain and/or reuse for polkadot-fellows also.

Or check/verify something from these:
#1127 (not sure if it is still an issue)
#2460 (not sure if it is still an issue)
paritytech/roadmap#50 (here several other related issues to check)

@acatangiu
Copy link
Contributor

Please re-open if you plan on continuing, but after you get the tests compiling locally:

cargo test -p asset-hub-rococo-integration-tests -p asset-hub-westend-integration-tests -p bridge-hub-rococo-integration-tests -p bridge-hub-westend-integration-tests -p collectives-westend-integration-tests -p coretime-rococo-integration-tests -p coretime-westend-integration-tests -p people-rococo-integration-tests -p people-westend-integration-tests

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.

3 participants