chore: chopsticks #650
chore: chopsticks #650
Conversation
Ad96el
force-pushed
the
ag_chopsticks_design
branch
from
c70704b to
f4faede
Compare
Ad96el
force-pushed
the
ag_chopsticks_design
branch
from
f4faede to
74b2fa5
Compare
…ode into ag_chopsticks_design
|
It might happen that that still errors are thrown by some concurrence issues regarding SQLite. We could either ignore those errors, since they are not part of the testing logic, or run the test not in parallel, which would result into a slower CI (currently 10 to maybe 15 mins). Resolving those issues seems challenging since they come directly from chopsticks. Ignoring the errors could result in some unforeseen side effects, which could give false positive tests. Manually rerunning the tests is also an option. |
There was a problem hiding this comment.
I will definitely give it another review once the comments are addressed and the conflicts with develop are resolved. In general, I think the new approach is viable, as long as it is not too much focused on our current set of tests but could be expanded with relatively little effort, which is the main goal of this PR, besides improving test reliability. Hence I would suggest to make all the different components more generic. I have not gone into the test logic itself yet, and I'll keep that for my new review!
| @@ -68,16 +68,20 @@ integration-tests: | |||
| image: paritytech/ci-unified:bullseye-1.70.0 | |||
| stage: test | |||
| variables: | |||
| CI: "true" | |||
| SPIRITNET_BLOCK_NUMBER: 6390611 | |||
There was a problem hiding this comment.
Please move these to the relevant part of our new GH workflow (when merging conflicts with develop).
| SPIRITNET_BLOCK_NUMBER: 6390611 | ||
| HYDRADX_BLOCK_NUMBER: 5235787 | ||
| POLKADOT_BLOCK_NUMBER: 21010819 | ||
| SPIRITNET_WASM_OVERRIDE: "../../target/debug/wbuild/spiritnet-runtime/spiritnet_runtime.compact.compressed.wasm" |
There was a problem hiding this comment.
No compact WASM is generated anymore, so this should refer to the regular, uncompressed, uncompacted one.
| }, | ||
| "scripts": { | ||
| "ts-check": "tsc --noEmit", | ||
| "lint": "eslint src && prettier --check src", | ||
| "lint:fix": "eslint --fix src && prettier --write src", | ||
| "clean": "rm -rf ./db", | ||
| "test": "LOG_LEVEL=error vitest", | ||
| "test:CI": "vitest --bail 0 --no-file-parallelism" | ||
| "test": "LOG_LEVEL=error vitest" |
There was a problem hiding this comment.
Definitely lower priority, but we should investigate how to make the tests run faster without them throw errors. Our new GH pipeline currently takes almost 1h, but I have no idea if this refactoring will make tests go faster or slower.
| # e2e Chopsticks tests | ||
|
|
||
| This project is a set of end-to-end tests for the KILT protocol against other parachains. | ||
| Other functionalities such as a creation of DID can be easy inserted. |
There was a problem hiding this comment.
| Other functionalities such as a creation of DID can be easy inserted. | |
| Other functionalities such as a creation of DID can be easily added. |
| POLKADOT_PORT= | ||
| SPIRITNET_WS= | ||
| SPIRITNET_PORT= | ||
| # Those env variables are mandatory for the tests to work. |
There was a problem hiding this comment.
| # Those env variables are mandatory for the tests to work. | |
| # These env variables are mandatory for the tests to work. |
| @@ -37,12 +44,13 @@ export function assignKiltTokensToAccounts(addr: string[], balance: bigint = ini | |||
| } | |||
| } | |||
|
|
|||
| // Sibling Sovereign Account | |||
| export const siblingSovereignAccount = '5Eg2fntQqFi3EvFWAf71G66Ecjjah26bmFzoANAeHFgj9Lia' | |||
There was a problem hiding this comment.
I think it would also be better to group "required" info for each parachain, such as its sibling account and para ID into its own object with its own defined type. Everything else is extra, which can extend the basic object, kinda like you did for the tests setup.
| /* | ||
| * Object with all the chain configurations. | ||
| */ | ||
| export const all: ChainConfigs = { |
| /* | ||
| * Get an environment variable and throw an error if it is not set. | ||
| */ | ||
| function getEnvVariable(name: string): string { |
There was a problem hiding this comment.
Then I would rename this function to:
| function getEnvVariable(name: string): string { | |
| function getRequiredEnvVariable(name: string): string { |
| */ | ||
| export async function setupNetwork( | ||
| relayChain: SetupOption, | ||
| sender: SetupOption, |
There was a problem hiding this comment.
This as well could involve configuring more than a single receiver chain, so it should probably be generalized.
There was a problem hiding this comment.
If you need to treat the realy chain differently because of a different function name, then I would simply have the relay chain argument and a list of chains to all connect horizontally with each other and vertically with the relaychain. As far as I can see, you don't need to know who the sender is here.
| @@ -28,11 +37,7 @@ export function setGovernance(addr: string[]) { | |||
|
|
|||
| /// Spiritnet ParaId | |||
| export const paraId = 2086 | |||
| export const KILT = { Concrete: { parents: 0, interior: 'Here' } } | |||
There was a problem hiding this comment.
The naming of this could be improved. What this is, is the Here location within the context of the chain itself. So perhaps just call it HERE?
fixes #3252
Bye-bye flaky tests 👋
I am still not super happy with the typing of the tests. Any feedback would be great.
I also added a README.md where adding a new test case is briefly explained.
Additional tests against the develop state are also introduced. Any change to the current XCM configuration should be tested against the currently open channels.
The current design should be generic enough to test the XCM configuration against any chains. Additionally, any XCM version can be easily tested by adding a new test suite case.