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

Add chopsticks workflow #24

Closed

Conversation

samelamin
Copy link
Contributor

@samelamin samelamin commented Sep 8, 2023

This PR attempts to integrate chopsticks into our CI/CD process. See #23

the goal of this is so we have a framework in place to ensure any and all commits are tested against a live chain state to catch any issues that other unit or integration tests might not catch.

Initially, I added a simple test to assert we can do xcm transfers between the relay chain and assethub and while this is a good first step, we can expand on this to cover all sorts of state transitions that might cause chains to brick,e.g. runtime upgrades, large migrations,etc

My only question/concern is that we might be tightly coupling our CI runs with the e2e test repo managed by acala but even if we break that dependency we are still dependent on chopsticks. Also, I do feel that having a defined best practice on testing chains might go a long way to stop and prevent chains from breaking in prod

TLDR; chopsticks + CI/CD.

Happy to hear feedback and change this approach to how people see fit

@samelamin samelamin force-pushed the add_chopsticks_workflow branch 9 times, most recently from f2da4ea to aaa129f Compare September 9, 2023 18:25
@samelamin samelamin marked this pull request as draft September 9, 2023 18:27
@samelamin samelamin force-pushed the add_chopsticks_workflow branch 2 times, most recently from 7ea5428 to 1d64499 Compare September 9, 2023 18:45
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good start, thanks!

.github/workflows/chopsticks.yml Outdated Show resolved Hide resolved
.github/workflows/chopsticks.yml Outdated Show resolved Hide resolved
1) remove nightly
2) restrict to runtimes
3) Add comment on what the last step does
Comment on lines +42 to +43
STATEMINE_WASM: "../target/release/wbuild/asset-hub-kusama-runtime/asset_hub_kusama_runtime.compact.compressed.wasm"
STATEMINT_WASM: "../target/release/wbuild/asset-hub-polkadot-runtime/asset_hub_polkadot_runtime.compact.compressed.wasm"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be Asset Hub throughout

Copy link
Contributor Author

@samelamin samelamin Sep 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The e2e repo needs updating because its classed as Statemine/Statemint still

https://github.com/AcalaNetwork/e2e-tests/blob/master/networks/statemint.ts


export default function buildTest(tests: ReadonlyArray<TestType>) {
for (const { from, to, test, name, ...opt } of tests) {
describe(`'${from}' -> '${to}' xcm transfer '${name}'`, async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"xcm transfer" is really vague; it doesn't say what it's actually testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the shared file that all tests use, I can rename but the idea is to keep all xcm related chopsticks tests there

name: 'KSM',
test: {
xcmPalletDown: {
tx: tx.xcmPallet.limitedTeleportAssets(kusama.ksm, 1e12, tx.xcmPallet.parachainV3(0, statemine.paraId)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the function signature of limitedTeleportAssets. Where is this stuff defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hardcodes in stuff like parents: 0, so cannot be used to test teleporting between system parachains. I know this PR is just the CI/CD but it will need to cover a large set of test cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, very good point. I think we as a community can evolve the e2e test repo so it can handle far more test cases.

@bkchr
Copy link
Contributor

bkchr commented May 15, 2024

Stale.

@bkchr bkchr closed this May 15, 2024
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.

4 participants