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

Extend end-to-end tests in people chains #63

Merged
merged 28 commits into from
Oct 28, 2024

Conversation

rockbmb
Copy link
Collaborator

@rockbmb rockbmb commented Oct 17, 2024

No description provided.

This mistakenly caused identities to remain provisional
even after judgement had been passed, due to using the wrong
address as the registrar when setting up the chain - Bob instead of
Alice.
1. Create an identity
2. Request a judgement on it
3. Have it provided by the test-only Alice registrar

This basic test is already quite useful, and can now be extended
into other more complex scenarios.
This is required to test pallet extrinsics that require certain origins
to be executed e.g. the identity pallet's `addRegistrar` function.

The original source for the idea can be found in https://hackmd.io/@xlc/H11BX5BLa#Dispatch-Call
@rockbmb rockbmb changed the title People chain e2e tests Extend end-to-end tests in people chains Oct 17, 2024
@rockbmb rockbmb force-pushed the people-chain-e2e-tests branch 2 times, most recently from 989753a to 374fd91 Compare October 18, 2024 19:14
@rockbmb rockbmb force-pushed the people-chain-e2e-tests branch from 374fd91 to 52f6914 Compare October 18, 2024 19:36
Also refactor tests, add some comments and documentation.
@rockbmb rockbmb force-pushed the people-chain-e2e-tests branch from 1034398 to 0b64c78 Compare October 21, 2024 17:25
@rockbmb rockbmb marked this pull request as ready for review October 21, 2024 17:27
@xlc
Copy link
Member

xlc commented Oct 21, 2024

can you push the yarn.lock file change

@rockbmb
Copy link
Collaborator Author

rockbmb commented Oct 22, 2024

@xlc Hello!

It's already in the tree, and mine hasn't changed compared to master. Did you detect any issues?

(As an experiment) can I ask that you

  1. modify this line in the subidentity test from await peopleClient.api.rpc('dev_newBlock', { count: 2 }) to await peopleClient.api.rpc('dev_newBlock', { count: 1 })
  2. try running yarn test people.e2e.test -t "setting on-chain identity, adding sub-identities"
    from the latest commit?

Subidentity extrinsics fail to be applied immediately after 1 block, but if you try the same thing in a local polkadot.js fork, it will work correctly - odd.

@xlc
Copy link
Member

xlc commented Oct 22, 2024

The CI is failing and you have modified package.json without modify yarn.lock.

@xlc
Copy link
Member

xlc commented Oct 22, 2024

also I don't really want any code in the root package. please have the polkadot tests in the polkadot packages under ./packages/polkadot/src, and shared helper in ./packages/shared/src

this means you should add the deps to the shared package, not the root one

@xlc
Copy link
Member

xlc commented Oct 22, 2024

Few more points:

  • you should use .dev instead of dev rpc. e.g. await peopleClient.dev.newBlock({ count: 2 })
  • I highly recommend use snapshots over manually writing assertions. You will see the value (and asserting it remains the same). So you don't need for example print the value and delete the print statement later.
  • If you are unsure why something isn't going as expected, increase the test timeout in vitest.config.mts file and add peopleClient.pause() to pause the test. Check the logs, it will show the ports of the chopsticks instance and you can use pjs apps to connect to it and inspect the events / send tx etc.

@rockbmb
Copy link
Collaborator Author

rockbmb commented Oct 22, 2024

@xlc thank you for the review!

I will address your comments in coming commits.

@rockbmb
Copy link
Collaborator Author

rockbmb commented Oct 26, 2024

@xlc I've addressed your review - could you take another look?

@xlc
Copy link
Member

xlc commented Oct 26, 2024

CI is still failing due to yarn.lock file, otherwise looks fine

@rockbmb
Copy link
Collaborator Author

rockbmb commented Oct 26, 2024

@xlc true (again) - apologies for the oversight. I'll push a fix.

@xlc
Copy link
Member

xlc commented Oct 26, 2024

the lint error indicates multiple versions of chopsticks are included, which is not good. you can confirm it by looking the yarn.lock file
do a yarn update chopsticks should resolve the error. make sure you do yarn lint locally to confirm. otherwise delete yarn.lock file and regenerate it could also work. but that will update other packages as well, which isn't bad, but just may have unexpected side effects

@rockbmb
Copy link
Collaborator Author

rockbmb commented Oct 27, 2024

yarn lint does confirm the issue.

In the end, I simply rebuilt yarn.lock. I inspected it, and there is now only one version of @acala-network/chopsticks being used.

@rockbmb
Copy link
Collaborator Author

rockbmb commented Oct 27, 2024

I've found the issue with the latest CI failures. Pushing a fix.

This is because people E2E tests require Bob's address to be loaded with
funds, which interferes with the people chain XCM tests' snapshots.
@xlc xlc merged commit 5f93dcf into open-web3-stack:master Oct 28, 2024
17 checks passed
@rockbmb rockbmb deleted the people-chain-e2e-tests branch October 28, 2024 12:21
github-merge-queue bot pushed a commit to paritytech/polkadot-sdk that referenced this pull request Nov 7, 2024
# Description

E2E tests to Polkadot/Kusama's people chains (in
open-web3-stack/polkadot-ecosystem-tests#63)
revealed that 2 of the identity pallet's extrinsics did not emit events
in case of success:
* `pallet_identity::rename_sub`, and
* `pallet_identity::set_subs`

This PR fixes that.

## Integration

Other than 2 extrinsics emiting an event when previously they did not,
no other behavior in pallets/extrinsics was modified, so no integration
is needed.

## Review Notes

N/A
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.

2 participants